HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Project Euler #54 in Java: Comparing poker hands of two players

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
pokercomparingjavahandsplayersprojecttwoeuler

Problem

I was bored the other day and decided to have a crack at Project Euler Problem 54 for some fun. Given a file containing one thousand poker hands dealt to two players, the task is to count the number of hands won by Player 1.

This is the first project Euler problem i have completed and thought it would be good to get some feedback as i feel to help myself improve on my coding style and optimise the solution.

Card Class

public class Card {

private String card;

private String suit;

private cardValue value;

public String getSuit() {
    return suit;
}

public cardValue getValue() {
    return value;
}

public Card(String card) {
    this.card = card;
    this.value = convertValue(this.card.substring(0, 1));
    this.suit = this.card.substring(1, 2);
}

public cardValue convertValue(String v){

    switch(v){
    case "2":
        return cardValue.Two;
    case "3":
        return cardValue.Three;
    case "4":
        return cardValue.Four;
    case "5":
        return cardValue.Five;
    case "6":
        return cardValue.Six;
    case "7":
        return cardValue.Seven;
    case "8":
        return cardValue.Eight;
    case "9":
        return cardValue.Nine;
    case "T":
        return cardValue.T;
    case "J":
        return cardValue.J;
    case "Q":
        return cardValue.Q;
    case "K":
        return cardValue.K;
    case "A":
        return cardValue.A;
    default:
        return cardValue.fail;
    }

}

public String getCard() {
    return card;
}

@Override
public String toString() {
    return this.card;
}

}


Hand class

```
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;

public class Hand {

private List broadwayList = Arrays.asList(cardValue.A,
cardValue.T, cardValue.J, cardValue.K, cardValue.Q);
private List wheelList = Arrays.asList(cardValue.A,
cardValue.Two,

Solution

Convention violations:

  • You're not indenting your class bodies by one level. That makes me twitch and is against every single coding standard I know of.



  • Also you're using a 2-space indent for each level. This is unusual (at the very least). The vast majority of java code is written with a 4-space indent.



  • cardValue should be CardValue. Types in java are usually in PascalCase and not camelCase. The same applies to handRankings :)



Beginner standard "mistakes":

You have the following code (or very similar) multiple times:

if (someCondition) {
    return true;
}
return false;


note that it's irrelevant whether there's an explicit else-block there.

In both cases you can instead write:

return someCondition;


Note that this is also true when you return false in your if-block, as long as you return !someCondition; then.

Your code seems to often keep a reference to what the classes are based on. e.g in Hand you have the String hand field, which is set in the constructor and (from a quick scan) never used again. You don't need to keep a reference to original data, so long as you can reconstruct the input from what you make of it. Even then it's usually a "waste" of memory.

You're very reliant on the "standard" control structures. This isn't a bad thing per se, it just shows you're not terribly familiar with altenative ways to control program flow. Consider researching Streams and Lambda-Expressions

Your methods (almost) always take a specific implementation as parameter. Instead of getHigherSet(ArrayList ...) you should use the interface to allow extensible handling of different ways to store hands: getHigherSet(List ...)
Responsibilities:

It might be easier to have CardValue and HandRanking be responsible for evaluating Cards and Hands respectively. Since Java enums are actually classes with some compiler-constraints they can have methods defined on them. Consider moving evaluateHand into HandRankings and convertValue into CardValue.

This will make Hand and Card a little easier to read.
Stream opportunities:

The first thing that I would rewrite into a Stream is checkFrequency. What you'll need for that is Collectors#groupingBy and Collectors#counting. The implementation I'll leave to you :)

Other opportunities include most of the loops you have there.
Closing remarks:

Overall your code is written cleanly and easy to follow, which is awesome. In some places I can see you're new to Java, maybe even new-ish to programming overall.

Well done!

Code Snippets

if (someCondition) {
    return true;
}
return false;
return someCondition;

Context

StackExchange Code Review Q#138476, answer score: 4

Revisions (0)

No revisions yet.