5
\$\begingroup\$

I have deliberately avoided any OOP for this project. I'm fairly happy with my implementation of the classic strategy game "Chomp". The computer just uses a random valid choice, so no AI.

I would appreciate feedback on points of style and correctness. Is the code basically OK? Does it seem to have a coherent style? What improvements would you make?

Thanks in advance.

"""
Chomp - a strategy game
"""

import random
import time

NUM_ROWS = 5
NUM_COLS = 6


def print_title():
    print(r"""
 ______     __  __     ______     __    __     ______  
/\  ___\   /\ \_\ \   /\  __ \   /\ "-./  \   /\  == \ 
\ \ \____  \ \  __ \  \ \ \/\ \  \ \ \-./\ \  \ \  _-/ 
 \ \_____\  \ \_\ \_\  \ \_____\  \ \_\ \ \_\  \ \_\   
  \/_____/   \/_/\/_/   \/_____/   \/_/  \/_/   \/_/   
""")


def print_instructions():
    print("Welcome to Chomp. Choose a square and all squares to the right")
    print("and downwards will be eaten. The computer will do the same.")
    print("The one to eat the poison square loses. Good luck!")
    print()


def who_goes_first():
    if random.randint(0, 1) == 0:
        return "computer"
    else:
        return "human"


def play_again():
    print("Would you like to play again? (yes or no)")
    return input().lower().startswith("y")


def print_matrix(matrix):
    for row in matrix:
        for elem in row:
            print(elem, end=" ")
        print()


def validate_user_input(player_choice, board):
    try:
        row, col = player_choice.split()
    except ValueError:
        print("Bad input: The input should be exactly two numbers separated by a space.")
        return False
    try:
        row = int(row)
        col = int(col)
    except ValueError:
        print("Input must be two numbers, however non-digit characters were received.")
        return False

    if row < 0 or row > NUM_ROWS - 1:
        print(f"The first number must be between 0 and {NUM_ROWS - 1} but {row} was passed.")
        return False
    if col < 0 or col > NUM_COLS - 1:
        print(f"The second number must be between 0 and {NUM_COLS - 1} but {col} was passed.")
        return False
    if board[row][col] == " ":
        print("That square has already been eaten!")
        return False
    return True


def update_board(board, row, col):
    for i in range(row, len(board)):
        for j in range(col, len(board[i])):
            board[i][j] = " "


def get_human_move(board):
    valid_input = False
    while not valid_input:
        player_choice = input("Enter the row and column of your choice separated by a space: ")
        valid_input = validate_user_input(player_choice, board)
    row, col = player_choice.split()
    return int(row), int(col)


def get_computer_move(board):
    valid_move = False
    while not valid_move:
        row = random.randint(0, NUM_ROWS - 1)
        col = random.randint(0, NUM_COLS - 1)
        if board[row][col] == " ":
            continue
        else:
            valid_move = True
    return row, col


def main():
    board = []
    for i in range(NUM_ROWS):
        row = []
        for j in range(NUM_COLS):
            row.append("#")
        board.append(row)

    board[0][0] = "P"
    game_is_playing = True
    turn = "human"

    print_title()
    print_instructions()

    while game_is_playing:
        if turn == "human":
            # Human turn
            print("Human turn.")
            print()
            print_matrix(board)
            print()
            row, col = get_human_move(board)
            if board[row][col] == "P":
                print()
                print("Too bad, the computer wins!")
                game_is_playing = False
            else:
                update_board(board, row, col)
                print()
                print_matrix(board)
                print()
                turn = "computer"
                time.sleep(1)
        else:
            # Computer turn
            row, col = get_computer_move(board)
            print(f"Computer turn. the computer chooses ({row}, {col})")
            print()
            if board[row][col] == "P":
                print()
                print("Yay, you win!")
                game_is_playing = False
            else:
                update_board(board, row, col)
                print_matrix(board)
                print()
                turn = "human"

    if play_again():
        main()
    else:
        print("Goodbye!")
        raise SystemExit


main()
\$\endgroup\$
4
\$\begingroup\$

Your game has the same kind of flaw like a lot of the other games that have recently been here on Code Review: an unnecessary recursion in the main game flow:

What do I mean by that? Let's look at your main function:

def main(): 

    # ... all of the actual game here ...

    if play_again():
        main()
    else:
        print("Goodbye!")
        raise SystemExit

