3
\$\begingroup\$

As part of a recent interview, I was assigned to write a small BlackJack program. After submitting the solution, I received an answer that "the solution was functionally OK, but was way below their standards of quality code, especially coming from a person with significant experience" (I have 15 years experience).

I have posted my code in order to receive an honest critical review on the quality of the code. It is true that I wrote it while on vacation, but does it really suck so much? If so, how could I have written it differently?

public class Deck {


    public enum CARD {
            S2(2),S3(3),S4(4),S5(5),S6(6),S7(7),S8(8),S9(9),S10(10),SJ(10),SQ(10),SK(10),SA(11),
            D2(2),D3(3),D4(4),D5(5),D6(6),D7(7),D8(8),D9(9),D10(10),DJ(10),DQ(10),DK(10),DA(11),
            C2(2),C3(3),C4(4),C5(5),C6(6),C7(7),C8(8),C9(9),C10(10),CJ(10),CQ(10),CK(10),CA(11),
            H2(2),H3(3),H4(4),H5(5),H6(6),H7(7),H8(8),H9(9),H10(10),HJ(10),HQ(10),HK(10),HA(11);

            private int points;

            private CARD(int points) {
                this.points = points;
            }

            public int getPoints() {
                return points;
            }

    }



    private Stack<CARD> deck = new Stack<Deck.CARD>();



    private Deck(Stack<CARD> deck) {
        this.deck = deck;
    }

    private Deck(){
        shuffleDeck();
    }

    private void shuffleDeck() {
        // we need it mutable and the list provided by Arrays is Immutable
        List<CARD> allCards = new ArrayList(Arrays.asList(CARD.values()));
        int max = 52;
        Random rand = new Random();
        while (allCards.size()>0) {
            CARD removedCard = allCards.remove(rand.nextInt(max)) ;
            this.deck.add(removedCard);
            --max;
        }
    }


    public CARD draw() {
        return deck.pop();
    }



    public static class Builder {

        Stack<CARD> deck=null;

        public static Builder createNew(){
            return new Deck.Builder();
        }

        public  Builder fromFile(File file) throws IOException {
            Scanner scanner=null;

            try {
                scanner = new Scanner(file);
                deck = new Stack<>();
                for (String card : scanner.nextLine().split(", ")) {
                    deck.add(Enum.valueOf(CARD.class, card));
                }


            } finally {
                if (scanner!=null) {
                    scanner.close();
                }
            }
            return this;
        }

        public Builder fromStack(Stack stack) {
            this.deck = stack;
            return this;
        }

        public  Builder validate() {
            if (deck==null) {
                return this;
            }
            if (deck.size()>52) {
                throw new RuntimeException("Critical error, deck is not initialized correctly.You need 52 unique cards.");

            }
            HashSet<CARD> allUniqueCards = new HashSet<>(deck==null?new HashSet<>():deck);
            // verify uniqueness of cards in case of 52 card condition is met but some are duplicates
            if (deck==null || allUniqueCards.size()==52) {
                return this;
            }
            // this is error that is not recoverable therefore we define it as RuntimeException
            throw new RuntimeException("Critical error, deck is not initialized correctly.You need 52 unique cards.");

        }

        public  Deck build() {

            return deck==null? new Deck():new Deck(deck);
        }


    } 

    public String toString() {
        String toString = deck.stream().map(t->{return t.toString()+", ";}).reduce("", (t,q)->{return t+q;});
        if (!toString.isEmpty()) {
            toString = toString.substring(0,toString.length()-2);
        }

        return toString; 
    }

}

Hand class

public class Hand {

    // in general it is one idea more correct to be Set, 
    //but I am on holiday right now and I need to write the tests fast.
    private List<CARD> hand = new ArrayList<>();


    public boolean hasBlackJack() {

        return score()==21;
    }

    public boolean has22(){
        return score()==22;
    }

    public boolean lessThan17(){
        return score()<17;
    }

    public boolean greaterThan(Hand hand) {
        return this.score()>hand.score();
    }

    public boolean greaterThan21(){
        return score()>21;
    }

    public void addCard(CARD card) {
        hand.add(card);
    }

    private int score(){
        return hand.stream().mapToInt(t->{return t.getPoints();}).sum();

    }

    public boolean empty() {
        return hand.size()==0;
    }

