I have programmed a card game in Python. It is over 700 lines long. How can I improve the code?

There are two players. Every round, each player takes one card from the top of the deck. Those cards are compared, and the winner of the round is assigned based on the following rules:

  • Red beats black
  • Black beats yellow
  • Yellow beats red

If both cards have the same colour, the card with the highest number wins. If they have the same colour and number, it is a draw.

The winner of the round keeps both cards. If the round was a draw, the players keep their own cards.

This is repeated until the deck is empty. The winner is the person with the most cards at the end of the game.

I used an online IDE called Repl.it. You can view and run the program here: NEA Card Game


import pickle
import os

from random import randint, shuffle
from time import sleep

# Card index constants

# Player constants
DRAW = 'draw'
PLAYER1 = '1'
PLAYER2 = '2'

def clear():
    print('\n' * 100)

def main():



    game = Game()

    menu_loop = True

    while menu_loop:


        with open('image.txt', 'r') as image_file:
            image_text = image_file.read()

        for c in image_text:
            if c == '0':
                print(' ', end='')
            elif c == '1':
                print('*', end='')
                print(c, end='')


        print('|                                            |')
        print('| 1 - Play game                              |')
        print('| 2 - Create a new deck                      |')
        print('| 3 - Load a deck                            |')
        print('| 4 - Delete a deck                          |')
        print('| 5 - Change the speed of the game           |')
        print('| 6 - View the leaderboard                   |')
        print('| 7 - Quit                                   |')
        print('|                                            |')

        menu_option = input('\nEnter menu option: ')

        if menu_option == '1':
            input('Press enter to continue.')

        elif menu_option == '2':
            input('Press enter to continue.')

        elif menu_option == '3':
            input('Press enter to continue.')

        elif menu_option == '4':
            input('Press enter to continue.')

        elif menu_option == '5':
            input('Press enter to continue.')

        elif menu_option == '6':
            top5 = FileFunctions.read_top5()
            input('Press enter to continue.')

        elif menu_option == '7':
            menu_loop = False

            print('\nPlease choose a number from the menu.\n')
            input('Press enter to continue.')

def login():

    # The password (for now) is 'Python'.
    password = FileFunctions.get_password()

    valid = False

    while not valid:

        password_attempt = input('Enter password: ')

        if password_attempt == password:
            valid = True
            print('Incorrect password.')

# Returns a tuple containing a colour and a number.
def new_card(colour):
    return (colour, randint(1, 10))

# Creates a new random deck.
# number_of_cards must be a multiple of 3 so there can
# be an even amount of each colour in the deck.
def new_random_deck(name, number_of_cards):

    deck = []

    for i in range(int(number_of_cards / 3)):

    FileFunctions.write_deck(name, deck)

def display_leaderboard(players):

    length = 20


    print('=' * (length + 2))

    for i in range(len(players)):

        score = len(players[i]) - 1

        string_part1 = str(i+1) + ' | ' + players[i][0]
        string_part2 = ' ' * (length - len(string_part1)) + str(score)

        print("%s%s" % (string_part1, string_part2))

    print('=' * (length + 2))


def change_deck_menu():

    valid = False

    while not valid:

        yes_or_no = input('Loading a different deck will reset the leaderboard. Do you wish to proceed? (y/n)').lower()

        if yes_or_no == 'y':
            valid = True
        elif yes_or_no == 'n':
            print('\nDeck has not been loaded.\n')
            print('\nPlease answer with \'y\' or \'n\'.\n')


    valid = False

    while not valid:

        deck_name = input('\nEnter name of deck to load: ')

        if deck_name.strip() == '':
            print('\nThe deck name will contain at least one visible character.')

            f = open(deck_name + '.bin', 'r')
            valid = True

        except FileNotFoundError:
            print('\nDeck \'%s\' does not exist.' % deck_name)
            valid = False


    print('\nDeck \'%s\' has been loaded.\n' % deck_name)

def new_deck_menu():


    valid = False

    while not valid:

        yes_or_no = input('\nAre you sure you want to create a new deck? (y/n)')
        yes_or_no = yes_or_no.lower()

        valid = True

        if yes_or_no == 'y':

            name_valid = False

            while not name_valid:

                deck_name = input('\nEnter deck name: ')

                if deck_name.strip() == '':
                    print('\nThe deck name must contain at least one visible character.\n')

                elif ' ' in deck_name:
                    print('\nThe deck name cannot contain spaces.\n')

                elif '.' in deck_name:
                    print('\nThe deck name cannot contain dots (the file extension will be added automatically).\n')

                elif '\\' in deck_name or '/' in deck_name:
                    print('\nThe deck name cannot contain slashes.\n')

                    name_valid = True

            number_valid = False

            while not number_valid:

                number = int(input('Enter amount of cards: '))

                if number % 2 == 0 and number % 3 == 0:
                    number_valid = True

                    print('\nAmount must be an even multiple of 3.')

            new_random_deck(deck_name, number)

            print('\nThe new deck has been created.')

        elif yes_or_no == 'n':
            print('\nCreation of new deck has been cancelled.')

            print('\nPlease answer with \'y\' or \'n\'.')
            valid = False


