13
\$\begingroup\$

So in my quest to learn coding while having very little formal training has got me here. I am trying to learn the OOP concept and this is my first proper attempt at using OOP.

Can you guys point out what can be improved and what are the better practices? I have tried my best to make the program as robust as possible - the debugging took hours. Is that normal?

# Game of Blackjack

import random


# Classes

class Card:
    def __init__(self, suit, rank):
        self.suit = suit
        self.rank = rank

    def __str__(self):
        return f'{self.rank} of {self.suit}'


class Deck:

    def __init__(self, type_of_deck='empty'):
        self.cards = []
        if type_of_deck == 'full':
            for suit in suits:
                for num in ranks:
                    self.cards.append(Card(suit, num))

    def __str__(self):
        temp_string = ''
        for card in self.cards:
            temp_string += str(card)
            temp_string += ', '
        return temp_string

    def __len__(self):
        return len(self.cards)

    def draw_card(self, position=0):
        if self.cards:
            if position == 'top':
                position = 0
            elif position == 'bottom':
                position = -1
            elif position == 'random':
                position = random.randrange(len(self))
            return self.cards.pop(position)
        else:
            return False

    def insert_card(self, card, position=0):
        if not isinstance(card, Card):
            raise TypeError("Given object is not a Card")
        if position == 'top':
            position = 0
        elif position == 'bottom':
            position = -1
        elif position == 'random':
            position = random.randrange(len(self))
        self.cards.insert(position, card)

    def view_card(self, position=0):
        if self.cards:
            if position == 'top':
                position = 0
            elif position == 'bottom':
                position = -1
            elif position == 'random':
                return random.choice(self.cards)
            return str(self.cards[position])
        else:
            return 'None'

    def shuffle(self):
        random.shuffle(self.cards)

    def clear(self):
        self.cards = []


class Hand(Deck):
    def __init__(self):
        Deck.__init__(self)
        self.points = 0

    def calculate_points(self):
        aces = 0
        self.points = 0
        for card in self.cards:
            if isinstance(card.rank, int):
                self.points += card.rank
            elif card.rank == 'Ace':
                self.points += 1  # aces at minimum has a value of 1
                aces += 1
            elif isinstance(card.rank, str):
                self.points += 10
        while self.points <= 11 and aces > 0: # Tallying up aces as that is all that's left
            self.points += 10
            aces -= 1

    def insert_card(self, card, position=0):  # Extending the method in order to account for revised points after each
        # card is added
        Deck.insert_card(self, card, position)
        self.calculate_points()


class Player:
    def __init__(self, name, starting_cash=0):
        self.hand = Hand()  # initialise an empty hand object
        self.in_game = True
        self.account = starting_cash
        self.victory_state = False
        self.name = name

    def __str__(self):
        return self.name

    def bet(self, amount):
        if amount > self.account or amount < 0:
            raise ValueError('Balance_Exceeded or invalid value')
        else:
            self.account -= amount
            return amount

    def payout(self, amount):
        self.account += amount
        return True


# Functions

def display_state(deck, dealer, player, type_of_display='partial'):
    print('---------------------------------------------------------------------')
    print('Deck                    Dealer                  Player')
    print(f'{len(deck)} cards left           {len(dealer.hand)} cards on hand         {len(player.hand)} cards on hand')
    if type_of_display == 'full':
        print(f'                        points : {dealer.hand.points}             points : {player.hand.points}')
    else:
        print(
            f'                        Top card : {dealer.hand.view_card("bottom"):{15}}     points : {player.hand.points}')
    print(f'Cards in hand : {player.hand}')
    if type_of_display == 'full':
        print(f"Cards in dealer's hand : {dealer.hand}")
    print(f"Account Balance : {player.account}      Pool : {pool}      Dealer's Balance : {dealer.account}")
    print('--------------------------------------------------------------------')


# Function to payout depending on victor which is read from a boolean state (player.victory_state) in their objects

def victory_result(dealer, player, prize_pool):
    if dealer.victory_state and not player.victory_state:
        dealer.payout(prize_pool)
        print('The house has won, Better luck next time')
    elif player.victory_state and not dealer.victory_state:
        player.payout(prize_pool)
        print('Congratulations on your victory, care for another try on your luck?')
    return 0


def ask_for_new_game():
    while True:
        if input('Another run? Enter y to continue, enter any other key to exit ').upper() == 'Y':
            return True
        else:
            return False


