OOP Python card game class methods

Question

I have written a simple card game where upon initialising the game each player is given two cards.

At the bottom I have created a method called returnCards that puts the cards of a player back in the deck.

I am new to OOP in python and am curious if it's good convention to create a standalone method like this?

I feel as though this method should actually be a Player class method but I'm unsure how to write it as such. More than anything I'm trying to understand good practices when writing OOP code

import random

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

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

    def build(self):
        for i in ["Hearts", "Diamonds", "Spades", "Clubs"]:
            for j in range(1,14):
                self.cards.append(Card(i, j))

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

class Game:
    def __init__(self, players, deck):
        self.players = players
        self.deck = deck
        self.cards = deck.cards

    def deal(self):
        for player in self.players:
            for i in range(2):
                player.cards.append(self.drawCard())

    def drawCard(self):
        drawnCard = self.deck.cards[0]
        self.deck.cards = self.deck.cards[1:]
        return drawnCard

class Player:
    def __init__(self, name):
        self.name = name
        self.cards = []


def returnCards(player, game):
    game.deck.cards.append(card for card in player.cards)
    player.cards = []


deckOfCards = Deck()
playerOne = Player("John")
playerTwo = Player("Harrry")
newGame = Game([playerOne, playerTwo], deckOfCards)
newGame.deal()
returnCards()

Answer 1

I would consider having two return_cards methods, on Player and Deck

class Player:

    ...

    def return_cards(self, deck):
        deck.return_cards(self.cards)
        self.cards = []


class Deck:

    ...

    def return_cards(self, cards):
        self.cards.extend(cards)

The idea is that objects handle their own responsibilities -they do things to themselves - rather than managing other objects' responsibilities. So Player is responsible for returning its cards to Deck, and Deck is responsible for what happens to the cards once they have been returned.

Another consideration is that we don't want objects to know too much about each others internals.

game.deck.cards.append(card for card in player.cards)

means that Game "knows" that Deck has a list named cards. This kind of design is couples objects to tightly - if Deck.cards becomes a set or a dict we have to change Game - it's better to access cards via methods rather than directly. See also encapsulation and the Law of Demeter.

Game would be responsible for telling Player that it's time to return the hand.

Answer 2

Yes, using separately defined functions is ok, but only when the function is not related and strongly tied with the objects (formatters etc.).

In your case it actually should be a method of Game class at it is basically a new game beginning. Take a look of how it might look with some tips in comments:

from collections import deque
import random


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

    # good practice is to include repr and str magic methods - that way you can print meaningful info
    def __repr__(self):
        return '<{} {}>'.format(self.value, self.suit)


class Deck:
    def __init__(self):
        # use deque - it's more efficient to pop from left than from a list -> good advice -> learn build in types :)
        self.cards = deque()
        self.build()

    def build(self):
        self.cards.clear()
        for i in ["Hearts", "Diamonds", "Spades", "Clubs"]:
            for j in range(1, 14):
                self.cards.append(Card(i, j))
        # need to shuffle in order to have random order of cards
        random.shuffle(self.cards)

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

    def get_card(self):
        return self.cards.popleft()

    def __repr__(self):
        return 'Deck: [{} Cards]'.format(self.length())


class Game:
    def __init__(self, players, deck):
        self.players = players
        self.deck = deck

    def deal(self):
        for player in self.players:
            for i in range(2):
                player.give_card(self.draw_card())

    # in python use underscore not camelCase for methods
    def draw_card(self):
        # avoid mutating other object properties in other objects
        return self.deck.get_card()

    def restart_game(self):
        # simply rebuild the deck - it will reshuffle all cards
        self.deck.build()
        for player in self.players:
            player.return_cards()


class Player:
    def __init__(self, name):
        self.name = name
        self.cards = []

    def give_card(self, card):
        self.cards.append(card)

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

    def __repr__(self):
        return 'Player: {} - [{}]'.format(self.name, ','.join([str(x) for x in self.cards]))


deck_of_cards = Deck()
player_one = Player("John")
player_two = Player("Harrry")
new_game = Game([player_one, player_two], deck_of_cards)
new_game.deal()
print(player_one, player_two)
print(new_game.deck)
new_game.restart_game()
print(player_one, player_two)
print(new_game.deck)

If for some reason not stated here you want to keep the original card objects (i.e. to count how many times the card was drawn etc.) you can follow @snakecharmerb answer (methods in deck and player) and I would handle the logic behind this in Game class (that way you don't need player and deck referencing each other, and it is more logical from the business perspective)

Answer 3

There is no absolute right and wrong when doing OOP. It's just a conceptual philosophy which helps you organize your code to achieve maintainability and sustainability.

You will do a lot of refactoring works in the future when the use case changes(it will always change). The changing use case may also affect how you organize your return_cards method.

So all you have to worry about is how the code PRESENTLY look like, is it understandable enough to you and to people you are working with? Again, there is no absolute answer.

Here is my opinion.

• Having a global return_cards function seems not reasonable. You are mixing two types of philosophy, procedural programming and object-oriented programming, together, while provides very limited clues and boundary to explain why it goes this way.

• PRESENTLY, I think return_cards() could only locate in the Game class. Since either Deck and Player can be regarded as the status of the Game. So let the method of the Game change the status of Game, it's perfectly understandable.

• Of course, you can do something like this:

class Game:
    ......
    def return_cards(self):
        for player in self.players:
            player_cards = self.players.give_all_cards_back()
            self.decks.collect_all_cards(player_cards)

but I don't think it's necessary PRESENTLY. When will it be? For example, when some other modules, such as UI or some text-printing procedure, are depending on your Player object, you have to notify them when players return back their card. Then it's reasonable to create something like Player.give_all_cards_back() and encapsulate all the related procedure together.

Here is some sample code from Ramalho's Fluent Python (O'Reilly), demonstrating how to build a Deck class, for your reference.

import collections

Card = collections.namedtuple("Card", ("rank", "suit"))

class FrenchDeck:
    ranks = [str(n) for n in range(2, 11)] + list("JQKA")
    suits = "spades diamonds clubs hearts".split()

    def __init__(self):
        self._cards = [Card(rank, suit) for suit in self.suits for rank in self.ranks]