Win a copy of Head First Agile this week in the Agile forum!
  • Post Reply Bookmark Topic Watch Topic
  • New Topic

Code review for a class implementing the Comparator interface  RSS feed

 
Faz Ali
Greenhorn
Posts: 21
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hi all,

I am faily new to the forum and a general lurker but since I am preparing to take the OCPJP exam, I plan on being more active here.
I am currently studying about 'Collections and Generics' and applying this knowledge on a card-based game I wrote for my Computer Science dissertation. One of the features I wanted for my Card game was the ability to sort hands based on a users perference. The Comparator interface is perfect for this and the class I've written is below. The sort combinations I've implemented are:

Grouped by Value && In Ascending order - E.g. {Ace of Clubs, Ace of Diamonds, Ace of Hearts, Ace of Spades, 2 of Clubs, ...}
Grouped by Value && In Descending order - E.g. {King of Clubs, King of Diamonds, King of Hearts, King of Spades, Queen of Clubs, ...}
Grouped by Suit && In Ascending order - E.g. {Ace of Clubs, 2 of Clubs, 3 of Clubs, 4 of Clubs, ...}
Grouped by Suit && In Descending order - E.g. {King of Clubs, Queen of Clubs, Jack of Clubs, 10 of Clubs, ...}

ALL THE ABOVE SORTS ALLOW FOR SUIT ORDER TO BE DEFINED ALSO

The code works perfectly but my goal is to get into a Java development position so I would appreciate any feedback on my code below in terms of overall design if you would be so kind. It implements Comparator, uses LinkedHashSet, Iterator, a static initialisation block, overloaded constructors, Exceptions etc. I just want to know if there is anything that I can do better, anything that I am doing plain wrong, design pattern etc, just for this one class.

One question I do have is checking for nulls in constructors and methods. It just seems like every method you write, you may have to do that which seems like overkill really. Surely there is some more elegant design for checking for nulls?

 
Wouter Oet
Bartender
Posts: 2700
IntelliJ IDE Opera
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Couple of remarks:

Try to "Program against an interface": Set<Card> set = new HashSet<Card>();
Use an enum/boolean for declaring ascending/descending instead of an int.
Some code of the constructor is the same as the setter for cardSuitOrder. Try to eliminate that.

Rename some variables e.g. in compare() you have
int x = 0; //Card c1 suit index
Why not use
int firstCardSuitIndex = 0;

In compare() I would use LinkedHashSet.toArray(T[]) instead of LinkedHashSet.toArray() which makes the casting redundant.
I would create 2 more Comparators, one for Value (with asc/desc) and one for Suit (also with asc/desc) and then delegate the compare method to them.
The compare method contains a lot of duplicate code and the method is long. Try to split it up into methods which can be reused.
 
Faz Ali
Greenhorn
Posts: 21
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Wouter Oet wrote:Couple of remarks:

Try to "Program against an interface": Set<Card> set = new HashSet<Card>();
Use an enum/boolean for declaring ascending/descending instead of an int.
Some code of the constructor is the same as the setter for cardSuitOrder. Try to eliminate that.

Rename some variables e.g. in compare() you have
int x = 0; //Card c1 suit index
Why not use
int firstCardSuitIndex = 0;

In compare() I would use LinkedHashSet.toArray(T[]) instead of LinkedHashSet.toArray() which makes the casting redundant.
I would create 2 more Comparators, one for Value (with asc/desc) and one for Suit (also with asc/desc) and then delegate the compare method to them.
The compare method contains a lot of duplicate code and the method is long. Try to split it up into methods which can be reused.


Great feedback Wouter, thanks.
I'm implementing all your changes apart from having multiple Comparators. - Having the single one seems logical to me since the code is not too complex and accommodates any values provided via the setters. If I split it up into multiple Comparators based on the diferent sort orders, I would end up with either duplicate code (constructors) or more classes/interfaces to prevent the duplicates code which I think would be overkill for this situation.
 
Wouter Oet
Bartender
Posts: 2700
IntelliJ IDE Opera
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I don't think it is that much overhead:

  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!