def check_for_bust(player_1, player_2):
    if player_1.hand.points > 21:
        player_1.in_game = False
        player_2.in_game = False
        player_2.victory_state = True
        print(f'{player_1} has BUST!')


def no_bust_victory_check(player, dealer):
    if not player.victory_state and not dealer1.victory_state:
        if dealer.hand.points >= player1.hand.points:
            dealer.victory_state = True
        else:
            player.victory_state = True


def make_bet(player, dealer):
    game_state1 = True
    prize_pool = 0
    while True:
        if player.account == 0:  # End the game when player is out of cash
            print('Your account is empty, Game over!')
            game_state1 = False
            break
        if game_state1:
            try:
                prize_pool = player.bet(int(input('Enter the amount to bet ')))
                if dealer1.account < prize_pool:  # End the game when dealer is out of cash
                    print('Dealer cannot bet that amount anymore, Game Over!')
                    game_state1 = False
                    break
                prize_pool += dealer.bet(prize_pool)
                break
            except ValueError:
                print('Enter a proper value')
    return prize_pool, game_state1


def player_action(player, dealer, deck_in_play):
    while player.in_game or dealer.in_game:
        if player.in_game:
            while True:  # while loop to force proper input
                try:
                    control_value = input("Enter your choice - (H)it or (S)tand ").upper()
                    if control_value == 'H':
                        player.hand.insert_card(deck_in_play.draw_card())
                        break
                    elif control_value == 'S':
                        player.in_game = False
                        break
                    else:
                        print('Invalid value try again!')

                except TypeError:
                    print('Invalid value try again!')
        display_state(deck_in_play, dealer, player)

        check_for_bust(player, dealer)

        if dealer.in_game and dealer.hand.points <= break_point:  # Break off point is 16 right?
            dealer.hand.insert_card(deck_in_play.draw_card())
            display_state(deck_in_play, dealer, player)
        else:
            dealer.in_game = False

        check_for_bust(dealer, player)


if __name__ == "__main__":

    # Suits and Cards for creating a full deck

    suits = ('Spades', 'Hearts', 'Clubs', 'Diamonds')
    ranks = ('Ace', 2, 3, 4, 5, 6, 7, 8, 9, 10, 'Jack', 'Queen', 'King')

    # Constants
    break_point = 16
    player_starting_cash = 1000
    dealer_starting_cash = 2000

    # Initialisation of game variables
    pool = 0
    playing_deck = Deck('full')
    playing_deck.shuffle()
    player1 = Player('Player', player_starting_cash)
    dealer1 = Player('House', dealer_starting_cash)
    game_state = True

    # Main loop

    while game_state and len(playing_deck) > 10:

        player1.in_game = True
        player1.victory_state = False
        dealer1.in_game = True
        dealer1.victory_state = False
        player1.hand.clear()
        dealer1.hand.clear()

        display_state(playing_deck, dealer1, player1)
        # Let the player bet an amount and force a proper input
        pool, game_state = make_bet(player1, dealer1)  # Tuple unpacking for multiple values...

        if not game_state:  # break out of main game session since either the Player or the House has lost
            break

        # Initially draw 2 cards each for dealer and player
        for i in range(0, 2):
            dealer1.hand.insert_card(playing_deck.draw_card())
            player1.hand.insert_card(playing_deck.draw_card())
            display_state(playing_deck, dealer1, player1)

        # Let the player and dealer hit until they stand
        player_action(player1, dealer1, playing_deck)

        no_bust_victory_check(player1, dealer1)
        pool = victory_result(dealer1, player1, pool)
        display_state(playing_deck, dealer1, player1, 'full')
        game_state = ask_for_new_game()
\$\endgroup\$
1
  • 2
    \$\begingroup\$ "the debugging took hours. Is that normal?" Debugging usually takes me as long as writing the program itself. Sometimes I don't even know whether I'm debugging, creating a feature or whether it's actually refactoring. Makes it hard to track. In the end, as long as the code works and it's good code, does it matter what part of the development took the longest? \$\endgroup\$ – Mast Dec 25 '19 at 17:35
7
\$\begingroup\$

I question if a hand is a deck of cards. Certain functionality that you have, like shuffling, viewing, and inserting to specific positions, doesn't seem to make much sense in the context of a hand. A hand may be a collections of cards, similar to a deck, but that doesn't mean one is the other.

Also, I don't think having points as an attribute is ideal. That's one thing that can become mismatched later if you refactor. It's a redundant field given a hand of cards can be easily checked when needed for a value. If it was an expensive calculation, then ya, caching the results may make sense. In this case though, I can't see performance being a concern.

