Python Bingo game that stores card in a dictionary

Question

I am super proud of this python bingo game I wrote. I am posting it here to make get some help reviewing it to make sure I'm using the best practices. This is for a class, and my professor is a stickler for using best practices. I went above and beyond what the assignment called for so I need a little more helping making sure the code is okay. Any help is appreciated!

Here is the code:

# Author: Helana Brock
# Homework 2, problem 3

import random

random_draw_list = random.sample(range(1, 76), 75)

def generate_card():
    """
    Generates a bingo card and stores the numbers in a dictionary.
    """
    card = {
        "B": [],
        "I": [],
        "N": [],
        "G": [],
        "O": [],
    }
    min = 1
    max = 15
    for letter in card:
        card[letter] = random.sample(range(min, max), 5)
        min += 15
        max += 15
        if letter == "N":
            card[letter][2] = "X" # free space!
    return card

def print_card(card):
    """
    Prints the bingo card.

    Arguments:
        card (dictionary): The card to be printed out.
    """
    for letter in card:
        print(letter, end="\t")
        for number in card[letter]:
            print(number, end="\t")
        print("\n")
    print("\n")

def draw(card, list):
    """
    Pops a number off a list of random numbers. Using the pop method ensures no duplicate
    numbers will be drawn.

    Arguments:
        card (dictionary): The card to to check for the number that was drawn.
        list (list): The list of random numbers to be drawn from.
    """
    number_drawn = random_draw_list.pop()
    for letter in card:
        i = 0
        for number in card[letter]:
            if number == number_drawn:
                card[letter][i] = "X"
            i += 1
    return number_drawn

def check_win(card):
    """
    First checks for diagonal wins, then four-corner, then horizontal, and finally, vertical.

    Arguments:
        card (dictionary): The card to check for a win.
    """
    win = False
    if card["B"][0] == "X" and card["I"][1] == "X" and card["N"][2] == "X" and card["G"][3] == "X" and card["O"][4] == "X":
        win = True
    elif card["O"][0] == "X" and card["G"][1] == "X" and card["N"][2] == "X" and card["I"][3] == "X" and card["B"][4] == "X":
        win = True
    elif card["B"][0] == "X" and card["O"][4] == "X" and card["B"][4] == "X" and card["O"][0] == "X":
        win = True
    for letter in card:
        if(len(set(card[letter]))==1):
            win = True
    for i in range(5):
        cnt = 0
        for letter in card:
            if card[letter][i] == "X":
                cnt += 1
        print(cnt)
        if cnt == 5:
            win = True
            break
    return win

def main():
    """
    Main method for the program. Generates a card, prints it, and loops through drawing
    and printing until the check_win method returns True or the user enters "quit".
    """
    print("Let's play bingo!")
    card = generate_card()

    print("\nHere is your card:\n")
    print_card(card)

    print("\nKeep pressing enter to continue or type quit to exit.\n")
    user_input = input()
    win = check_win(card)
    balls_till_win = 0

    while win == False and user_input != "quit":
        number_drawn = draw(card, random_draw_list)
        balls_till_win += 1

        print(f"\nNumber drawn: {number_drawn}.")
        print(f"Total balls drawn: {balls_till_win}.\n")
        print_card(card)

        win = check_win(card)

        user_input = input()
    if win == True:
        print(f"\nBingo! Total balls to win: {balls_till_win}.")
    else:
        print("Goodbye! Thanks for playing!")

main()

Answer 1

Style

Python comes with an "official" Style Guide for Python Code (aka PEP 8) which is a widely accepted standard for naming conventions and the like. Since you say your professor is keen on using best practices, I'd recommend to have a look at those guidelines. There is also a variety of tools that can help you to check for those best practices automatically. A non-exhaustive list (including Pylint) can be found in this meta-post here on Code Review.

From what I have seen, your code looks quite good in that regard. I especially like your docstrings. The most prevalent issue in your code would be that there are usually two blank lines between different function definitions in order to make their separation more clear.

Code

Idiomatic Python

As @nz_21 already mentioned, if sth == True: and also if sth == False: can be expressed in a more idiomatic way as if sth: and if not sth: (Note: Pylint would also catch this particular "mistake".)

Redefining built-ins

Some of the variable names you have chosen (min, max, list) redefine built-in functions from the Python core. You should avoid that. The simplest way is to append a single trailing underscore, e.g. list_, in order to avoid that issue. Also the parameter list seems to be unused in draw(...).

Random number generation

card[letter] = random.sample(range(min_, max_), 5) could also be rewritten as card[letter] = [random.randrange(min_, max_) for _ in range(5)]. Although I don't think that it will make a big difference in your case. The documentation explicitly list them as alternatives:

random.randrange(start, stop[, step])

Return a randomly selected element from range(start, stop, step). This is equivalent to choice(range(start, stop, step)), but doesn’t actually build a range object.