As one can clearly see main will call main every time the player chooses to play another round, leading to deeper and deeper recursion. If someone would be really obsessed with your game and would try to play more than sys.getrecursionlimit() games (here on my machine it's 3000), there would be a RuntimeError.

Fortunately this can easily be fixed using a simple while loop:

def main():
    while True:

        # ... all of the actual game here ...

        if not play_again():
            print("Goodbye!")
            break

Recursion gone, welcome binge playing till your fingers bleed.


Now that we have that sorted out, let's look at some of the details of your code.

print_*

Not much to critique here, but I would like to introduce you to textwrap.dedent. textwrap.dedent allows you to indent text blocks as you would do with code and then takes care to remove the additional leading whitespace.

from textwrap import dedent


def print_title():
    print(
        dedent(r"""
             ______     __  __     ______     __    __     ______  
            /\  ___\   /\ \_\ \   /\  __ \   /\ "-./  \   /\  == \ 
            \ \ \____  \ \  __ \  \ \ \/\ \  \ \ \-./\ \  \ \  _-/ 
             \ \_____\  \ \_\ \_\  \ \_____\  \ \_\ \ \_\  \ \_\   
              \/_____/   \/_/\/_/   \/_____/   \/_/  \/_/   \/_/   
        """)
    )

But it basically boils down to personal taste which version you prefer. The same logic could be applied to print_instructions to only need a single call to print.

who_goes_first

who_goes_first could be simplified a little bit:

def who_goes_first():
    return random.choice(("computer", "human"))

Or even be left out? You current code does not use it.

print_matrix

Again, the code could be simplified a little bit, e.g. using str.join:

def print_matrix(matrix):
    for row in matrix:
        print(" ".join(row))

From what you can actually see, there should be no difference to the previous output, but there is actually no trailig whitespace in this version. You could even go a little bit futher than this using a list comprehension:

def print_matrix(matrix):
    print("\n".join(" ".join(row) for row in matrix))

validate_user_input / get_human_move

validate_user_input does a good job at validating the given user input, but keeps the results of the parsing to itself. That leads to duplicate code in get_human_move. With a little bit of rewriting that duplication can be removed:

def validate_user_input(player_choice, board):
    try:
        row, col = player_choice.split()
    except ValueError:
        raise ValueError(
            "Bad input: The input should be exactly two numbers separated by a space."
        )

    try:
        row = int(row)
        col = int(col)
    except ValueError:
        raise ValueError(
            "Input must be two numbers, however non-digit characters were received."
        )

    if row < 0 or row > NUM_ROWS - 1:
        raise ValueError(
            f"The first number must be between 0 and {NUM_ROWS - 1} but {row} was passed."
        )

    if col < 0 or col > NUM_COLS - 1:
        raise ValueError(
            f"The second number must be between 0 and {NUM_COLS - 1} but {col} was passed."
        )

    if board[row][col] == " ":
        raise ValueError("That square has already been eaten!")

    return row, col


def get_human_move(board):
    while True:
        player_choice = input("Enter the row and column of your choice separated by a space: ")
        try:
            row, col = validate_user_input(player_choice, board)
            break
        except ValueError as ex:
            print(ex)
    return row, col

So what has happened here?

  1. validate_user_input now does not print the error message itself, but raises an informative exception instead.
  2. if no reason to raise an exception has occured, validate_user_input now returns row and col, to they do not need to be recomputed in get_human_move
  3. get_human_move was adapted to that change and now tries to get the validated user input, prints the reason if that fails, and asks the user to try again.

Of course, raising exceptions is just one way to do this. There are many other ways that lead to a similar structure.

If you decide to implement these changes, parse_user_input is maybe a more appropiate name now, given its new capabilites.

You might want think about passing NUM_ROWS/NUM_COLS as parameters or determine it from board to cut down on global variables as well.

Quick sidenote: 0-based indexing might be unfamiliar to people that have no programming backgroud (or use MatLab all the time ;-)), so maybe allow the user to enter 1-based indices and substract 1 before validation.

update_board

Depending on your choice on the previous point, this is either the way to go, or you should use NUM_ROWS/NUM_COLS here too to be consistent.

get_computer_move