    public String toString() {
        // the sequential stream is not nessesary realy, but it makes it one idea easier to test later.
        String toString = hand.stream().sequential().map(t->{return t.name();}).reduce("", (t,q)->{return t.isEmpty()?q: t+", " + q;});
        return toString;
    }


}

Player class

public class Player {

    private String playerName =null;

    private Hand hand = new Hand();



    public Player(String playerName) {
        this.playerName=playerName;

    }

    public boolean hasBlackJack(){
        return hand.hasBlackJack();
    }

    public boolean has22() {
        return hand.has22();
    }

    public boolean lessThan17(){
        return hand.lessThan17();
    }

    public boolean greaterThan21() {
        return hand.greaterThan21();
    }

    public boolean hasEmptyHand() {
        return hand.empty();
    }

    public boolean handGreaterThan(Player player) {
        return this.hand.greaterThan(player.hand);
    }

    public void draw(Deck deck) {
        hand.addCard(deck.draw());
    }


    public String getPlayerName(){
        return playerName;
    }

    public String toString() {
        return playerName + ":" + " " + hand.toString(); 
    }

}

    //Assumption in case of a tie SAM wins.
public class Play21 {

    Player dealer = null;
    Player sam = null;
    Deck deck =  null;


    public Play21(Player sam,Player dealer,Deck deck) {
        this.sam=sam;
        this.dealer=dealer;
        this.deck = deck;
    }

    public static void printWinner(Player winner,Player looser) {
        System.out.println(winner.getPlayerName());
        System.out.println(winner.getPlayerName().equals("sam")?winner:looser);
        System.out.println(winner.getPlayerName().equals("sam")?looser:winner);
    }




    private Player round1() {

        sam.draw(deck);
        dealer.draw(deck);
        sam.draw(deck);
        dealer.draw(deck);

        if (dealer.hasBlackJack()&&sam.hasBlackJack()) {
            return sam;
        }
        else if (sam.hasBlackJack()) {
            return sam;
        }
        else if(dealer.hasBlackJack()) {
            return dealer;
        }

        if (dealer.has22()&&sam.has22()) {
            return dealer;
        }
        else if (sam.has22()) {
            return dealer;
        }
        else if (dealer.has22()) {
            return sam;
        }

        return null;
    }



    private Player round2() {
        if (sam.hasEmptyHand()||dealer.hasEmptyHand()) {
            throw new IllegalStateException("You need to invoke round1 first");
        }

        while(sam.lessThan17()) {
            sam.draw(deck);
        }

        if (sam.greaterThan21()) {
            return dealer;
        }


        while (sam.handGreaterThan(dealer)) {
            dealer.draw(deck);
        }

        if (dealer.greaterThan21()) {

            return sam;
        }
        if (dealer.handGreaterThan(sam)) {
            return dealer;
        }
        else {
            return sam;
        }
    }

    public Player play(Deck deck){


        Player winner = round1();
        if (winner!=null) {
            return winner;
        }
        return round2();


    }

    public static Deck createDeck(String args[]) throws IOException {
        Deck deck =null;

        if (args.length==0) {
            deck=Deck.Builder.createNew()
                             .validate()
                             .build();
        }
        else if (args.length==1) {
            File file =  new File(args[0]);
            if (file.isFile()) {
                deck=Deck.Builder.createNew()
                                 .fromFile(file)
                                 .validate()
                                 .build();
            }
            else {
                // Unrecoverable exception therefore declared as RuntimeException
                throw new RuntimeException("Path is not a file, or file does not exist,or file is unreadable(check permissions).");
            }

        }
        else {
            throw new RuntimeException("Illegal number of arguments. ");
        }

        return deck;
    }