def change_speed():

    valid = False

    current_delay = FileFunctions.load_round_delay()
    current_delay = round(current_delay, 3)
    current_delay = str(current_delay)

    while not valid:

        print('The current round delay is %s seconds.' % (current_delay))

        yes_or_no = input('Are you sure you want to change the speed of the game? (y/n)').lower()
        valid = True

        if yes_or_no == 'y':

            input_loop = True

            while input_loop:

                input_loop = False

                    seconds = float(input('\nEnter delay between each round in seconds: '))

                    if seconds < 0:
                        print('The round delay cannot be a negative number.')
                        input_loop = True

                except ValueError:
                    input_loop = True
                    print('Please enter a float or an integer.')


            print('\nThe new round delay has been saved.')

        elif yes_or_no == 'n':
            print('\nChanging of game speed has been cancelled.')

            print('\nPlease answer with \'y\' or \'n\'.\n')
            valid = False

def delete_deck_menu():

    valid = False

    while not valid:

        deck_name = input('\nEnter the name of the deck you want to delete: ')

        if deck_name.strip() == '':
            print('\nThe deck name will contain at least one visible character.')
        elif ' ' in deck_name:
            print('\nThe deck name will not contain spaces.')
        elif '.' in deck_name:
            print('\nPlease only enter the name of the deck. The file extension will be added automatically.\n')

                with open(deck_name + '.bin', 'r'):
                    valid = True

            except FileNotFoundError:
                valid = False
                print('\nDeck \'%s\' could not be found. Make sure you have spelt the name correctly.' % deck_name)

    os.remove(deck_name + '.bin')

    print('\nDeck \'%s\' has been deleted.\n' % deck_name)

