Project Euler #54 Solution

This is not the latest question in iterative code review of this program. You can find the latest answer here.

Here's my solution to Project Euler #54. Click the link for the full project description (abbreviated here):


The file, poker.txt, contains one-thousand random hands dealt to two players. Each line of the file contains ten cards (separated by a single space): the first five are Player 1's cards and the last five are Player 2's cards. You can assume that all hands are valid (no invalid characters or repeated cards), each player's hand is in no specific order, and in each hand there is a clear winner.


How many hands does Player 1 win?

9 classes but most of them are short enums.

Note: You will need to put the data file poker.txt on your classpath.

Card.java

package problem54;

public class Card implements Comparable {
    private final Value value;
    private final Suit suit;

    public Card(Value value, Suit suit) {
        this.value = value;
        this.suit = suit;
    }

    public Card(String s) {
        this(Value.of(s.charAt(0)), Suit.of(s.charAt(1)));
    }

    public Value getValue() {
        return value;
    }

    public Suit getSuit() {
        return suit;
    }

    @Override
    public int compareTo(Card o) {
        return value.compareTo(o.value);
    }

    @Override
    public String toString() {
        return String.format("%s_%s", value.getChar(), suit.getChar());
    }
}

Hand.java (the Heavy Lifting is here)

```
package problem54;

import java.util.ArrayList;
import java.util.Collections;
import java.util.LinkedList;
import java.util.List;

public class Hand implements Comparable {
private final List cards;
private final ValueRanking ranking;

public Hand(List cards) {
List temp = new ArrayList<>(cards);
temp.add(new Card(Value.NULL, Suit.NULL));
Collections.sort(temp);
this.cards = Collections.unmodifiableList(temp);

ValueRanking straightRanking = strai

For the most part, this is beautiful code.
The way you lay out the classes is logical,
the implementation is clean,
following good practices,
even the formatting is almost impeccable.
But if I look closer,
some odd elements do pop up,
and in the end it looks as if you rushed the second half of the implementation.

Parsing

The Parser class is very nicely written.
But it doesn't fully complete its job.
The fact that the first 5 cards per line belong to one player and the second 5 cards belong to another is dealt with here, and left for Round to handle.
It would be better if all the logic related to parsing the input into program objects was encapsulated in Parser (= hidden from the rest of the program).
The Round class could just be in charge of holding the Hands of players,
and oblivious to the fact that the hands come from a set of 10 cards,
and the implied magic number 5.

Storing cards in a LinkedList

I noticed with suspicion in the Parser that cards are added to a LinkedList.
A linked list is great when you frequently add or delete the first or last element.
But in modeling a poker game it's not really obvious when and how this would be useful.
On closer look,
I see that you're not taking advantage of the cards being in a LinkedList.
In fact there are lots of random accesses of elements.

And it gets worse.
The Round class creates the hands using .subList on the original LinkedList of 10 cards.
The consequence of that is when you do cards.get(2) later in the program on the hand that comes from cardList.subList(5, 10),
the underlying LinkedList will be traversed from the first element to the 7th.

There are two important points here:

• Use the List implementation appropriate for the purpose. In the program an ArrayList would be better than a LinkedList.

• Be careful when using .subList, and consider carefully how it really works, and its consequences

UPDATE:
As you pointed out,
the "damage" is not so bad as it seemed at first,
since the Hand constructor converts the linked lists to array lists quite early on.
Even so,
it would have been better to not use a better storage from the start.
Interestingly,
if Parser was fully in charge of parsing and creating the Hand objects,
then there wouldn't be a question of using sublists in the first place,
and the whole issue just vanish.

Duplicated evaluations

There are several .compareTo calls that are evaluated twice,
for example here:

if(rank.compareTo(o.rank) != 0) return rank.compareTo(o.rank);
else if(primary.compareTo(o.primary) != 0) return primary.compareTo(o.primary);
else if(secondary.compareTo(o.secondary) != 0) return secondary.compareTo(o.secondary);

Although the cost of this is probably tiny,
for the sake of clean coding,
and to avoid repeating yourself,
I suggest to rewrite in a way to eliminate duplication, for example:

int compare = rank.compareTo(o.rank);
if (compare != 0) return compare;

compare = primary.compareTo(o.primary);
if (compare != 0) return compare;

compare = secondary.compareTo(o.secondary);
if (compare != 0) return compare;

Coding style

For the most part,
the coding style is great.
But then, things get a bit out of hand at a few places.
I have some suggestions to apply consistently everywhere:

• Avoid extremely long lines, by adding more line breaks appropriately

• Avoid single-letter variable names

• Use braces even with single-statement if

• Put a space before ( in if conditions and for loops

Misc

Instead of this:

if(v == null) return NULL;
return v;

I suggest using the ternary operator:

return v == null ? NULL : v;

I don't think this is too cryptic (I'm strongly against cryptic code),
and it has the advantage of not writing return twice.

You put the input file in the resources root:

InputStream is = Problem.class.getResourceAsStream("/p054_poker.txt");

I'd recommend to put it in a /data/ directory,
rather than the root.
If you have just one file it may not matter much,
but having files lying around in the root dir just seem sloppy.

In the ValueRanking constructor,
I'm not sure what to make of this:

try {
    Collections.sort(kickerTemp);
} catch (NullPointerException e) {
    System.out.println(kicker);
    throw e;
}

It's not really clear what's going on here,
at least a comment explaining would be nice.

StackExchange Code Review Q#81772, answer score: 7