    public static void main(String[] args) throws IOException{
        Deck deck = createDeck(args);
        Player dealer = new Player("dealer");
        Player sam = new Player("sam");

        Play21 play = new Play21(sam,dealer,deck);
        Player winner = play.play(deck);
        printWinner(winner, winner==sam?dealer:sam);






    }

}
\$\endgroup\$
  • 3
    \$\begingroup\$ I've downvoted this only because you don't post the specific requirements for your Black Jack implementation - I see a lot of non-standard things here, that don't make sense in a normal Black Jack game. You should post what the requirements for your implementation are to help us better understand what's happening, and why. \$\endgroup\$ – 410_Gone Aug 9 '17 at 17:24
  • \$\begingroup\$ @EBrown I am afraid to type the exact text. I will just note that from what I have read online the only difference in Rules is that the ACE is aways 11 points and that if both sam and dealer have same points SAM wins. \$\endgroup\$ – Alexandar Petrov Aug 9 '17 at 17:59
  • \$\begingroup\$ Lot spaces. Why are you afraid type in the exact text? You assume allCards.size = 52 rather than just use allCards.size. There are more efficient shuffles. codereview.stackexchange.com/questions/169547/… \$\endgroup\$ – paparazzo Aug 9 '17 at 18:22
  • 1
    \$\begingroup\$ An interview question is the property of company? That is crazy. \$\endgroup\$ – paparazzo Aug 9 '17 at 18:26
  • 2
    \$\begingroup\$ I've rolled back Rev 7 → 6. On Code Review, we try to keep questions pure, without injecting your own conclusions into the question. \$\endgroup\$ – 200_success Aug 9 '17 at 19:48
16
\$\begingroup\$