I'd probably get rid of the Hand class altogether, and just make it a list of cards:

class Player:
    def __init__(self, name, starting_cash=0):
        self.hand = []  # initialize an empty list
        self.in_game = True
        self.account = starting_cash
        self.victory_state = False
        self.name = name

Lists already have insert (and append) and clear methods, and can be trivially iterated to do whatever.

I'll admit, I'm biased from writing Clojure. I avoid writing collection-like-wrapper classes unless I found that there's specific, delicate states that I need. Usually, a basic collection like a dictionary or list already does mostly what you want, and wrapping it just complicates otherwise simple, easy to read operations.

To deal with points, I'd probably give the player calculate_points method (or have a standalone function) that calculates points as needed. Again, I don't think there's a ton of gain by caching that, and caching can lead to consistency problems. For example, your clear method doesn't reset points. If the user clears the deck, then tries to use points before calling calculate_points, they'll get an invalid result.


I wouldn't have Deck accept a type_of_deck string. That leads to a couple problems:

  • If the user typos the string, it will silently fail.

  • You'll need to remember to change the string in both the constructor and use-sites if you ever refactor. IDE refactoring won't be able to help you with that.

Instead, I'd make the __init__ just a basic setter, then have a populate method:

class Deck:
    def __init__(self):
        self.cards = []

    def populate(self):
        self.clear()  # To avoid double-populating

        for suit in suits:
            for num in ranks:
                self.cards.append(Card(suit, num))

Then, the user can call populate instead of passing a string. You could also, either instead of populate, or along with it, have a "pseudo-constructor" class method:

@classmethod
def new_populated(cls):
    new_deck = cls()  # Same as Deck()
    new_deck.populate()

    return new_deck

. . .

playing_deck = Deck.new_populated()  # instead of Deck('full')

I think it reads much better and will allow for less mistakes.


insert_card and view_card are more complicated than they need to be. You're having each doing multiple jobs based on a passed string. Trying to anticipate future needs and creating new features ahead of time can be helpful, but also remember: in many cases, you aren't going to need those features.

Look at your uses of both:

  • insert_card never uses the second parameter. Can you ever, within the context of a blackjack deck of cards, see ever needing to add a card to multiple positions (or really, would you ever add a card to a blackjack deck after the deck is formed?)

  • view_card is only used once, and is used to view the "bottom" card of a hand. I think normal indexing of a list would suffice here.

Unless you can think of a good reason to keep them as is, I'd get rid of view_card and reduce insert_card down to a minimum simple line of code to do an insert.

Just like before, messing around with strings to dispatch behavior is messy and can lead to errors.



We're getting ready to do Christmas stuff now, so I need to go.

Good luck and Merry Christmas.

\$\endgroup\$
2
  • \$\begingroup\$ Much thanks for the good points, a hand doesn't need the full functionality of a deck, though i figured much of the behavior was same why not use it - it's more so because I wanted to try the inheritance features in python than a proper use case. Thanks for introducing me to pseudo constructors, they sound very useful. Good catch on hand.points giving incorrect value even after the hand is clear - how would you have implemented it in an air tight manner - call it after any change to hand? Agree on the needless features. How would you dispatch behavior without strings as in display_state func? \$\endgroup\$ – Aravind Pallippara Dec 26 '19 at 10:19
  • 1
    \$\begingroup\$ @AravindPallippara For points , like I said, I would just compute them as needed instead of storing them. That'll be cheap math to do. And for dispatching, for cases where you do want to dispatch on something like a string, use an Enum (from enum import Enum). Enums give more safety since they can't easily be typo'd, and are easier to refactor. The doc page for them explains how to use them. \$\endgroup\$ – Carcigenicate Dec 26 '19 at 15:47
6
\$\begingroup\$

Don't have much time, just some things I noticed.


Deck.__str__

This method can utilize the .join built in method. Your current implementation leaves a "," at the end of the string. .join is smart enough not to. Take a look:

def __str__(self):
    return ', '.join(str(card) for card in cards).strip()

Does the exact same thing yours does, but in one line. .strip() removes the extra whitespace at the end of the string.

ask_for_new_game

This function can also be written in one line. Since you're checking one boolean expression, and returning a value either way, the while True: loop isn't necessary. There is also an easier way to return a boolean. Take a look:

def ask_for_new_game():
    return input('Another run? Enter y to continue, enter any other key to exit ').upper() == 'Y'

