1
\$\begingroup\$

I have created this program and intend to implement it into basic card games that I create. I would like help cleaning up redundant code and overall, make it more concise. For example: I couldn't come up with a better solution for Player inheriting deck and putting card_list as a class variable for Deck. Any help would be appreciated.

import random

class Card():

    def __init__(self, value, suit):
        self.value = value
        self.suit = suit

    def show_card(self):
        print(str(self.value) + " of " + str(self.suit))

class Deck():

    card_list = []

    def __init__(self):
        self.construct_deck()
        self._playing_pile = []
        self._discard_pile = []

    def construct_deck(self):
        for s in ['Clubs', 'Diamonds', 'Hearts', 'Spades']:
            for v in ['Ace', 2, 3, 4, 5, 6, 7, 8, 9, 10, 'Jack', 'Queen', 'King']:
                self.card_list.append(Card(v, s))

    def show_deck(self):
        for card in self.card_list:
            card.show_card()

    def show_playing_pile(self):
        for card in self._playing_pile:
            card.show_card()

    def show_discard_pile(self):
        for card in self._discard_pile:
            card.show_card()

    def shuffle_deck(self):
        self.shuffled_cards = random.shuffle(self.card_list)

    def draw_card_from_deck(self):
        self._draw_from_deck = self.card_list.pop()
        return self._draw_from_deck

    def return_card_to_deck(self):
        self._return_to_deck = self.card_list.append(self._draw_from_deck)
        return self._return_to_deck

    def put_card_in_playing_pile(self):
        self._put_in_playing_pile = self._playing_pile.append(self._draw_from_deck)
        return self._put_in_playing_pile

    def put_card_in_discard_pile(self):
        self._put_in_discard_pile = self._discard_pile.append(self._draw_from_deck)
        return self._put_in_discard_pile

class Player(Deck):

    def __init__(self, name):
        self.name = name
        self._players_hand = []

    def player_draws_card_from_deck(self, deck):
        self._players_hand.append(deck.draw_card_from_deck())

    def player_returns_card_from_hand_to_deck(self, deck):
        self.card_list.append(self._players_hand.pop())

    def player_returns_card_from_playing_pile_to_deck(self, deck):
        self.card_list.append(self._playing_pile.pop())

    def player_returns_card_from_discard_pile_to_deck(self, deck):
        self.card_list.append(self._discard_pile.pop())

    def show_players_hand(self):
        for card in self._players_hand:
            card.show_card()
\$\endgroup\$
2
\$\begingroup\$

Hi there thanks for sharing your code, I have a few comments/suggestions.

Implement the __str__ method

In your code, you have a method specifically designed to print out what your card looks like. Using card.print_card() the __str__ method is a special method designed to return a string representation of our object. In your case it would look something like this.

def __str__(self):
    return str(self.value) + " of " + str(self.suit)

Calling could would look like this

print(card)

Method Names

A lot of the method names you've chosen provide information about the class as well. If I want to print a Card object, to me it would make sense to say, card.print() and not card.print_card() I already know I'm dealing with a Card. This kind of sounds like it can print any card, not just the instance I'm dealing with.

Some more examples from your code

deck.show_deck() vs deck.show()

player.player_draws_card_from_deck() vs player.draw_card_from_deck()

Player IS a Deck

In your current code, you have a player class derived from a Deck. This means that every Player IS a Deck. To me it makes more sense to use composition over inheritance. I.e. a Player HAS a Deck. But even then, a player doesn't really have a deck either, they have a hand like you have in your example.

Your game can deal with players (with hands) and one or more decks. Players could be able to draw cards FROM decks. It could be 2 different decks, or all from one deck.

Consider the following.

player1 = Player("Bob")
player2 = Player("Jim")

deck = Deck(shuffled=true) # deck used by all players
discard_pile = Deck()

player1.draw_from(hand_size=5, deck)
discard_pile.add(player1.hand())
player2.draw_from(hand_size=5, deck)
discard_pile.add(plater2.hand())

Notice here how I created another Deck instance to act as the discard pile. Really a Deck is just a bunch of cards right? I Deck doesn't have a "playing" pile and a "discard" pile. What if my game doesn't ever discard cards? By having multiple decks to represent multiple piles of cards, then I have full control. So e.g. If I want to make a solitaire game, that could be represented by a number of smaller "decks" that get added to. The managing of when cards getting added/removed can be handled in some Game class.

Unneeded class variables

Consider these 2 methods

def draw_card_from_deck(self):
    self._draw_from_deck = self.card_list.pop()
    return self._draw_from_deck

def return_card_to_deck(self):
    self._return_to_deck = self.card_list.append(self._draw_from_deck)
    return self._return_to_deck

Implementing adding/removing cards like this severely limits flexibility. At the moment, the only card I can add back to the deck is the last one I took off. But what if I run into this scenario.

As a player, I want to take a card off the top of the deck, and replace it with a different card from my hand.

This seems like a fairly reasonable thing to want to do, but at the moment, as soon as I draw 2 cards without placing one back, that other card is gone forever as it's no longer the self._draw_from_deck value anymore.

What if I had code like this

card = player.draw(deck)
card_to_insert = player.get(Card(5, "Spades"))
if card_to_insert:
    deck.place(card_to_insert)

If I had the functionality to take any card object and place it to the deck, suddenly we don't need to worry about what the last card was to be taken or the last card that was put back (if we want these values we can of course still hold onto them)

General tips and things to watch out for

Consider this line self.shuffled_cards = random.shuffle(self.card_list) and now look at this documentation about the random.shuffle method here https://docs.python.org/2/library/random.html your code isn't doing what you think it's doing here. (hint, in-place)

In your list of values, you have a mix of data types, integers and strings. I would stick to just one data type per collection. Since you want to use strings to represent "Jack", "Queen" etc. I would stick with Strings for everything "1", "2", "3" etc. And parse the integer value from it where needed.

Using PEP8 standards, any method without a leading underscore can be considered "public" and I should be able to use freely and not need to worry about unexpected results. What happens if I do the following.

deck = Deck()
deck.construct_deck()
deck.construct_deck()
deck.construct_deck()
deck.show_deck()

We constantly append to the same list.

Being able to compare cards for equality would be useful, consider overriding the __eq__ and __hash__ methods. And if you want to be able to do card1 < card2, you can also override __lt__ and __gt__

Final Comments

My final suggestion would be, try and make a card game using what you've written. See what problems you run into, what works, what doesn't work. Something straight forward like War.

Hopefully you found this review helpful!

\$\endgroup\$
  • \$\begingroup\$ Thank you for the suggestions, I am slowly working through them. \$\endgroup\$ – Throwaway25663579348 Aug 11 '17 at 21:55

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.