3
\$\begingroup\$

I have been working on a 2-player battleship game and have finally got it working, bug free.

I'm posting this here to ask you what you think of it. Are there any changes that could be made to improve the performance/general layout of the code?

If you come across any issues with the code itself, (maybe a bug yet to be found) then I'd be grateful if you could let me know.

The game is a fairly simple console-based battleship game, where the players take it in turns to sink a mutual ship that is defined by an x and y coordinate on a 5x5 game board.

from random import randint

game_board = []

player_one = {
    "name": "Player 1",
    "wins": 0,
}

player_two = {
    "name": "Player 2",
    "wins": 0,
}

colors = {
    "reset":"\033[00m",
    "red":"\033[91m",
    "blue":"\033[94m",
    "cyan":"\033[96m",
}

# Building our 5 x 5 board
def build_game_board(board):
    for item in range(5):
        board.append(["O"] * 5)

def show_board(board):
    for row in board:
        print(" ".join(row))

# Defining ships locations
def load_game(board):
    print("WELCOME TO BATTLESHIP!")
    print("Find and sink the ship!")
    del board[:]
    build_game_board(board)
    print(colors['blue'])
    show_board(board)
    print(colors['reset'])
    ship_col = randint(1, len(board))
    ship_row = randint(1, len(board[0]))
    return {
        'ship_col': ship_col,
        'ship_row': ship_row,
    }

ship_points = load_game(game_board)

# Players will alternate turns.
def player_turns(total_turns):

    if total_turns % 2 == 0:
        total_turns += 1
        return player_one

    return player_two

# Allows new game to start
def play_again():

    positive = ["yes", "y"] 
    negative = ["no", "n"]

    global ship_points

    while True:
        answer = input("Play again? [Y(es) / N(o)]: ").lower().strip()
        if answer in positive:
            ship_points = load_game(game_board)
            main()
            break

        elif answer in negative:   
            print("Thanks for playing!")
            exit()

# What will be done with players guesses
def input_check(ship_row, ship_col, player, board):
    guess_col = 0
    guess_row = 0
    while True:

        try:
            guess_row = int(input("Guess Row:")) - 1
            guess_col = int(input("Guess Col:")) - 1
        except ValueError:

            print("Enter a number only: ")
            continue
        else:

            break
    match = guess_row == ship_row - 1 and guess_col == ship_col - 1
    not_on_game_board = (guess_row < 0 or guess_row > 4) or (guess_col < 0 or guess_col > 4)

    if match:
        player["wins"] += 1
        print("Congratulations! You sunk my battleship!")
        print('The current match score is %d : %d (Player1 : Player2)' % (player_one["wins"], player_two["wins"]))
        print("Thanks for playing!")
        play_again()

    elif not match:
        if not_on_game_board:
            print("Oops, that's not even in the ocean.")

        elif board[guess_row][guess_col] == "X" or board[guess_row][guess_col] == "Y":
            print("You guessed that one already.")

        else:
            print("You missed my battleship!")
            if player == player_one:
                board[guess_row][guess_col] = "X"
            else:
                board[guess_row][guess_col] = "Y"

        print(colors['cyan'])
        show_board(game_board)
        print(colors['reset'])

    else:
        return 0


begin = input("Type 'start' to begin: ")

while (begin != str('start')):
    begin = input("Type 'start' to begin: ")

def main():

    for turns in range(6):

        if player_turns(turns) == player_one:
            print(ship_points)
            print("Player One")
            input_check(
                ship_points['ship_row'],
                ship_points['ship_col'],
                player_one, game_board
            )

        elif player_turns(turns) == player_two:
            print("Player Two")
            input_check(
                ship_points['ship_row'],
                ship_points['ship_col'],
                player_two, game_board
            )

        if turns == 5:
            print("This game is a draw.")
            print(colors['red'])
            show_board(game_board)
            print(colors['reset'])
            print('The current match score is %d : %d (Player1 : Player2)' % (player_one["wins"], player_two["wins"]))
            play_again()

if __name__ == "__main__":
    main()
\$\endgroup\$
4
\$\begingroup\$

This one's common it seems-

Whitespace

Python relies on whitespace and proper indentation to be readable; both for the developer(s) and people from all over the world who view the code. You should try to add whitespace to seperate logical pieces of code from one another. Especially your load_game() function needs this. I've rewritten it so it's more readable:

def load_game(board):
    del board[:]
    build_game_board(board)

    print(colors['blue'])
    show_board(board)
    print(colors['reset'])

    ship_col = randint(1, len(board))
    ship_row = randint(1, len(board[0]))

    return {
                'ship_col': ship_col,
                'ship_row': ship_row,
              }

main() function and input / output

In the above example code, you may have noticed I got rid of the program's output. The reason I did this is because in general, it's easier to keep most in- / output in one part of the code (main()). This way, it's easier to use ~switch-case~ statements and to efficiently evaluate user input.

Bad code:

def some_function():
    some_input = input()
        some_result = evaluate(some_input)
        if some_result:
            some_other_result = do_this()
            if some_other_result:
                 do_this_again()
        else:
            do_that()

Slightly better code:

def some_function(result):

    if evaluate(result):
        do_this()

    else:
        do_that()

def main():
    print("Some welcome message.")
    evaluate_me = input()

    some_function(result)

input_check()

There's a couple of things that are off:

  • The name of the function is too generic. Use something more meaningful, so you don't have to explain it with comments.

  • Your code is not seperated into logical parts; whitespace is off:

    guess_col = 0
    guess_row = 0
    while True:
    
         try:
            guess_row = int(input("Guess Row:")) - 1
            guess_col = int(input("Guess Col:")) - 1
        except ValueError:
    
             print("Enter a number only: ")
             continue
         else:
    
             break
    

Better code:

guess_col = 0
guess_row = 0

while True:
    try:
        guess_row = int(input("Guess Row:")) - 1
        guess_col = int(input("Guess Col:")) - 1
        break     # break will run here if no ValueError gets raised
    except ValueError:
        print("Enter a number only.")
  • Your else: statement (see above) will silently let an error escape if it ever gets to run (which, with the current setup, seems unlikely). Why not get rid of the statement altogether?

  • You don't need to use continue, because the indented code will only run if a ValueError occurs, and if that does happen, the else: statement would not run (else only runs if any other condition is False, not if any condition is True).

  • You made the same mistake of using an else: return 0 statement at the end of the function, even though there is no way this would run. You could change the if / elif / else to an if / else statement:

    if match: # do stuff

    else: # do other stuff

  • return 0 is confusing. If you need to return none, use return None (wasn't that easy?). return 0 may be understood as an exit code, in which case you should use sys.exit(0) (https://stackoverflow.com/questions/23547284/exit-0-vs-return-0-python-preference). You could also just use return or return False, if any.

General

Just above the line where you declare main():

begin = input("Type 'start' to begin: ")

while (begin != str('start')):
    begin = input("Type 'start' to begin: ")

Simplify this:

while True:
    start_game = input("Type 'start' to begin: ")
    if start_game == "start": 
    # No need for parentheses;
    # "start" is already str(). No need to convert.
        break

Some of your code is right on the edge of the 79 characters border (one line is 116 characters long). Look into shortening your lines to improve readability.

\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks for the help. One question though, my else: break statement in my code is needed. If I were to remove that, then the code wouldn't work would it? I tried getting rid of it, and program kept prompting me for an input for the row and column, even though I put a valid value. \$\endgroup\$ – zalidael Jun 2 '17 at 11:35
  • \$\begingroup\$ Changed my answer. \$\endgroup\$ – Daniel Jun 2 '17 at 13:45

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.