From that it seems that the later approach would make more sense for a large range of values to choose from.

Edit: removed after hint in the comments with regard to sampling with and without replacement. Thanks to @Wombatz.

Global variables

random_draw_list is used as a global variable, but unfortunately not as a constant as one might expect, but as a mutable global variable. That makes it harder to reuse code. Since draw(...) is the only function using this variable, it makes no sense to have it as a global variable. Instead, it should defined in main() after you have changed draw(...) to accept it as parameter (maybe that's what list was supposed to do in the first place?) and passed when calling draw(...) from main().

Top-level script environment

In order to show what part of the file is actually supposed to be run as a script, Python defines the concept of a top-level script environment. Although the name sounds daunting at first, it's quite likely that you have seen it in someone else's Python code: if __name__ == "__main__": This tells the Python interpreter to only run that part of code if the file is used as a script. In your case all there is to do is to do add the above right before calling the main-function:

if __name__ == "__main__":
    main()

That also helps you to avoid the issue that Python always starts a new bingo game whenever you try to import a function from your file ;-)

Answer 2

Initializing your bingo card

You don't need to initialize your dictionary with lists, since random.sample returns a list by default. Just iterate through the string "BINGO" to set your keys. Also, the if check doesn't need to happen, you know that you will be replacing the 3rd element of the list at the key 'N' every time, so just have that be an instruction at the end:

def generate_card():
    """
    Generates a bingo card and stores the numbers in a dictionary.
    """
    # just start with a plain dictionary, random.sample
    # returns a new list, so no need to pre-allocate lists
    # you won't be using in the future
    card = {}

    # min and max are builtin functions, so use
    # underscores to avoid name shadowing
    _min = 1
    _max = 15

    for letter in 'BINGO':
        card[letter] = random.sample(range(_min, _max), 5)
        _min += 15
        _max += 15

    # You know which letter this needs to be, no need for the if block
    card["N"][2] = "X" # free space!
    return card

Checking the win condition

To check over the diagonals, you can use zip for your keys and the indices to validate. For horizontals iterate over card.values() and check all(item == 'X' for item in row). For columns, you can zip together the rows using argument unpacking.

By iterating over a dictionary, it returns the keys by default, which is why enumerate(dict) is the desired structure here. To check the opposite direction, zip(reversed(range(5)), card) will give you (idx, key) pairs in opposite order in a pythonic way, since dict cannot be reversed:

# down to the right
# iterating over the dictionary yields the keys
if all(card[k][idx]=='X' for idx, key in enumerate(card)):
   print('win!')
   return True

# up to the right
elif all(card[k][idx]=='X' for idx, key in zip(reversed(range(5)), card)):
   print('win!')
   return True

# horizontal condition
for row in card.values():
    if all(item=='X' for item in row):
        return True

# vertical condition
for column in zip(*card.values()):
    if all(item=='X' for item in column):
        return True

To show how this works:

import random

d = {k: random.sample(range(10), 3) for k in 'abc'}
# {'a': [5, 3, 7], 'b': [1, 8, 7], 'c': [4, 5, 8]}

# diagonal
for idx, key in enumerate(d):
    print(key, d[key][idx])
a 5
b 8
c 8


# opposite diagonal
for idx, key in zip(reversed(range(3)), d):
    print(key, d[key][idx])
a 7
b 8
c 4


# rows
for row in d.values():
    print(row)

[5, 3, 7]
[1, 8, 7]
[4, 5, 8]

for col in zip(*d.values()):
    print(col)

(5, 1, 4)
(3, 8, 5)
(7, 7, 8)

Also, the return True on all of the conditions means that the function will stop once a winning condition is hit, rather than going through all of the conditions when you really don't need to. This is called short-circuiting, meaning that you only evaluate up to the desired condition.

Looking at this a bit further, I'd probably refactor the row and column check into a different function:

def check_line(values):
    """
    values is an iterable over either rows or columns
    of the card. Should be iterable(str)

    returns boolean
    """
    for line in values:
        if all(val=='X' for val in values):
            return True

# the last two loops then look like:
elif check_line(card.values()):
    return True

elif check_line(zip(*card.values())):
    return True

return False

Class?

The fact that you're passing card around to a lot of functions says (to me) that you could probably use a class here. The generate_card method can be done on __init__, and everything else takes a self rather than a card:

