Code review for a class implementing the Comparator interface

[Faz Ali]

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?

import java.util.Comparator;
import java.util.Set;
import java.util.LinkedHashSet;
import java.util.Iterator;

public class CardSorter implements Comparator<Card>
{
	public static final int INCREASING_ORDER = 0;
	public static final int DECREASING_ORDER = -1;
	private static final LinkedHashSet<Card.PlayingCardSuit> defaultCardSuitOrder = new LinkedHashSet<Card.PlayingCardSuit>(); //For default suit order
	
	private LinkedHashSet<Card.PlayingCardSuit> cardSuitOrder; //For explicitely supplied suit order
	private int cardValueOrder; //positive = increasing order, negative = decreasing order
	private boolean groupValues; //flag to indicate if values should be grouped by value or by suit
	
	public CardSorter(int cardValueOrder, boolean groupValues) //alphabetical suit order implied
	{
		this(defaultCardSuitOrder, cardValueOrder, groupValues);
	}
		
	public CardSorter(LinkedHashSet<Card.PlayingCardSuit> cardSuitOrder, int cardValueOrder, boolean groupValues) throws IllegalArgumentException
	{
		if(cardSuitOrder.size() != 4)
		{
			throw new IllegalArgumentException("All four PlayingCardSuit values have not been supplied");			
		}
		this.cardSuitOrder = cardSuitOrder;
		this.cardValueOrder = cardValueOrder;
		this.groupValues = groupValues;
	}
	
	public void setSuitOrder(LinkedHashSet<Card.PlayingCardSuit> cardSuitOrder) throws IllegalArgumentException //Setter for suit order
	{
		if(cardSuitOrder.size() != 4)
		{
			throw new IllegalArgumentException("All four PlayingCardSuit values have not been supplied");			
		}
		this.cardSuitOrder = cardSuitOrder;
	}
	
	public void setValueOrder(int cardValueOrder) //setter for indicating ascending or descending order
	{
		this.cardValueOrder = cardValueOrder;
	}
	
	public void setGroupValuesFlag(boolean groupValues) //setter for indicating group by value or group by suit
	{
		this.groupValues = groupValues;
	}
	
	public int compare(Card c1, Card c2) //Interface method implemented
	{
		if(c1.equals(c2))
		{
			return 0;
		}
		int x = 0; //Card c1 suit index
		int y = 0; //Card c2 suit index
		Object[] order = cardSuitOrder.toArray(); //To get the index positions of the Suit order
		for(int z=0; z<order.length; z++)
		{
			if(c1.getSuitType() == (Card.PlayingCardSuit)order[z])
			{
				x = z;
			}
			if(c2.getSuitType() == (Card.PlayingCardSuit)order[z])
			{
				y = z;
			}
		}
		if(groupValues) //group the values
		{
			if(cardValueOrder >= 0) //in ascending order
			{
				if(c1.getValue() == c2.getValue())
				{
					return x < y ? -1 : 1;
				}
				if(c1.getValue() < c2.getValue())
				{
					return -1;
				}
				else
				{
					return 1;
				}
			}
			else //in descending order
			{
				if(c1.getValue() == c2.getValue())
				{
					return x < y ? -1 : 1;
				}
				if(c1.getValue() < c2.getValue())
				{
					return 1;
				}
				else
				{
					return -1;
				}
			}
		}
		else //do not group by value
		{
			if(cardValueOrder >= 0) //in ascending order
			{
				if(c1.getSuit() == c2.getSuit())
				{
					return c1.getValue() < c2.getValue() ? -1 : 1;
				}
				if(x < y)
				{
					return -1;
				}
				else
				{
					return 1;
				}
			}
			else //in descending order
			{
				if(c1.getSuit() == c2.getSuit())
				{
					return c1.getValue() < c2.getValue() ? 1 : -1;
				}
				if(x < y)
				{
					return -1;
				}
				else
				{
					return 1;
				}
			}		
		}
	}
	
	static //Default suit order
	{
		defaultCardSuitOrder.add(Card.PlayingCardSuit.CLUBS);
		defaultCardSuitOrder.add(Card.PlayingCardSuit.DIAMONDS);
		defaultCardSuitOrder.add(Card.PlayingCardSuit.HEARTS);
		defaultCardSuitOrder.add(Card.PlayingCardSuit.SPADES);
	}
}

[Wouter Oet]

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]

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]

I don't think it is that much overhead:

class CardSorter implements Comparator<Card> {

	private final Comparator<Card> cardComparator;


	CardSorter(boolean asc) {
		this.cardComparator = new ValueCardComparator();
	}

	CardSorter(boolean asc, Set<Card.PlayingCardSuit> suits) {
		this.cardComparator = new SuitCardComparator(asc, suits);
	}

	@Override
	public int compare(Card o1, Card o2) {
		return cardComparator.compare(o1, o2);
	}


}

class ValueCardComparator implements Comparator<Card> {
	@Override
	public int compare(Card o1, Card o2) {
		return 0;
	}
}

class SuitCardComparator implements Comparator<Card> {
	public SuitCardComparator(boolean asc, Set<Card.PlayingCardSuit> suits) {
	}

	@Override
	public int compare(Card o1, Card o2) {
		return 0;
	}
}