Maybe you shoud add some simple heuristics to make your computer a little bit smarter, e.g. it sometimes chooses to take the poison even if it still has alternatives, or even the possibilty to win. Also (theoretically), it is be possible that this random sampling algorithm never finds a valid solution ;-) To avoid that, generate a list of valid rows and values, and pick a sample from these lists.

If you stick to the current approach, you can at least get rid of the "support variable" valid_move:

def get_computer_move(board):
    while True:
        row = random.randint(0, NUM_ROWS - 1)
        col = random.randint(0, NUM_COLS - 1)
        if board[row][col] == EMPTY_SPOT:
            continue
        else:
            break
    return row, col

main

main already had some structural remarks, so let's look at its code a little bit:

This

board = []
for i in range(NUM_ROWS):
    row = []
    for j in range(NUM_COLS):
        row.append("#")
    board.append(row)

can be recuded to a single nested list comprehension:

board = [["#" for _ in range(NUM_COLS)] for _ in range(NUM_ROWS)]

Depending on your choice regarding the global variables, NUM_ROWS/NUM_COLS would need to be removed here to. Maybe even allow them to be set as command line arguments by the user?

The rest of the code has some "magic values" (they are not so magic in your case), e.g. board[0][0] = "P", turn = "computer", and turn = "human" (there was also " " earlier to mark empty spots). The problem with those "magic values" is that your IDE has no chance to help you spot errors. You did write "p" instead of "P" and now the game does weird things? Too bad. You will have to find that bug yourself! The way to go about this would be to use global level constants, because this is what globals are actually good for, or an enum if you have several distint values like human and computer.

This is a possible way to do it (with a sketch of how main would look like):

FILLED_SPOT = "#"
POISON_SPOT = "P"
EMPTY_SPOT = " "


class Actor(enum.Enum):
    HUMAN = "human"
    COMPUTER = "computer"

# ... lot of code here ...

def main():
    while True:
        board = [[FILLED_SPOT for _ in range(NUM_COLS)] for _ in range(NUM_ROWS)]

        board[0][0] = POISON_SPOT
        turn = Actor.HUMAN  # or: who_goes_first()

        # ...

        while game_is_playing:
            if turn == Actor.HUMAN:
                # Human turn
                # ...
                if board[row][col] == POISON_SPOT:
                    # ...
                else:
                    # ...
                    turn = Actor.COMPUTER
            else:
                # Computer turn
                # ...
                if board[row][col] == POISON_SPOT:
                    # ...
                else:
                    # ...
                    turn = Actor.HUMAN

        if not play_again():
            print("Goodbye!")
            break


I chose this to showcase both variants. In your case to could also just use either of those, no need to have both of them in the game.


All of this may sound harsh, but it really isn't meant that way! Your code is actually quite enjoyable to review. Your style is quite consistent, the names are good and the general structure is clear.

Keep up the good work!

\$\endgroup\$
  • \$\begingroup\$ Thanks @AlexV That's exactly the kind of feedback I was looking for. One thing that confuses me though - I hear strong opinions about not using while True - that the condition should be made explicit, and yet it seems like a useful construct to me, and you are recommending it here. I'm guessing other reviewers would say to change it. Is it just a matter of taste? \$\endgroup\$ – Robin Jul 13 at 7:30
  • \$\begingroup\$ I don't have a strong opinion on that. But I think I prefer the while True: ... break approach for simple cases like here, simply because that helps me not to have to find another meaningful name for a variable. If you have to check multiple conditions, or the inner loop would also need to break the outer loop, the explicit approach is the way to go I think. \$\endgroup\$ – AlexV Jul 13 at 7:44
  • \$\begingroup\$ I've just been revisiting this code, and noticed something weird with global variables. For example in update_board I pass board as a parameter, and update it inside the function, but don't return the updated board, yet it still gets updated. So it appears I'm basically using a global value for board. Should I remove the board parameter and use global board instead, to be at least coherent? I know globals are discouraged, but it's maybe at least better to be explicit about them. \$\endgroup\$ – Robin Sep 30 at 8:40
  • 1
    \$\begingroup\$ @Robin: Python has mutable (e.g. list, dict) and immutable types/objects (e.g. int, string, tuple) types. board is a list of list and therefore mutable, so you can modify it directly without returning the modified version. I don't see how a global variable would make this more obvious. \$\endgroup\$ – AlexV Sep 30 at 9:15

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.