class BingoCard:

    def __init__(self, _min=1, _max=15):
        """
        Generates a bingo card and stores the numbers in a dictionary.
        _min and _max are integers that default to 1 and 15
        if a default game is desired
        """

        # just start with a plain dictionary, random.sample
        # returns a new list, so no need to pre-allocate lists
        # you won't be using in the future
        self.card = {}

        for letter in 'BINGO':
            self.card[letter] = random.sample(range(_min, _max), 5)
            _min += 15
            _max += 15

        # You know which letter this needs to be, no need for the if block
        self.card["N"][2] = "X" # free space!
        # __init__ doesn't return anything

    # this makes your card printable
    # and requires you return a string
    def __str__(self):
        return '\n'.join('\t'.join((letter, *map(str, values))) for letter, values in self.card.items())


    def draw(self, random_draw_list):
        """
        Pops a number off a list of random numbers. Using the pop method ensures no duplicate
        numbers will be drawn.

        Arguments:
            self (dictionary): The card to to check for the number that was drawn.
            list (list): The list of random numbers to be drawn from.
        """
        number_drawn = random_draw_list.pop()
        for letter in self.card:
            # don't track an index here, use enumerate
            for i, number in enumerate(self.card[letter]):
                if number == number_drawn:
                    self.card[letter][i] = "X"
        return number_drawn


    def check_win(self):
        # down to the right
        if all(self.card[k][idx]=='X' for idx, key in enumerate(self.card)):
           print('win!')
           return True

        # up to the right
        elif all(self.card[k][idx]=='X' for idx, key in zip(reversed(range(5)), self.card)):
           print('win!')
           return True

        # horizontal condition
        elif self.check_line(self.card.values()):
            return True

        # vertical condition
        elif self.check_line(zip(*self.card.values())):
            return True

        return False


    @staticmethod
    def check_line(values):
        """
        values is an iterable over either rows or columns
        of the card. Should be iterable(str)

        returns boolean
        """
        for line in values:
            if all(val=='X' for val in values):
                return True



# then, your bingo card is an instance of this class

card = BingoCard()

card.check_win()
# False

The __str__ function is outlined in the python data model, but the gist of it is that __str__ allows you to redefine what the informal string representation of an object looks like.

random_draw_list

To make this work, I'd add it as a condition on your while loop. You pop elements off of it, but what happens when there's nothing else to pop? You will get an IndexError for attempting to pop from an empty list. To make the game re-playable, I'd have the user choose to re-play, and generate that new list on each play, just as you re-create the bingo card on each play:

def generate_random_list():
    return random.sample(range(1, 76), 75)


def main():
    card = BingoCard()
    # generate the draw list here, on each play of the game
    # that way if you run out of balls, you can handle that by restarting
    # the game
    random_draw_list = generate_random_list()

    print(f"This is your card:\n {card}")

    # you don't need the user input here, I think it might
    # be better to include it elsewhere, before generating the
    # card. Instead, keep going while there are numbers to draw
    while random_draw_list:
        number_drawn = card.draw(random_draw_list)
        # this variable is better named balls_drawn
        balls_drawn += 1

        print(f"You drew {number_drawn}")
        print(f"Total balls drawn: {balls_drawn}")

        if check_win(card):
            print(f"You won after drawing {balls_drawn} balls!")
            break

        # if you were to check for user input during the game,
        # it would make more sense to do that after you've at least
        # checked one round
        keep_playing = input("Keep playing? (y/n)").strip().lower()
        if keep_playing == 'n':
            print("Ok, thanks for playing")
            break
    else:
        print("Sorry, there are no more balls to draw")


# This is where you prompt the user to play:
if __name__ == "__main__":
    while True:
        play = input("Would you like to play bingo? (y/n)")
        if play.strip().lower() == 'y':
            main()
        else:
            break

Answer 3

Nice job. Here's two comments to add to what other's have said.

range() excludes the stop point

The built-in function range(start, stop, step) does not include stop in the items it returns. So in generate_card():

min_ = 1
max_ = 15
...
random.sample(range(min_, max_), 5)

only selects from the numbers 1, 2, 3, ..., 14. max_ should be 16. I sometimes write code with an explicit +1 like this:

 random.sample(range(min_, max_+1), 5)

to remind myself that I intended to include max_ in the range.

Separation of concerns

When a function in a program does multiple unrelated things, it may make the program more difficult to debug, modify, or enhance. For example, draw() gets the next numbered ball and adds it to the BINGO card. If a the player could have multiple bingo cards, each card would be drawing it's own random balls. Clearly, that wouldn't work. It would be better if the ball was drawn separately and then checked against each card.

Answer 4

Some low-hanging fruit:

if win == True 

can be written as

if win:

Know that print delimits by a newline by default; you don't need to explicitly add \n unless you want two newlines.

You can use classes to make card a class on its own.

Answer 5

Tried the BINGO game. Encountered a few errors. One of them is an IndexError in the random.pop() function after 26 draws.

<ipython-input-1-16d606983ecb> in draw(card, list)
     53         list (list): The list of random numbers to be drawn from.
     54     """
---> 55     number_drawn = random_draw_list.pop()
     56     for letter in card:
     57         i = 0

IndexError: pop from empty list