7
\$\begingroup\$

I worked on this for a software engineer interview as the take home challenge. The company came back with this constructive feedback:

  1. You made a slight mistake in ace handling that led to the inability to handle multiple aces
  2. You hard-coded global variables for player1 and player2 state (why is this bad?)
  3. There was also some duplicated print code, some minor style issues, and a small functionality issue.

Although the interview process is over, I want to improve this code for my own knowledge. Any advice you have on the above three points or additional feedback would be appreciated!

import random

#initializing constants
CARDS = ('A', '2', '3', '4', '5', '6', '7', '8', '9', '10', 'J', 'Q', 'K')
RANKS = {'A':1, '2':2, '3':3, '4':4, '5':5, '6':6, '7':7, '8':8, '9':9, '10':10, 'J':10, 'Q':10, 'K':10}

'''Card consists of one element in RANKS. 
This class is capable of returning the 
value of the rank and also returning a
printable string representation of a card.'''
class Card:

    '''rank refers to an element in RANKS.
    If the value exists in RANKS, initialize it.
    Else, print "Invalid card: 'rank'"'''
    def __init__(self, rank):
        if(rank in RANKS):
            self.rank = rank
        else:
            self.rank = None
            print("Invalid card: ", rank)

    #Returns the value of this card's rank
    def get_rank(self):
        return self.rank

    #Returns printable string representation of the card
    def __str__(self):
        return str(self.rank)

'''Hand consists of a list of cards.
This class is capable of adding 
a card, returning the total value of the
hand, printing face up cards, showing the 
player their full hand if they consent, 
checking if the player has a bust, and
returning a printable representation
of a hand.'''
class Hand:
    #hand refers to a list of cards
    def __init__(self):
        self.hand = []

    #Adds a card to the list
    def add_card(self, card):
        self.hand.append(card)

    #Returns the value of a hand
    def get_value(self):
        value = 0
        ace = 0
        for card in self.hand:
            value += RANKS[card.get_rank()]
            if str(card.get_rank()) == 'A':
                '''If the value of the hand is <= 11, we want an Ace to be worth 11 points.
                Since 1 has already been added because 'A' : 1 in VALUES, 
                we only need to add 10 additional points'''
                if value <= 11:
                    value += 10

        return value

    '''Return cards visible to both players 
    (everything except the first card)'''
    def print_face_up_cards(self):
        for i in range(1, (len(self.hand))):
            print(self.hand[i], end = ' ')
        print()

    '''Asks the player if they want to view their hand and 
    proceeds accordingly'''
    def viewHand(self):
        global turn
        view = input("\n" + turn  + ": Do you want to view your full hand? Type 'yes' to view hand, type 'no' to continue.\n")

        #User input check 
        while view.lower() != 'yes' and view.lower() != 'no':
            view = input(turn + ": Please type 'yes' or 'no' to continue.\n")

        if view.lower() == 'yes':
            print("\n" + turn + "'s Hand: ", self, sep = "")
            print(turn + "'s Hand total: ", self.get_value(), sep = "")
        print()

    #Check if the player's hand is over 21 (bust)
    def bust(self):
        global turn, playerBust, player1, player2

        #If the player has a bust, proceed according to player
        if self.get_value() > 21:
            playerBust = True
            if turn == 'Player 1':
                print(turn + ": you have gone over 21. You're busted! Player 2 wins!")
                #Print Player 1's hand and total
                print("Player 1's Hand: ", self)
                print("Player 1's Hand total: ", self.get_value())
                #Print Player 2's hand and total
                print("Player 2's Hand: ", player2)
                print("Player 2's Hand total: ", player2.get_value())
            if turn == 'Player 2':
                print(turn + ": you have gone over 21. You're busted! Player 1 wins!")
                #Print Player 1's hand and total
                print("Player 1's Hand: ", player1)
                print("Player 1's Hand total: ", player1.get_value())
                #Print Player 2's hand and total
                print("Player 2's Hand: ", self)
                print("Player 2's Hand total: ", self.get_value())

    #Returns printable string representation of the hand
    def __str__(self):
        cards = ''
        for card in self.hand:
            cards += str(card) + " "
        return cards.strip()

