*
The moose likes Beginning Java and the fly likes cannot read a Collection of a custom class as a parameter Big Moose Saloon
  Search | Java FAQ | Recent Topics | Flagged Topics | Hot Topics | Zero Replies
Register / Login


Win a copy of Java 8 in Action this week in the Java 8 forum!
JavaRanch » Java Forums » Java » Beginning Java
Bookmark "cannot read a Collection of a custom class as a parameter" Watch "cannot read a Collection of a custom class as a parameter" New topic
Author

cannot read a Collection of a custom class as a parameter

Jon Camilleri
Ranch Hand

Joined: Apr 25, 2008
Posts: 659

Any idea why my code hangs?



Performance seems like a drag when running this program as well, see jconsole screenshot here.


Jon
Bear Bibeault
Author and ninkuma
Marshal

Joined: Jan 10, 2002
Posts: 60082
    
  65

Surely you can provide more detail than "my code crashes".


[Asking smart questions] [Bear's FrontMan] [About Bear] [Books by Bear]
Jon Camilleri
Ranch Hand

Joined: Apr 25, 2008
Posts: 659

Bear Bibeault wrote:Surely you can provide more detail than "my code crashes".


The output indicates that the lines 09 to 15 (Poker class) seems to hang up with no output being displayed, which calls the Dealer class (lines 274 to 286), and, was coded to return a Collection of the Card class back to the Poker class. Memory usage was also noted to scale up unexpectedly while the program was running, when monitoring this program using jconsole.



Stephan van Hulst
Bartender

Joined: Sep 20, 2010
Posts: 3401
    
    9
Hi Jon. I have several comments about your code. These are mostly personal preferences, but I strongly urge you to consider them:
- Put fields at the top of your code, and methods below them. It's hard to find out what your code is referring to when you have to scroll down to find the fields.
- Don't make one line methods like getSomething() {return something}. It's hard to distinguish them from fields (especially when generic types are involved).
- Place opening braces either on the same line as the method signature, or on a new line below it. Match the closing brace with the column of either the opening brace or the method signature. Choose one, and stick with it.
- Make all your fields private. Always. There is rarely a good reason to make them more visible than that.
- Don't use protected. There is rarely a good reason to make members protected, and only after long and hard consideration. Always prefer private and default access.
- Make internal classes static. This includes enums.
- Don't overuse inheritance. It's a powerful mechanism, but it violates encapsulation, a far more important concept. Conceptually a deck of cards is not a card, so don't make Deck extend Card. Conceptually, a dealer isn't a player, so don't make Dealer extend Player. As a matter of fact, make all your classes final unless you have a good reason not to.
- Try not to use identifiers already used by the standard API. Especially not well known class names like Number. This will confuse anyone who reads your code. Number is the supertype of all numeric classes, like Integer, Float, etc. Use a name like Rank instead.
- Limit the use of byte, short and long if you can. This will avoid the need to cast ints, which is the default integer type.
- Card is a prime example of a class you can make immutable. Right now there is a probable bug in it; namely a public constructor without parameters that does nothing. Calling this constructor will return a card that is in an inconsistent state.
- Consider overriding equals() and hashCode(). This will make your Card class a lot more easy to use.

Example:

In this example, I left in the (int, int) constructor, to demonstrate how you can make the switch more readable. Normally I would leave it out, and force the user to use the (Suit, Rank) constructor.
Stephan van Hulst
Bartender

Joined: Sep 20, 2010
Posts: 3401
    
    9
As for your problem, take a look at the Dealer class, the getAllCards() method. Ask yourself when the for loop will end.
Jon Camilleri
Ranch Hand

Joined: Apr 25, 2008
Posts: 659

Stephan van Hulst wrote:Hi Jon. I have several comments about your code. These are mostly personal preferences, but I strongly urge you to consider them:
- Put fields at the top of your code, and methods below them. It's hard to find out what your code is referring to when you have to scroll down to find the fields.
- Don't make one line methods like getSomething() {return something}. It's hard to distinguish them from fields (especially when generic types are involved).
- Place opening braces either on the same line as the method signature, or on a new line below it. Match the closing brace with the column of either the opening brace or the method signature. Choose one, and stick with it.
- Make all your fields private. Always. There is rarely a good reason to make them more visible than that.
- Don't use protected. There is rarely a good reason to make members protected, and only after long and hard consideration. Always prefer private and default access.
- Make internal classes static. This includes enums.
- Don't overuse inheritance. It's a powerful mechanism, but it violates encapsulation, a far more important concept. Conceptually a deck of cards is not a card, so don't make Deck extend Card. Conceptually, a dealer isn't a player, so don't make Dealer extend Player. As a matter of fact, make all your classes final unless you have a good reason not to.
- Try not to use identifiers already used by the standard API. Especially not well known class names like Number. This will confuse anyone who reads your code. Number is the supertype of all numeric classes, like Integer, Float, etc. Use a name like Rank instead.
- Limit the use of byte, short and long if you can. This will avoid the need to cast ints, which is the default integer type.
- Card is a prime example of a class you can make immutable. Right now there is a probable bug in it; namely a public constructor without parameters that does nothing. Calling this constructor will return a card that is in an inconsistent state.
- Consider overriding equals() and hashCode(). This will make your Card class a lot more easy to use.



Thanks for the feedback.

With regards the design of Deck and Cards, I thought I would extend Deck as a set of Cards, would you suggest other data structures to implement a set of Cards, perhaps an array of Cards[], rather than a parent? However taking the array[] approach, which is quite a problematic data structure from my experience in Java, would go against the notion of designing in terms of Objects (OOP), rather than rigging functional programming within OOP design. What is your advice?

Actually I dropped in the blank (inconsistent) constructor for Card simply to work around the need for Deck to have a constructor for Card without having to pass the Suit and the Rank.


Further reading
Bug 7037278 - ISSUE: boolean[] [] cannot be initialized - http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7037278

Jon Camilleri
Ranch Hand

Joined: Apr 25, 2008
Posts: 659

Stephan van Hulst wrote:
- Consider overriding equals() and hashCode(). This will make your Card class a lot more easy to use.



My application does not really require a hashCode at this stage, however I would like to know whether my classes make use of hash codes (so far I haven't made any use as far as I know), thus implementing an efficient hashCode would make my application more efficient?

On the other hand, "this method must be overridden in every class that overrides the equals method."
Source from http://www.technofundo.com/tech/java/equalhash.html.

Sun have already encoded a hashCode, hence, I could theoretically speaking get away with it, right? Why would I override functionality written by Sun Microsystems software engineers, and, re-invent the wheel?

Source: hashCodeObject.hashCode() documentation at the Java 1.6 API, and, at http://download.oracle.com/javase/1.4.2/docs/api/java/lang/Object.html...lookfor hashCode().

It would be nice if Sun (Oracle) decided to make the option of adding #methodName() or #property to the end of the URL.

General information about Java hashCode() at this wikipedia article.

However, having done some reading it seems necessary to learn a bit about hashCode() and equals() for SCJP 1.4; not sure what they expect for 1.6 assuming it is available

" This property is important to the performance of hash tables and other data structures that store objects in groups ("buckets") based on their computed hash values."


Having had second thoughts, I have updated a hashCode() that generates a random number, however, reliability of the uniqueness of the hashCode() is dependent on the probability of the Random class generating the same number. The hashCode that was suggested was based on hashCode() that should be generated from enums Suit and Rank, within the Card class, and, it seemed too intriguing to follow that error-prone logic, however, I am open to suggestions in this regard.


Stephan van Hulst
Bartender

Joined: Sep 20, 2010
Posts: 3401
    
    9
Jon Camilleri wrote:With regards the design of Deck and Cards, I thought I would extend Deck as a set of Cards, would you suggest other data structures to implement a set of Cards, perhaps an array of Cards[], rather than a parent? However taking the array[] approach, which is quite a problematic data structure from my experience in Java, would go against the notion of designing in terms of Objects (OOP), rather than rigging functional programming within OOP design. What is your advice?

Simply use a List to store cards. Your Poker class can keep a List<Card> as the deck, and each player can keep a List<Card> as their hand.

Actually I dropped in the blank (inconsistent) constructor for Card simply to work around the need for Deck to have a constructor for Card without having to pass the Suit and the Rank.

You've run into this problem because you extended Card in the first place, which you shouldn't have. Only extend classes when you can say "these two types of things are the same, the one is simply more specialized than the other". A deck of cards is not the same thing as a card. A deck doesn't have a suit, it doesn't have a rank, it doesn't have a value. They are two conceptually different things. Keep that in mind when you extend classes.

My application does not really require a hashCode at this stage, however I would like to know whether my classes make use of hash codes (so far I haven't made any use as far as I know), thus implementing an efficient hashCode would make my application more efficient?

The equals method is very useful for various purposes. For value classes (if you make Card immutable, it is a perfect value class) you should almost always override the equals method. And like the contract states, if you override equals, you should also override hashCode.

On the other hand, "this method must be overridden in every class that overrides the equals method."
Source from http://www.technofundo.com/tech/java/equalhash.html.

Sun have already encoded a hashCode, hence, I could theoretically speaking get away with it, right? Why would I override functionality written by Sun Microsystems software engineers, and, re-invent the wheel?

This is not how it works. hashCode does not provide some functionality that has to be implemented only once. If you override equals, you *have to* override hashCode, because if two objects are meaningfully equal, they have to return the same hashCode. Object's default implementation of hashCode can not guarantee this, because as far as Object is concerned, each instance is unique. It's a bit like the toString method. What it returns is specific to the class overriding it, and Object can not determine what String is appropriate to return.

Having had second thoughts, I have updated a hashCode() that generates a random number, however, reliability of the uniqueness of the hashCode() is dependent on the probability of the Random class generating the same number. The hashCode that was suggested was based on hashCode() that should be generated from enums Suit and Rank, within the Card class, and, it seemed too intriguing to follow that error-prone logic, however, I am open to suggestions in this regard.

You should never ever ever return a random value in hashCode, because you can not guarantee that two objects that are equal will return the same hashCode (which is an absolute requirement). The implementation I provided above should work fine, and you are free to use it.
Simply said, whatever information you base your decision on in the equals method, you should also base your hashCode on. As you can see in the methods I provided, the return value in both methods is based on the Rank and Suit of the card. This makes sense, because these two properties define whether two different card instances have the same value.
Jon Camilleri
Ranch Hand

Joined: Apr 25, 2008
Posts: 659

Having had second thoughts, I have updated a hashCode() that generates a random number, however, reliability of the uniqueness of the hashCode() is dependent on the probability of the Random class generating the same number. The hashCode that was suggested was based on hashCode() that should be generated from enums Suit and Rank, within the Card class, and, it seemed too intriguing to follow that error-prone logic, however, I am open to suggestions in this regard.

You should never ever ever return a random value in hashCode, because you can not guarantee that two objects that are equal will return the same hashCode (which is an absolute requirement). The implementation I provided above should work fine, and you are free to use it.
Simply said, whatever information you base your decision on in the equals method, you should also base your hashCode on. As you can see in the methods I provided, the return value in both methods is based on the Rank and Suit of the card. This makes sense, because these two properties define whether two different card instances have the same value.

Thank you, however, I could not figure out how to include the Rank and Suit of the card, since being Enums I was coming across too many casts that I could not resolve when trying to code a .hashCode that basically:

1. Store an arbitrary non-zero constant integer value (say 7) in an int variable, called hash.
2. Involve significant variables of your object in the calculation of the hash code, all the variables that are part of equals comparison should be considered for this. Compute an individual hash code int var_code for each variable var as follows -
a. If the variable(var) is byte, char, short or int, then var_code = (int)var;
3. Combine this individual variable hash code var_code in the original hash code hash as follows -
hash = 31 * hash + var_code;
Follow these steps for all the significant variables and in the end return the resulting integer hash.
4. Lastly, review your hashCode method and check if it is returning equal hash codes for equal objects. Also, verify that the hash codes returned for the object are consistently the same for multiple invocations during the same execution.

Sourced from Equals and Hash Code in Java article.

Stephan van Hulst
Bartender

Joined: Sep 20, 2010
Posts: 3401
    
    9
Again, look at the example I provided. It does exactly this. Read it, test it, and tell us if there's something you don't understand.
Jon Camilleri
Ranch Hand

Joined: Apr 25, 2008
Posts: 659

Stephan van Hulst wrote:Again, look at the example I provided. It does exactly this. Read it, test it, and tell us if there's something you don't understand.

Yes indeed, it's correct, however, somehow I thought it was incorrect, because either Eclipse going out of sync or some copy and paste error (my bad), was loading a bunch of errors and the hashCode() was not being returned for the Card class.
Jon Camilleri
Ranch Hand

Joined: Apr 25, 2008
Posts: 659

Jon Camilleri wrote:
Stephan van Hulst wrote:Again, look at the example I provided. It does exactly this. Read it, test it, and tell us if there's something you don't understand.

Yes indeed, it's correct, however, somehow I thought it was incorrect, because either Eclipse going out of sync or some copy and paste error (my bad), was loading a bunch of errors and the hashCode() was not being returned for the Card class.


Here's the updated code...

 
It is sorta covered in the JavaRanch Style Guide.
 
subject: cannot read a Collection of a custom class as a parameter
 
Similar Threads
reading a private method within a class
Arraylist insights
Type mismatch: cannot convert from Image to Image[]
Overriding equals() method within a subclass
static insights?