patternjavaMinor
Project Euler #54 in Java: Comparing poker hands of two players
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
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,
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:
Beginner standard "mistakes":
You have the following code (or very similar) multiple times:
note that it's irrelevant whether there's an explicit else-block there.
In both cases you can instead write:
Note that this is also true when you
Your code seems to often keep a reference to what the classes are based on. e.g in Hand you have the
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
Responsibilities:
It might be easier to have
This will make
Stream opportunities:
The first thing that I would rewrite into a Stream is
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!
- 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.
cardValueshould beCardValue. Types in java are usually inPascalCaseand notcamelCase. The same applies tohandRankings:)
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.