Return to Answer

• 1

• 113k
• 15
• 154
• 396

• 98.1k
• 17
• 219
• 419

Your code is neat, and structured, and the OO nature appears to be clear.

Some other issues I have with it are more boilerplate types.

##Comparable

Comparable has a contract:

The natural ordering for a class C is said to be consistent with equals if and only if e1.compareTo(e2) == 0 has the same boolean value as e1.equals(e2) for every e1 and e2 of class C

In practice, what this means is that in every case where you implement Comparable, you also need to override the equals and hashCode() methods. You implement Comarable in ValueRanking, Hand and Card. Each of these need to be changed in order for the consistency of the contract to be maintained.

Note that the contract allows some tricks, for example, equals becomes:

public boolen equals(Object other) {
 return other instanceof Hand && compareTo((Hand)other) == 0;
}

Hashcode is a bit more complicated though.

By having a correct implementation for these you can then rely on using them as keys in Maps, etc. It is good practice to implement things correctly.

In your case, as you know, this will not affect the results of your program, but it will impact the reusability of it.

##Enums

The null placeholder positions x in the Suit and Value enums are out of place, and should not exist. What is their purpose?

##Value Look-ups

Some alternative suggestions....

In your Value Enum you have a Map to provide look-ups between characters and Values. The code is:

private static final Map<Character, Value> valueMap = new HashMap<>();
static {
 for(Value c : Value.values()) {
 valueMap.put(c.getValueChar(), c);
 }
}
public static Value of(char value) {
 Value v = valueMap.get(value);
 return v == null ? NULL : v;
}

I would consider a simpler, and probably faster way of doing it:

private static final Value[] valueMap = new Value[128];
static {
 for(Value c : Value.values()) {
 valueMap[c.getValueChar()] = c;
 }
}
public static Value of(char value) {
 Value ret = value < valueMap.length ? valueMap[value] : null;
 if (ret == null) {
 throw new IllegalArguementException("Illegal value char '" + value + "'.");
 }
 return ret;
}

The above code does not have the overhead of the char -> Character translation and all the map overheads.

On further thought, a switch statement, though long-winded, would probably be the best (performance wise):

switch (value) {
 case 'A' : return ACE;
 case 'K' : return KING;
 ....
}

I would normally add a newline after the : in the case statement, but in a situation like that where all cases are identical, I may get lazy for the sake of consistency...

The same thing can be done for Suit

##makeCardList

This function is used as follows:

String[] cardStrings = nextLine.split(" ");
List<Card> first = makeCardList(cardStrings, 0, 5);
List<Card> second = makeCardList(cardStrings, 5, 10);

and

public static List<Card> makeCardList(String[] cardStrings, int start, int stop) {
 List<Card> cardList = new ArrayList<>(stop - start);
 for(int i = start; i < stop; i++) {
 cardList.add(new Card(cardStrings[i]));
 }
 return cardList;
}

This code can be simplified with the Java List.subList() function, and some neat Java 8 functions:

List<Card> cardList = Stream.of(nextLine.split(" "))
 .map(c -> new Card(c))
 .collect(Collectors.toList());
List<Card> first = cardList.subList(0, 5);
List<Card> second = cardList.subList(5, 10);