Blackjack game interview task

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.

• 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.

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.

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?

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.