class Game:

    def __init__(self):

        self.player1_name = ''
        self.player2_name = ''

    def play(self):

        round_delay = FileFunctions.load_round_delay()

        self.player1_name, self.player2_name = self._get_names()

        deck_name = FileFunctions.load_current_deck_name()

        play_again = True

        while play_again:

            # Read the deck from the deck file.
                deck = FileFunctions.load_deck(deck_name)
            except FileNotFoundError:
                print('\nCould not find the deck file. Try loading a different deck with option 3 of the main menu.\n')

            player1_cards = []
            player2_cards = []


            game_round = 1


            while len(deck) > 0:


                player1_card = deck[-1]
                player2_card = deck[-2]


                print('ROUND', game_round, '\n')

                self._display_cards(player1_card, player2_card)

                winner = self._compare_cards(player1_card, player2_card)

                if winner == PLAYER1:
                    print('\nWinner:', self.player1_name)
                elif winner == PLAYER2:
                    print('\nWinner:', self.player2_name)
                    print('\nWinner: draw')


                if winner == PLAYER1:

                elif winner == PLAYER2:

                # If it is a draw, the players keep their own cards.

                game_round += 1

            if len(player1_cards) > len(player2_cards):
                winner = self.player1_name
                winning_cards = player1_cards

            elif len(player1_cards) < len(player2_cards):
                winner = self.player2_name
                winning_cards = player2_cards

                winner = DRAW
                winning_cards = []

            print('%s has %d cards.' % (self.player1_name, len(player1_cards)))
            print('%s has %d cards.\n' % (self.player2_name, len(player2_cards)))

            print('Winner of game:', winner)

            if winner != DRAW:
                FileFunctions.write_name_and_cards(winner, winning_cards)
                self._display_winning_cards(winner, winning_cards)

            valid = False

            while not valid:
                yes_or_no = input('\nWould you like to play again? (y/n)').lower()
                valid = True
                if yes_or_no == 'n':
                    play_again = False
                elif yes_or_no != 'y':
                    print('Please answer with \'y\' or \'n\'.')
                    valid = False


    def _get_names(self):


        valid = False

        while not valid:

            player1_name = input('Enter player 1\'s name: ')

            if '_' in player1_name:
                print('Names cannot contain underscores.')
            elif player1_name.strip() == '':
                print('The name must contain at least one visible character.')
            elif len(player1_name) > 15:
                print('The name cannot contain more than 15 characters.')
                valid = True

        valid = False

        while not valid:

            player2_name = input('Enter player 2\'s name: ')

            if '_' in player2_name:
                print('Names cannot contain underscores.')
            elif player2_name.strip() == '':
                print('The name must contain at least one visible character.')
            elif player2_name == player1_name:
                print('Player 1 and player 2 must have different names.')
            elif len(player2_name) > 15:
                print('The name cannot contain more than 15 characters.')
                valid = True

        return (player1_name, player2_name)

    def _display_cards(self, card1, card2):

        space_length = 25 - len(self.player1_name)

        players_string = ''
        players_string += self.player1_name.upper()
        players_string += ' ' * space_length
        players_string += self.player2_name.upper()

        print('=' * 40)
        print('=' * 40)

        print('colour:', card1[COLOUR], end='')
        space_length = 17 - len(card1[COLOUR])
        print(space_length * ' ', end='')
        print('colour:', card2[COLOUR])

        print('number:', card1[NUMBER], end='')
        space_length = 17 - len(str(card1[NUMBER]))
        print(space_length * ' ', end='')
        print('number:', card2[NUMBER])

    def _compare_cards(self, card1, card2):

        if card1[COLOUR] == card2[COLOUR]:

            if card1[NUMBER] > card2[NUMBER]:
                return PLAYER1
            elif card1[NUMBER] < card2[NUMBER]:
                return PLAYER2
                return DRAW


            if card1[COLOUR] == 'red':
                return PLAYER1 if card2[COLOUR] == 'black' else PLAYER2

            elif card1[COLOUR] == 'black':
                return PLAYER1 if card2[COLOUR] == 'yellow' else PLAYER2

            elif card1[COLOUR] == 'yellow':
                return PLAYER1 if card2[COLOUR] == 'red' else PLAYER2

    def _display_winning_cards(self, winner, winning_cards):

        while winner[-1] == ' ':
            winner = winner[:-1]

        if winner[-1].lower() == 's':
            winner += '\''
            winner += '\'s'

        print('\n%s CARDS:\n' % winner.upper())

        space_const = 15

        space_after_colour = ''

        length_of_largest_int = len( str( len(winning_cards) + 1 ) )

        for i in range(len(winning_cards)):

            card = winning_cards[i]

            space_after_colour = ' ' * ( space_const - len(card[0]) )
            space_after_number = length_of_largest_int - len(str(i+1))

            card_string = str(i+1)
            card_string += ' ' * space_after_number
            card_string += ' | COLOUR: ' + card[0] + space_after_colour + 'NUMBER: ' + str(card[1])


class FileFunctions:

    # Reads the current deck's name
    def load_current_deck_name():
        with open('bin_files/current_deck_name.bin', 'rb') as cd_file:
            return pickle.load(cd_file)

    def change_current_deck_name(new_name):
        with open('bin_files/current_deck_name.bin', 'wb') as cd_file:
            pickle.dump(new_name, cd_file)

    # Writes deck to a file
    def write_deck(name, deck_array):

        deck_string = ''

        for card in deck_array:

            deck_string += card[0]
            deck_string += ','
            deck_string += str(card[1])
            deck_string += '\n'

        with open(name + '.bin', 'wb') as deck_file:
            pickle.dump(deck_string, deck_file)

    # Reads deck from a file and returns it as an array
    def load_deck(name):

        deck_array = []

        with open(name + '.bin', 'rb') as deck_file:
            deck_text = pickle.load(deck_file)

        deck_text = deck_text.split('\n')

        for card_string in deck_text:

                card = card_string.split(',')
                card[1] = int(card[1])
                deck_array.append( (card[0], card[1]) )
            except IndexError: # The line is empty

        return deck_array

    def get_password():
        with open('bin_files/password.bin', 'rb') as passcode_file:
            return pickle.load(passcode_file)

    def clear_leaderboard():
        with open('bin_files/win.bin', 'wb') as win_file:
            pickle.dump('', win_file)

    # Writes name and cards to win.bin
    def write_name_and_cards(name, cards):

        # Write the name and cards to the file

            with open('bin_files/win.bin', 'rb') as win_file:
                win_string = pickle.load(win_file)
        except EOFError:
            win_string = ''

        win_string += name

        for card in cards:
            win_string += '\n'
            win_string += card[0]
            win_string += ','
            win_string += str(card[1])

        win_string += '_'

        with open('bin_files/win.bin', 'wb') as win_file:
            pickle.dump(win_string, win_file)

        # Delete any players not in the top 5

        # Read all players from win.bin
        with open('bin_files/win.bin', 'rb') as win_file:
            players_string = pickle.load(win_file)

        # Convert the string into an array.
        players = players_string.split('_')

        # Convert the array into a 2D array.
        for i in range(len(players)):
            players[i] = players[i].split('\n')

        # Remove ['']
        while players[-1] == ['']:

        top5 = []

        while len(top5) < 5:

            index_of_highest = 0

            for i in range(len(players)):
                if len(players[i]) > len(players[index_of_highest]):
                    index_of_highest = i

            except IndexError:
                break # The players array contains less than 5 players.

        top5_string = ''

        for player in top5:
            top5_string += '\n'.join(player)
            top5_string += '_'

        with open('bin_files/win.bin', 'wb') as win_file:
            pickle.dump(top5_string, win_file)

    # Returns the top 5 players from win.txt as a tuple
    def read_top5():

        with open('bin_files/win.bin', 'rb') as win_file:
            players = pickle.load(win_file)

        players = players.split('_')

        for i in range(len(players)):
            players[i] = players[i].split('\n')

            while players[-1] == ['']:
        except IndexError: # The players array might be empty

        return players

    def write_round_delay(seconds):
        with open('bin_files/round_delay.bin', 'wb') as rd_file:
            pickle.dump(seconds, rd_file)

    def load_round_delay():
        with open('bin_files/round_delay.bin', 'rb') as rd_file:
            return pickle.load(rd_file)