Since you are evaluating a boolean expression, and returning boolean values, you can simply return the expression. Since it returns a boolean value, that value will be returned.

Deck.__init__

You can simplify this method to one line using list comprehension. Take a look:

def __init__(self, type_of_deck='empty'):
    self.cards = [Card(suit, num) for num in ranks for suit in suits if type_of_deck == 'full']

While it may seem like a longer line, it reduces line count, which is almost always a good thing.

Player.bet bug

There is a bug in this method. It allows the user to bet 0 and start the game. Take a look at the first line in the method:

def bet(self, amount):
    if amount > self.account or amount < 0:
        raise ValueError('Balance_Exceeded or invalid value')
    else:
        self.account -= amount
        return amount

A simple fix is to change amount < 0 to amount <= 0.

Type Hints

You should use type hints to express what type of parameter are accepted and what values are returned by your methods/functions. Here's an example:

def bet(self, amount: int) -> int:

This says that the parameters amount should be an integer when passed, and that the method returns an integer. While this may be a trivial example, it helps as you write bigger and more expandable programs. It also helps people reading your code understand what types the method accepts.

Docstrings

You should include docstrings to your methods/functions to provide description about what each of them do. Take a look at sphinx. It's a great tool that can "create intelligent and beautiful documentation".

\$\endgroup\$
1
  • \$\begingroup\$ I am very thankful for your review. Using .join() method skipped me even though I have used it in the past. Same with list comprehension - much obliged for reminding me. The ask_ for_new_game function originally only accepted Y and N, i skipped that for some reason and ended with the old structure, good point about shortening it. I will learn to use hints, pycharm kept reminding me about that a lot throughout when writing the code. About the bug, betting zero means the pool will also be zero, hence no benefit to the player - so should it be considered a bug? \$\endgroup\$ – Aravind Pallippara Dec 25 '19 at 16:20
2
\$\begingroup\$

Specific suggestions:

  1. The try/catch blocks should be as small as possible. In the first case that means only bet = int(input('Enter the amount to bet ')) should be guarded, and in the second case only control_value = input("Enter your choice - (H)it or (S)tand ").upper().
  2. You could still take this much further in terms of OO. Game and Dealer classes, for example, would be helpful to encapsulate several of the methods.
  3. An abstract deck of cards is a sequence of cards, so it should probably inherit from list. One immediate benefit is that the custom __len__ simply goes away.
  4. view_card should return None rather than 'None' if there are no cards. Or you could even throw something like a NoCardsInDeckError since you should never reach that point.
  5. The suits and ranks are the same for all decks (at least if you're playing Blackjack) so they should probably conceptually be enums.
  6. It is generally considered good practice to implement your own exception classes rather than overloading built-in ones. And giving one thing two meanings is generally a bad idea. So

    if amount > self.account or amount < 0:
        raise ValueError('Balance_Exceeded or invalid value')
    

    should probably be something like

    if amount > self.account:
        raise BalanceExceededError()
    elif amount <= 0:
        raise TooLowBetError()
    
  7. I can't quite tell, but it looks like the support for more than one player is not yet complete. check_for_bust only allows for two players, for example.
  8. Writing this in TDD fashion would be a handy way to ensure a minimal and correct implementation. You would probably need in the order of 30+ tests to ensure basic game compliance and many more to get to a bulletproof implementation.
  9. More generally, asking yourself questions like "Is it still a deck if there are no cards in it?" or "Who or what is responsible for this?" can be useful ways to work out the model.
\$\endgroup\$
2
  • \$\begingroup\$ I understand that a 'game' class would have helped, but why would a dealer class make sense when much of the functionality is extremely similar to a player? Also is it better to go for full OOP or mix and match paradigms to make code more readable? Agree with inheriting from list, that makes a lot of sense, just would have had to extend the insert method with isinstance check. I was lazy about implementing exception classes, I will keep that in mind for next time. And yeah there isnt support for more than 2 players, I should expand it. I haven't got to using unit testing proper, I should soon \$\endgroup\$ – Aravind Pallippara Dec 26 '19 at 10:25
  • 1
    \$\begingroup\$ Good point about the Dealer class. One thing which comes to mind is that the dealer is probably better suited than the game (and certainly the players!) to handle the pot and winnings. As usual it depends a lot on the structure of the rest of the game. As for unit testing, I would highly recommend learning TDD, not just unit testing. I used unit testing for some years before being taught actual TDD, and it makes a huge difference to the testability and conciseness of your result. \$\endgroup\$ – l0b0 Dec 27 '19 at 4:35

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.