'''Deck consists of a list of cards.
This class is capable of dealing a card to a hand,
shuffling a deck, and returning a printable string
representation of a deck'''
class Deck:
    '''deck refers to a list of cards.
    Since I am not accounting for suit, 
    I add all of the cards 4 times to 
    acheive the correct total'''
    def __init__(self):
        self.deck = []
        for i in range (4):
            for rank in RANKS:
                self.deck.append(Card(rank))

    '''Deal a single card and remove it
    from the deck.'''
    def deal(self):
        return self.deck.pop()

    #Shuffle the deck.
    def shuffle(self):
        random.shuffle(self.deck)

    #Returns printable string representation of the deck.
    def __str__(self):
        cards = ''
        for card in self.deck:
            cards += str(card) + " "
        return cards.strip()

'''This function takes in the player and the player's hand.
 This function facilitates the logic of a single turn
in Blackjack'''
def playerTurn(player, playerHand):
    global player1Stay, player2Stay

    #Print whose turn it is
    print('Turn: ' + player)

    #Ask the player if they want to view their hand and proceed accordingly
    playerHand.viewHand()

    #Ask the player if they want to hit or stay and proceed accordingly
    hitOrStay = input (player + ": Do you want to hit or stay? Type 'hit' to receive another card, type 'stay' to stop taking cards.\n")

    #User input check 
    while hitOrStay.lower() != 'hit' and hitOrStay.lower() != 'stay':
        hitOrStay = input(player + ": Please type 'hit' or 'stay' to continue.\n")

    if hitOrStay.lower() == 'hit':
        #Deal a card and add it to the hand
        playerHand.add_card(deck.deal())
        print("\nOne card has been added to ", player, "'s hand.", sep="")
        #Print player's face up cards
        print(player + "'s face-up cards: ", end = ' ', sep="")
        playerHand.print_face_up_cards()
        #Ask the player if they want to view their hand and 
        #proceed accordingly        
        playerHand.viewHand()

    #Record if the player decides to stay
    elif hitOrStay.lower() == 'stay': 
        if player == 'Player 1':
            player1Stay = True
        if player == 'Player 2':
            player2Stay = True
    return

#Check results of the game when both players are done
def checkOutcome():
    global player1, player2
    #Print both player's hands and values
    print("Player 1 Hand: ", player1)
    print("Player 1 Hand Total: ", player1.get_value())
    print()
    print("Player 2 Hand: ", player2)
    print("Player 2 Hand Total: ", player2.get_value())

    #Print outcome if there is a tie or a winner
    if player1.get_value() == player2.get_value():
        print("Both players have ", player1.get_value(), "! There is a tie!", sep = '')
    elif player1.get_value() == 21:
        print("Player 1 has Blackjack! Player 1 wins!")
    elif player2.get_value() == 21:
        print("Player 2 has Blackjack! Player 2 wins!")
    else:
        print("\n***Player 1 Wins!***" if player1.get_value() > player2.get_value() else "\n***Player 2 Wins!***")

#Global variables
deck = Deck()
player1Stay = False
player2Stay = False
playerBust = False
turn = 'Player 1'
player1 = Hand()
player2 = Hand()

def main():
    global player1Stay, player2Stay, playerBust, turn, deck, player1, player2

    #Shuffle the deck
    deck.shuffle()

    #Deal two cards to each player's hand
    for i in range(2):
        player1.add_card(deck.deal())
        player2.add_card(deck.deal())
    print("\nThe cards have been dealt! Each player has two cards.")

    #While either player has not "stayed" and neither player's hand is over 21 (bust)
    #game continues
    while (not player1Stay or not player2Stay) and not playerBust:
        #Print both player's face up cards
        print("Player 1's face-up cards: ", end = '')
        player1.print_face_up_cards()
        print("Player 2's face-up cards: ", end = '') 
        player2.print_face_up_cards()
        print()

        #Facilitate the respective player's 
        #turn and check if they have a bust
        if turn == 'Player 1':
            playerTurn(turn, player1)
            player1.bust()
        else:
            playerTurn(turn, player2)
            player2.bust()

        #Change turns 
        if turn == 'Player 1' and not player2Stay:
            turn = 'Player 2'
        elif turn == 'Player 2' and not player1Stay:
            turn = 'Player 1'

    #If both players have stayed, check the outcome to finish the game
    if not playerBust:
        checkOutcome()