if __name__ == '__main__':



When you first run the program, you will need to choose the create deck option, and then load it in the load deck option. Then you can choose the play game option.

  • \$\begingroup\$ I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Feel free to edit and give it a different title if there is something more appropriate - perhaps you could give a more descriptive name than just card game... \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Oct 10 '18 at 18:11
  • 6
    \$\begingroup\$ what is the game logic? what kind of card game? I thought it was some derivation from standard deck of playing cards, but it's not! \$\endgroup\$ – hjpotter92 Oct 10 '18 at 20:25

Well, for starters, you can make your script half the size by deleting all the unneeded blank lines...

This is actually one of the things defined in Python's official style-guide, PEP8:

Blank lines

Surround top-level function and class definitions with two blank lines.

Method definitions inside a class are surrounded by a single blank line.

Extra blank lines may be used (sparingly) to separate groups of related functions. Blank lines may be omitted between a bunch of related one-liners (e.g. a set of dummy implementations).

Use blank lines in functions, sparingly, to indicate logical sections.

Note the repeated use of the word "sparingly".

It is well-worth a read and will make your code easier to read by other Python developers.

Most developers use editors/editor plugins that automatically format the code according to PEP8 whenever the file is saved. I personally use Atom with the beautify plugin for this. There is also a commandline tool, autopep8, which can be run on a script file and is actually called behind the scenes by many of these plugins.

Another thing is your comments above your functions. These should instead be docstrings, which allow you to access the documentation in an interactive session (and using automatic documentation builder tools).

Here is how it could look like for (a randomly chosen) function from your code:

def new_random_deck(name, number_of_cards):
    """Create a new random deck.
    `number_of_cards` must be a multiple of 3 so there can
    be an even amount of each colour in the deck.

    The deck will be saved using `name` as file name.
    assert number_of_cards % 3 == 0, "`number_of_cards` must be a multiple of 3"
    for _ in range(int(number_of_cards / 3)):
        deck.extend([new_card('red'), new_card('black'), new_card('yellow')])
    FileFunctions.write_deck(name, deck)

Note that I also removed the blank lines here, used list.extend instead of three list.append calls, used _ as the agreed-upon name of an unused iteration variable and explicitly asserted that the restriction on number_of_cards is actually true (assert statements can be disabled by running the script with the -O flag).

You could also use the itertools module (which is in the standard library) to make this into a list comprehension:

from itertools import islice, cycle

colours = 'red', 'black', 'yellow'
deck = [new_card(colour) for colour in islice(cycle(colours), number_of_cards)]

islice works just like normal list slicing, but on iterators (which can be infinite). cycle just infinitely cycles through the given iterable, starting at the beginning after having reached the end.

While I can't say that I actually understand the game (have not tried playing it yet), it seems weird to me that when you create a random new deck that while the number of cards in that deck is restrained, the content of the deck is not (in other words there is no way to ensure that it returns a random permutation, i.e. a shuffled version, of a deck). Or, a deck can contain the same card multiple times. This might be fine for your game but is not how most card games work (if they do contain a card multiple times, this number is at least fixed).