Avoid repetitive code

    public enum CARD {

            S2(2),S3(3),S4(4),S5(5),S6(6),S7(7),S8(8),S9(9),S10(10),SJ(10),SQ(10),SK(10),SA(11),
            D2(2),D3(3),D4(4),D5(5),D6(6),D7(7),D8(8),D9(9),D10(10),DJ(10),DQ(10),DK(10),DA(11),
            C2(2),C3(3),C4(4),C5(5),C6(6),C7(7),C8(8),C9(9),C10(10),CJ(10),CQ(10),CK(10),CA(11),
            H2(2),H3(3),H4(4),H5(5),H6(6),H7(7),H8(8),H9(9),H10(10),HJ(10),HQ(10),HK(10),HA(11);

Consider breaking this up into two enum definitions.

public enum Rank {

    TWO(2),
    THREE(3),
    FOUR(4),
    FIVE(5),
    SIX(6),
    SEVEN(7),
    EIGHT(8),
    NINE(9), 
    TEN(10),
    JACK(10),
    QUEEN(10),
    KING(10), 
    ACE(11);

and

public enum Suit {

    SPADES,
    HEARTS,
    DIAMONDS,
    CLUBS

}

Now you don't repeat the points definition four times.

I'm not sure that I'd make these internal to Deck. That makes sense for something like Map.Entry, which has no meaning outside Map. But a card matters in multiple situations. But of course, this something of a judgment call.

I'm more accustomed to putting one enum value per line in Java. This is especially true when passing a value to the constructor.

Avoid deprecated types

    private Stack<CARD> deck = new Stack<Deck.CARD>();

Stack has been deprecated in favor of Deque.

You say CARD in one place and Deck.CARD in another.

    private Deque<Card> deck = new ArrayDeque<>();

No deprecated type needed.

Now there is no need for a second type declaration. The <> will do the right thing in newer Java types.

No magic numbers

        int max = 52;

Worse, you don't need the magic number. Consider

        int max = CARD.values().length;

But I broke CARD up into Rank and Suit. How about

        int max = allCards.size();

Now there is an exact match. But we don't need to do that.

            CARD removedCard = allCards.remove(rand.nextInt(max)) ;

This could be

            CARD removedCard = allCards.remove(rand.nextInt(allCards.size()));

Now we don't need max at all. We can just reuse the value that already tracks what we want: the number of cards remaining in the deck.

Don't create multiple Random objects

What happens if you shuffle two decks within the same seed value? You'd end up with identical decks. The fix is to make one Random object in static context (i.e. at the beginning of the program).

    public static final Random random = new Random();

Now there is never more than one Random object, and it seeds just once.

Use the built-in methods

You rolled your own shuffle method. Why? There's a standard method for that: Collections.shuffle.

Choose an access level

        Stack<CARD> deck=null;

This uses the default package private access level. Why? Is there something special that causes other classes in the package to need to be able to access it? If so, I would comment that.

        private Deque<CARD> deck;

Otherwise, you can make it private.

In Java, you don't need to initialize objects to null. It does that for you by default.

try-with-resources

            Scanner scanner=null;

            try {
                scanner = new Scanner(file);

If we write

            try (Scanner scanner = new Scanner(file)) {

Then we don't need the finally block. The Scanner will be closed automatically, regardless.

Redundant logic

You do

            if (deck==null) {
                return this;
            }

Then later, you have

            HashSet<CARD> allUniqueCards = new HashSet<>(deck==null?new HashSet<>():deck);
            // verify uniqueness of cards in case of 52 card condition is met but some are duplicates
            if (deck==null || allUniqueCards.size()==52) {

But you know that deck could never be null here. Why not just say

            Set<CARD> allUniqueCards = new HashSet<>(deck);
            // verify uniqueness of cards in case of 52 card condition is met but some are duplicates
            if (allUniqueCards.size() == VALID_DECK_SIZE) {

Now we don't check for things that could never happen.

I also fixed the magic number problem here.

I expect types to be an interface rather than an implementation. So the original code looks wrong to me.

Feedback

does it really suck so much?

I'm not sure that that's what

the solution was functionally OK, but was way below their standards of quality code

means. When I read interview code, I expect it to be exemplary. It should exhibit good coding standards. And it should fit my organization's coding standards. If we're getting a hundred applicants and reviewing code from ten of them, then I expect to be taking the top 30% to the next step. It's not enough for code to not suck. It should be good if not great.

I don't mean to be rude in what I say next, but I don't want to soften the harsh edges either. I want to give the same kind of review that I would give internally in commenting on your code if I was contributing to a hiring decision.

I find things like magic numbers, repetitive code, and multiple calls to Random as unacceptable. I don't want to have to edit this code later. This is worrisome to me in someone with fifteen years experience, as you are likely somewhat set in your ways. I would be more willing to overlook these kinds of things in an intern or new developer who might be more malleable.

Some of the decisions you make might make more sense in another language, e.g. C (picking on C because I know it well enough to say; may also fit in other languages that I don't know as well). But they don't make sense in Java. In particular, I'm talking about the way that you write your own shuffle method, max, and explicitly set objects to null. In C, all those would look like reasonable decisions. In Java, they don't fit.

Some of the decisions fit better in older versions of Java. The finally block with the Scanner would have been state of the art in earlier versions of the language. Now, the try-with-resources form is standard. If this is a Java post, I would expect applicants to know this.

And of course, it is always possible that my judgment is wrong. Perhaps you would be a better developer than the other person in actuality. But we'd never know, since I would probably keep looking past this submission. I'd prefer to hire someone who consistently produces code that I find easy to read and edit later.

Perhaps some of my comments are overly pedantic. But in my experience, I can hold out for someone who meets my level of pedantry. So why wouldn't I?

\$\endgroup\$
10
\$\begingroup\$

main method

In the main method, I don't think you should pass args to the createDeck function. The main's job is pretty much to check input and be the workflow of your game. After all, if I was to use your createDeck function, how would I know what parameters I need to pass to this method to make it work?

In fact, I'm not sure the createDeck method itself is useful. If you were to change your Builder method to receive a fileName instead of a File, you could move all that logic at the same place, where it belongs: The Builder. Also, RuntimeException is pretty vague, I feel you should throw the most specific exception possible, in this case, IllegalArgumentException.

public static void main(String[] args) throws IOException{

    Deck.Builder deckBuilder = Deck.Builder.createNew();

    if(args.length == 0) {
        deckBuilder = deckBuilder.createNew();
    } else if (args.length == 1) {
        deckBuilder = deckBuilder.fromFilePath(args[1]);
    } else {
        throw new IllegalArgumentException("Expected at most one argument");
    }

    Deck deck = deckBuilder.validate().build();
    Player dealer = new Player("dealer");
    Player sam = new Player("sam");

    Play21 play = new Play21(sam,dealer,deck);
    Player winner = play.play(deck);
    printWinner(winner, winner==sam?dealer:sam);
}

I don't feel like there's a need to show you the new fromFilePath method, it's pretty easy to see what goes in there.

Hand class

The Hand doesn't need to know about the specifics of the BlackJack game. That means you shouldn't have the 4 methods hasBlackJack, has22, lessThan17 and greaterThan21. Just expose the getScore method and let the game handle the score things. In real life, a hand is just a subset of cards, reflect that in your code. greaterThan turned out to be useless, so I removed it.

public class Hand {

    private List<CARD> hand = new ArrayList<>();

    public void addCard(CARD card) {
        hand.add(card);
    }

    public int getScore(){
        return hand.stream().mapToInt(t->{return t.getPoints();}).sum();
    }

    //Verb-like method, so empty -> isEmpty
    public boolean isEmpty() {
        return hand.size() == 0;
    }

    public String toString() {
        // the sequential stream is not nessesary realy, but it makes it one idea easier to test later.
        String toString = hand.stream().sequential().map(t->{return t.name();}).reduce("", (t,q)->{return t.isEmpty()?q: t+", " + q;});
        return toString;
    }
}

Now the Hand is pretty lean.

Player class

We'll apply the same pattern for Player as we did for Hand. All a player is is a name and a hand.

public class Player {

    private String playerName = null;
    private Hand hand = new Hand();

    //Check for null
    public Player(String playerName) {
        this.playerName = playerName;
    }

    //Check for null, change the name to make it a little more explicit
    public void drawACard(Deck deck) {
        hand.addCard(deck.draw());
    }

    public int getHandScore() {
        return hand.getScore();
    }

    public String getPlayerName(){
        return playerName;
    }

    public String toString() {
        return playerName + ":" + " " + hand.toString(); 
    }
}

Play21 class

Play21 is a weird class name. We're playing BlackJack here right? So why not name it BlackJackGame? I think it's much more clear.

In your constructor you don't check for null input. You always need to check for null input in public methods/ctor. This'll make debugging much easier. I know it was just an interview question, but I think these little details make the difference between an excellent and an okay answer.

"Printing stuff" should never be in your Domain classes. The BlackJackGame has the responsibility to give access to the winning player, but shouldn't have to print it. Imagine you were to change your UI to a web-based game, what purpose would have printWinner? That brings another problem up. The BlackJackGame shouldn't be the class that has the main function. Create a Program class, or Application or whatever that has the responsibility to be a console app. Now, if you split this you'd have a clear Domain class BlackJackGame which handles the game, and a Program class that has the responsibility to print/ask input. The printWinner method should be in the Program class, not the BlackJameGame one.

round1 and round2 aren't good method names. What do they do and what do they return? A method should pretty much always be verb-like. So playFirstRound and playSecondRound would be much better. Next, I don't think these methods should return the winner. They're job is to change the scores of the player, that's all.

if (dealer.hasBlackJack()&&sam.hasBlackJack()) {
    return sam;
}
else if (sam.hasBlackJack()) {
    return sam;
}
else if(dealer.hasBlackJack()) {
    return dealer;
}

Something's off right? You don't need to check if both have blackjack is sam is to win. Just check if sam has a blackjack, if he does, he wins. Then, check if the dealer has one.

I'll show you how I would have written BlackJackGame class and explain the differences afterwards.

public class BlackJackGame {

    Player dealer = null;
    Player sam = null;
    Deck deck =  null;
    Player winner = null;
    public static final int BlackJackScore = 21;

    //Check for null
    public BlackJackGame(Player sam,Player dealer,Deck deck) {
        this.sam = sam;
        this.dealer = dealer;
        this.deck = deck;
    }

    private void playFirstRound() {

        sam.drawACard(deck);
        dealer.drawACard(deck);
        sam.drawACard(deck);
        dealer.drawACard(deck);

        int samScore = sam.getHandScore();
        int dealersScore = dealer.getHandScore();

        if(samScore == BlackJackScore) {
            winner = sam;
        } else if (dealersScore == BlackJackScore) {
            winner = dealer;
        }

        //Maybe there's something I don't get in the game
        //what is the difference between 22 and > 21?
        if(samScore > BlackJackScore) {
            winner = dealer;
        } else if (dealerScore > BlackJackScore) {
            winner = sam;
        }
    }

    private void playSecondRound() {
        if (sam.hasEmptyHand() || dealer.hasEmptyHand()) {
            throw new IllegalStateException("You need to invoke round1 first");
        }

        if(getWinner() != null) {
            throw new IllegalStateException("The game is already won");
        }

        //I'm no BlackJack pro, but I think you should have a constant for 17, 
        //explaining what's its significancy
        while(sam.getHandScore() < 17) {
            sam.drawACard(deck);
        }

        int samScore = sam.getHandScore();

        if (samScore > BlackJackScore) {
            winner = dealer;
            return;
        }

        while (samScore > dealer.getHandScore()) {
            dealer.drawACard(deck);
        }

        int dealerScore = dealer.getHandScore();

        if (dealerScore > BlackJackScore) {
            winner = sam;
        } else if (dealerScore > samScore) {
            winner = dealer;
        } else {
            winner = sam;   
        }
    }

    //You had a Deck parameter, but you didn't use it.
    public play(){
        playFirstRound();

        if(getWinner() != null) { return; }

        playSecondRound();
    }

    public Player getWinner() {
        return winner;
    }
}

The main difference is that it's now BlackJackGame's responsibility to set a winner, regarding the rules of the game.

To resume, I think the main problem with your code is that it violates the Single Responsibility Principle, your classes do things they shouldn't be doing. Focusing on the Domain will give you leaner classes.

\$\endgroup\$
  • 3
    \$\begingroup\$ @AlexanderPetrov Perhaps you should take a step back and read these answers objectively, instead of defending your (bad) programming practices. You came to Code Review wondering why the interviewer thought your code wasn't up-to-snuff, now is not the time to get defensive; use these as learning exercises. \$\endgroup\$ – 410_Gone Aug 9 '17 at 17:51
  • \$\begingroup\$ Comments are not for extended discussion; this conversation has been moved to chat. \$\endgroup\$ – 200_success Aug 9 '17 at 18:10
6
\$\begingroup\$
  • The first thing I'd do differently is the representation of a card. Your version contains code duplication, because, while it's true that there are 52 distinct cards, they can be described by two properties that have less than 52 possible values, namely rank and suit. So instead of making one enum for the cards, I would make an enum for the ranks and one enum for the suits, and define a card as a class with these two properties as fields. This would also make it possible to distinguish cards by suit, which can be useful since there are blackjack variants where a suited blackjack pays more than a non-suited blackjack. And if you want to avoid unnecessary object creation, you can make a static factory method to return already existing card objects.
  • My next two observations pertain to the following line:

    List<CARD> allCards = new ArrayList(Arrays.asList(CARD.values()));
    
    • First, you are using a raw type constructor. This is not illegal, but by using a raw type constructor instead of a diamond constructor (or an explicit constructor like new ArrayList<CARD>), you are denying the type safety at compile-time provided by generics, increasing the potential for bugs. For example, the following code would compile:

      List<CARD> allCards = new ArrayList(Arrays.asList(new String[]{"a", "b", "c"}));
      

      Of course, you wouldn't do something as obviously wrong as this, but if you inadvertently construct a raw List with elements of a wrong type, the compiler won't complain when you assign it to a List<CARD>.

    • Also, the statement does more than it needs to do because it creates two lists, only one of which is used. It might be convenient to write the code like that, but program-logic-wise, it is more complicated than what you actually want to do, which is simply to add all the enum values to a new ArrayList. The most direct way to do that would obviously be creating an empty ArrayList, iterating over the array and adding each card to the list. There is a convenience method in Collections that does the last two steps:

      List<CARD> allCards = new ArrayList<>();
      Collections.addAll(allCards, CARD.values());
      

      Since Java 8, you can also use streams:

      List<CARD> allCards = Arrays.stream(CARD.values()).collect(Collectors.toCollection(ArrayList::new));
      

      Note that, even though the constructor reference ArrayList::new doesn't specify a type parameter, the created ArrayList will still be generic and not raw (see also here).

  • The local variable max in the method shuffleDeck() is redundant and makes the code confusing to read, because it only mirrors the size of allCards. It would be less ambiguous if you simply did this:

    Random rand = new Random();
    while (allCards.size()>0) {
        CARD removedCard = allCards.remove(rand.nextInt(allCards.size())) ;
        this.deck.add(removedCard);
    }
    
  • What's the point of the method createNew() in Deck.Builder? Why not simply declare a public no-argument constructor with an empty body? Actually, fromFile(File) and fromStack(Stack) should be constructors too, because as instance methods, they can only be invoked on an instance of Builder, and the only available constructor is the (implicit) no-argument constructor, so the design of declaring instance methods that actually serve the purpose of returning a new Builder is really quite convoluted.

  • Speaking of Builder.fromFile(Stack), here's a raw type again. What you need is probably fromFile(Stack<CARD>), unless you actually want the method to accept any kind of Stack and assign that Stack to the field deck, which is declared as Stack<CARD> (the assignment itself would succeed), so that a ClassCastException is thrown at runtime when accessing the elements of deck, because someone passed a Stack<BigInteger> as a joke and a BigInteger cannot be cast to CARD.
  • About Builder.validate(): You already check deck for being null in the beginning, so you don't need to consider this case later on in the method. Also, in the second step, you might as well check if deck.size() != 52 instead of deck.size() > 52, because if deck.Size() < 52, the same exception will be thrown later anyway, the only difference being that you create a new HashSet in between.
  • What is actually the purpose of the Deck.Builder class? It has three methods that are really constructors in disguise, one validate() method that is never called anywhere in the quasi-constructors, its only state is a Stack<CARD>, just like that of a Deck, and the only remaining method, build(), does nothing but invoke one of the two dubious private constructors of Deck, optionally passing its own Stack<Card> to it. It doesn't even call validate(), either. So what is the point of this class?
  • There is a shortcut for what you do in Deck.toString():

    return deck.stream().map(CARD::toString).collect(Collectors.joining(", "));
    
  • I have not much to say about the hand class, because I agree with everything TopinFrassi said about it. Just one more heads-up: A hand having a score of 21 is not equivalent to a hand being a blackjack. Explaining why this is the case is outside the scope of this question, because it pertains to the rules of blackjack and not to programming.

\$\endgroup\$
5
\$\begingroup\$

The player class doesn't need to exist. All it does hold a name and forward operations to its hand.

Your shuffle assumes the deck starts out empty. It is a proper shuffle so you did that right. However there exists a Collections.shuffle(List) for your shuffling needs.

The ace card can have 2 possible values 1 and 11, you didn't account for that at all.

Your builder doesn't do any defensive copies which would let using code modify the internals of Deck.

\$\endgroup\$
  • \$\begingroup\$ Doesn't Player class have value from purely sematic point of view ? With respect of the Collections.shuffle(List) isn't this just a sugar. Shouldn't be any worse from quality perspective if I have implemented it self. \$\endgroup\$ – Alexandar Petrov Aug 9 '17 at 15:54
5
\$\begingroup\$

I usually don't do Java code, and TopinFrassi covered most of what I wanted to write, but

  • CARD does not follow the Java enum naming convention.
  • This

    public String toString() {
      // the sequential stream is not nessesary realy, but it makes it one idea easier to test later.
      String toString = hand.stream().sequential().map(t->{return t.name();}).reduce("", (t,q)->{return t.isEmpty()?q: t+", " + q;});
      return toString;
    }
    

    should be

    public String toString() {
      // the sequential stream is not nessesary realy, but it makes it one idea easier to test later.
      return hand.stream().sequential().map(t->{return t.name();}).reduce("", (t,q)->{return t.isEmpty()?q: t+", " + q;});
    }
    

    or possibly you could have used String.join to make this more readable (there is too much on that one line)

  • I would have preferred to see int cardsLeft = allCards.size(); instead of int max = 52;

  • You use 52 in a few places, this should be a well named constant on top

  • This part

       if (deck.size()>52) {
            throw new RuntimeException("Critical error, deck is not initialized correctly.You need 52 unique cards.");
    
        }
    

    is odd, why not deck.size()!=52 since 51 or fewer cards are bad as well? Not to mention that you check the count of unique cards later on. So you can throw away this whole if statement, and the user will not see a difference.

  • if (deck==null || allUniqueCards.size()==52) { <- seems you checked for null earlier on, why check again?

  • If I read your code correctly, then shuffling a deck twice will give you a deck with 104 cards, that just seems wrong. Perhaps call it fillDeck?

\$\endgroup\$
  • \$\begingroup\$ It appears that we nearly had a race-condition here ;) (with regard to our answers) \$\endgroup\$ – Stingy Aug 9 '17 at 19:58
  • \$\begingroup\$ Your answer is way better than mine, but we definitely saw the same things \$\endgroup\$ – konijn Aug 9 '17 at 20:02
-2
\$\begingroup\$

If I would have to judge this I would would ask: Where are the UnitTests?

Most software projects rapidly grow beyond the point where each and very developer knows every line of code well enough to predict side effects of a change. UnitTests give immediate feedback if a change accidentally broke some intended behavior. They also document what behavior you really implemented (intentionally).

So I would consider missing UnitTest being "way below my coding standards.


BTW: UnitTest can only protect code (and requirements) they cover. The only way to reliably create a sufficient coverage is doing TDD.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.