main()
\$\endgroup\$
  • \$\begingroup\$ One minor issue I found while eyeballing is: Your docstrings are above your classes. Why is that? They should be inside your class :) Maybe, pep8 your code once. \$\endgroup\$ – Dawny33 Sep 23 '16 at 4:32
5
\$\begingroup\$

I'll try to address some of those minor style issues which he/she might have meant:

  1. As I've pointed out in the comment, please pep8 (lint) your code. It would help you iron out a lot of style issues in the code. Your docstrings for the functions should also be inside your function definition. The follow the following format:

    What it does
    :param_name: explain about the parameter
    :return: Explain what the method returns
    
  2. Your naming convention is a bit off. In the line self.rank = rank, use an underscore for the instance variables. [A nice reference to why]

  3. By You hard-coded global variables for player1 and player2 state, I guess maybe they want to hint that they want the players as classes, and then instantiate them with required params and payer-specific methods. (I am not completely sure about this. Maybe, let a better dev confirm this)

\$\endgroup\$
2
\$\begingroup\$

Handling multiple aces

This may be what they meant when they said you can't handle multiple aces:

#Returns the value of a hand
def get_value(self):
    value = 0
    ace = 0
    for card in self.hand:
        value += RANKS[card.get_rank()]
        if str(card.get_rank()) == 'A':
            '''If the value of the hand is <= 11, we want an Ace to be worth 11 points.
            Since 1 has already been added because 'A' : 1 in VALUES, 
            we only need to add 10 additional points'''
            if value <= 11:
                value += 10

    return value

This might be ok if hand was sorted, however this doesn't appear to be the case. The assumption that you can add 10 if the rolling value is <= 11 is incorrect because it means that the order you draw the ace is important. If you get (A, 7, 7), then I don't want the ace to be worth 11, I want it to be worth 1, which is what it would have calculated if I had got (7,7,A). This can be demonstrated if I force cards to be drawn:

Player 1's Hand: A 7 7
Player 1's Hand total: 25

Player 1: you have gone over 21. You're busted! Player 2 wins!
Player 1's Hand:  A 7 7

In this scenario, the first ace should be treated as 1 (giving a total of 15), not 11. A better strategy would be to sum all the values (using 1 for ace) and increment the ace variable you aren't using whenever an ace is encountered. After adding all values, you can then add 10 if ace > 0.

Player1 and Player2

As far as the global players go, you've imposed unnecessary restrictions on your program by having hard coded player variables like player1Stay. You've gone to the effort of creating a class for a Hand to hold the cards, adding a Player class to hold the state would make it easier for you to make changes going forward. For example you could easily add a configurable name to the players, or work from a player collection so that you can have 1-n players instead of always 2.

\$\endgroup\$
  • \$\begingroup\$ Thank you for explaining! I thought that the order in which the card was drawn did matter, since I thought the value was decided when the Ace is dealt, based on the current total at that time. Is this actually decided after both players have stayed and the dealer is determining a winner? \$\endgroup\$ – mel Sep 23 '16 at 23:58
  • \$\begingroup\$ @mel The player gets to pick if they want to use an ace as 1 or 11, but they can change their mind with each new card dealt. So, an initial deal of 5 and ace would be scored 16. If the player was then dealt a 6, they would have to either bust (which they won't choose to do) or use the ace as a1 instead, to give a new total of 12. \$\endgroup\$ – forsvarir Sep 24 '16 at 0:58

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.