14
\$\begingroup\$

I have just finished learning basic-intermediate Python subjects and wanted to test myself. This Battleship game is the third game i have written.

import random


def create_random_ship():
    return random.randint(0, 5), random.randint(0, 5)


def play_again():
    try_again = input("Wanna play again? <Y>es or <N>o? >: ").lower()
    if try_again == "y":
        play_game()
    else:
        print("Goodbye!")
        return


print("Welcome to the Battleship game!"
      "\nYour main objective is to find and destroy all the hidden ships on map!\n")

print("""\nIntroductions:
\nYou have 10 ammo and there are 3 hidden ships on map.
In order to hit them, you have to enter specific numbers for that location. For example:
For the first row and first column, you have to write 1 and 1.
I wish you good fortune in wars to come!\n""")


def play_game():
    game_board = [["O", "O", "O", "O", "O"],
                  ["O", "O", "O", "O", "O"],
                  ["O", "O", "O", "O", "O"],
                  ["O", "O", "O", "O", "O"],
                  ["O", "O", "O", "O", "O"]]

    for i in game_board:
        print(*i)

    ship1 = create_random_ship()
    ship2 = create_random_ship()
    ship3 = create_random_ship()
    ships_left = 3
    ammo = 10

    while ammo:
        try:
            row = int(input("Enter a row number between 1-5 >: "))
            column = int(input("Enter a column number between 1-5 >: "))
        except ValueError:
            print("Only enter number!")
            continue

        if row not in range(1,6) or column not in range(1, 6):
            print("\nThe numbers must be between 1-5!")
            continue

        row = row - 1 # Reducing number to desired index.
        column = column - 1 # Reducing number to desired index.

        if game_board[row][column] == "-" or game_board[row][column] == "X":
            print("\nYou have already shoot that place!\n")
            continue
        elif (row, column) == ship1 or (row, column) == ship2 or (row, column) == ship3:
            print("\nBoom! You hit! A ship has exploded! You were granted a new ammo!\n")
            game_board[row][column] = "X"
            ships_left -= 1
            if ships_left == 0:
                print("My my, i didn't know you were a sharpshooter! Congratz, you won!")
                play_again()
        else:
            print("\nYou missed!\n")
            game_board[row][column] = "-"
            ammo -= 1

        for i in game_board:
            print(*i)

        print(f"Ammo left: {ammo} | Ships left: {ships_left}")

    play_again()


if __name__ == "__main__":
    play_game()

What do you think? How can I make it better? What are my mistakes?

\$\endgroup\$
5
  • 1
    \$\begingroup\$ "I know that there is a bug possiblity ..." brings your question dangerously close to off-topic waters. This might lead to your question being closed until you have fixed the bug. \$\endgroup\$
    – AlexV
    Nov 7, 2019 at 14:11
  • \$\begingroup\$ for start, I think it's not a good practice to have \n inside a triple-quoted string. \$\endgroup\$
    – Ron Klein
    Nov 7, 2019 at 14:53
  • 2
    \$\begingroup\$ @RonKlein Feel free to point that out in an answer, with suggestions on how to do that more proper. Small answers are still valid answers. \$\endgroup\$
    – Mast
    Nov 8, 2019 at 10:21
  • 2
    \$\begingroup\$ Please do not change the code of your question after it has been answered. This invalidates the answers. Please read How to Answer for more details. \$\endgroup\$
    – Gloweye
    Nov 12, 2019 at 11:15
  • 1
    \$\begingroup\$ @Gloweye Sorry, i revert it back. \$\endgroup\$
    – Haliax
    Nov 12, 2019 at 11:21

5 Answers 5

19
\$\begingroup\$

Magic Numbers

Your biggest issue is too many magic numbers and hard-coded information.

If you added at the top of your script:

ROWS = 5
COLUMNS = 5

then you could have:

def create_random_ship():
    return random.randrange(ROWS), random.randrange(COLUMNS)

Game board can be created with:

game_board = [["O"] * COLUMNS for _ in range(ROWS)]

And you can ask for input from the user with:

row = int(input(f"Enter a row number between 1-{ROWS} >: "))

Along the same vein, you could have:

print(f"""\nIntroductions:
\nYou have {INIT_AMMO} ammo and there are {INIT_ENEMY} hidden ships on map.
...""")

so your welcome instructions can keep up-to-date with any changes to your game initial conditions.

Instructions

You have a if __name__ == "__main__": guard to prevent the code from running if imported into another script, but your instruction are unconditionally printed. You should move the instruction printing into an instructions() function, and call that only if appropriate.

Infinite Recursion

play_game() calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), ...

Traceback (most recent call last):
  [...omitted...]
RecursionError: maximum recursion depth exceeded

Don't do that. Much simpler is to have play_again() return a True/False result, and loop based on the return value:

def play_many_games():

    instructions()

    play_game()

    while play_again():
        play_game()

    print("Goodbye!")

if __name__ == "__main__":
    play_many_games()

No more recursion.

In the comments, @Graipher suggests an alternate method to avoid the recursion:

while True:
    play_game()
    if not play_again():
        break

Yet another alternative introduces a loop condition variable, which is pre-set before the loop:

 playing = True
 while playing:
     play_game()
     playing = play_again()

All three approaches avoid the infinite recursion.

I Saw Three Ships...

ship1 = create_random_ship()
ship2 = create_random_ship()
ship3 = create_random_ship()

# ...

    elif (row, column) == ship1 or (row, column) == ship2 or (row, column) == ship3:

What are you going to do if you change this to four ships, or even 5 ships?

With computers, there are 3 important numbers: zero, one and many. If you have more than one ship, you have "many" ships, and should put them in a container, such as a list():

ship_coordinates = []
for _ in range(ships_left):
    ship_coordinates.append(create_random_ship())

And when you want to see if a particular row/column matches any one of the ship coordinates in the container, you use the in function:

    elif (row, column) in ship_coordinates:

There are other container types you can use, such as a set() which has faster in performance. However, it cannot add the same value more than once, and using it would result in slightly different behaviour to what you currently have. You've mentioned that you have a bug where ships can have the same coordinates, and I am trying very hard not to accidentally fix that. (Code Review is for reviewing working code only, not help debugging, so I am tiptoeing around the bug, so that you can fix it yourself.)

\$\endgroup\$
7
  • \$\begingroup\$ Now that I've spend a while on Code Review, I gotta say that it's a lot more common than I expected that people try to start a new game or re-try input through "recursion." Quoted recursion because there's no actual intent to recurse. \$\endgroup\$
    – Gloweye
    Nov 8, 2019 at 10:13
  • 1
    \$\begingroup\$ Note that the code as given has a bug that wouldn't be an issue if it wasn't using magic numbers. The game board is 5 x 5 but the inputs are 1 through 6. \$\endgroup\$ Nov 8, 2019 at 12:10
  • 4
    \$\begingroup\$ I'm personally more a fan of while True: play_game(); if not play_again(): break \$\endgroup\$
    – Graipher
    Nov 8, 2019 at 12:45
  • 2
    \$\begingroup\$ @Graipher For a third variant playing = True; while playing: play_game(); playing = play_again() but it introduces an extra variable. \$\endgroup\$
    – AJNeufeld
    Nov 8, 2019 at 14:27
  • 1
    \$\begingroup\$ I prefer game(); while play_again(): game(). It's the least lines even if you have code duplication. I don't think it's worth polluting python with a do: game(); while play_again(), though. Even if that's semantically what you'd want. \$\endgroup\$
    – Gloweye
    Nov 12, 2019 at 11:03
4
\$\begingroup\$

How can i prevent that ships may have the same coordinates?

In other words, you want to pick three distinct locations on the board randomly; one for each ship. Sounds like you want a random sample (without replacement) of size three from the population of the game board coordinates:

from random import sample
from itertools import product

ROWS = 5
COLUMNS = 5
NUM_SHIPS = 3

# game_coordinates = [
#   (0, 0), (0, 1), (0, 2), (0, 3), (0, 4),
#   (1, 0), (1, 1), (1, 2), (1, 3), (1, 4),
#   (2, 0), (2, 1), (2, 2), (2, 3), (2, 4),
#   (3, 0), (3, 1), (3, 2), (3, 3), (3, 4),
#   (4, 0), (4, 1), (4, 2), (4, 3), (4, 4)
# ]
game_coordinates = list(product(range(ROWS), range(COLUMNS)))

ship1, ship2, ship3 = sample(game_coordinates, NUM_SHIPS)

Explanation of the above code:

First we generate a list of all the game coordinates using itertools.product to calculate the Cartesian product of the row and column indices.

Then using random.sample, we take a random sample (without replacement) of size three from game_coordinates to get the coordinates of the three ships.

EDIT: To expand on the above example (and to address @TemporalWolf's comment), I should clarify that creating the list of game coordinates only needs to happen once per program run.

Near the top of the program, you can declare game constants like so:

ROWS = 5
COLUMNS = 5
NUM_SHIPS = 3
GAME_COORDINATES = list(product(range(ROWS), range(COLUMNS)))

Then in play_game:

# set of ships the player must hit to win the game
ships = set(sample(GAME_COORDINATES, NUM_SHIPS))

# ...

# player hit a ship!
elif (row, column) in ships:
    # ...
    game_board[row][column] = "X"
    ships.remove((row, column))

    # player hit all the ships -- victory!
    if not ships:
        # print congratulatory message

The list of coordinates GAME_COORDINATES is created only once at the beginning of the program, and used (and re-used) in each call to play_game within the same program run.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ This is a really resource intensive way to do this, as it generates all possible outcomes then throws away all but three of them. For this program it's not a big deal, but for more complex boards this would bog down quickly. \$\endgroup\$ Nov 8, 2019 at 19:30
  • 1
    \$\begingroup\$ While it's true that the generation of the full list is costly, the generation of the list only need to happen once per program invocation. After it's generated, one can continue to randomly pick ships from the same list if the player wishes to play again during the same program run. \$\endgroup\$
    – Setris
    Nov 9, 2019 at 0:30
3
\$\begingroup\$

I'd say that

game_board = [["O", "O", "O", "O", "O"],
            ["O", "O", "O", "O", "O"],
            ["O", "O", "O", "O", "O"],
            ["O", "O", "O", "O", "O"],
            [ "O", "O", "O", "O", "O"]]

for i in game_board:
    print(*i)

produces a neat display:

O O O O O
O O O O O
O O O O O
O O O O O
O O O O O

which is an awesome way of translating array to display.

elif (row, column) == ship1

is also an intelligent way of coupling function with comparison where initialisation is class like:

ship1 = create_random_ship()

but, a ship class might be better

The Ship class

class Ship:
    def __init__(self):
        self.coord = (random.randint(0, 5), random.randint(0, 5))

    def __eq__(self, other):
        return self.coord == other

with initialisations as

ship1 = Ship()
ship2 = Ship()
ship3 = Ship()

This also helps if you want to add more info to your ships like let's say you want to make 3 teams

Messages Display

We might want to hold messages in a structure like

class Message:
    welcome = ("Welcome to the Battleship game!\n"
                "Your main objective is to find and destroy all the hidden ships on map!\n")
    instructions = ("\nIntroductions:\n"
                "You have 10 ammo and there are 3 hidden ships on map.\n"
                "In order to hit them, you have to enter specific numbers for that location. For example:\n"
                "For the first row and first column, you have to write 1 and 1.\n"
                "I wish you good fortune in wars to come!\n")
    # ...

and use as

print(Message.welcome)
print(Message.instructions)

Ship Initialisations

Ships could be better instantiated, maybe a function which modifies the game coordinates to represent the ship with a new symbol. Could be useful in the case where one ship occupies more than one tile

Game Class

A game class might be better suited with initialisations of the board size and number of random ships.

battle_ship = Game(board_size=(5,5), rand_ships=3)

Then you have methods as

battle_ship.play()
battle_ship.play_again()

Miscellaneous

  • +1 for use of f strings.
  • The game is also pretty smooth, maybe an Exception to catch KeyboardInterrupt might enhance it better.
  • I find it tiring to add row and col line by line, maybe a format like 1 5 might be used to specify it at once.
\$\endgroup\$
2
  • 3
    \$\begingroup\$ The init function for your Ship class could create multiple ships on the same coordinates. I would add a method to create multiple ships at once or to have the init method have the others ships as argument to ensure that there will be only one ship at one coordinate. \$\endgroup\$ Nov 8, 2019 at 11:25
  • \$\begingroup\$ @GáborFekete Yes a better alternative would be only a Ship class with the generate_ships method apart, either in a Game class or as a function \$\endgroup\$ Nov 8, 2019 at 13:26
2
\$\begingroup\$

First things first: random.randint() is inclusive of it's second term, so (0, 5) can generate 5 (this is different than range(), for example, which is exclusive of the 2nd term). If you want to maintain the similarity with range(), there is random.randrange() which acts like you expect. This is reflected in the docs:

random.randint(a, b):

Return a random integer N such that a <= N <= b. Alias for randrange(a, b+1).

There are multiple ways to deconflict ships, the simplest is to just regenerate when you detect a conflict:

import random

number_of_ships = 3
boardsize = 5

def generate_ships(number_of_ships, max_x, max_y):
    while True:
        ships = [(random.randint(0, max_x - 1), random.randint(0, max_y - 1)) for _ in range(number_of_ships)]
        if len(set(ship for ship in ships)) == number_of_ships:
            return ships  # return once we have a list of unique ships

ship_locations = generate_ships(number_of_ships, boardsize, boardsize)

In this case I just make a set of all ship coordinates and see if that set is the same length as the number of ships requested. If any share coordinates, the set will be short, so I regenerate the list.

I'm using a list comprehension, which is equivalent to:

ships = []
for _ in range(number_of_ships):
    ships.append((random.randint(0, max_x - 1), random.randint(0, max_y - 1)))

It's worth noting _ is not a special variable, but by convention shows I'm not using that value (I just want the code to repeat number_of_ships times.)

I ran a quick test and over 1 million attempts to generate ships there was a conflict about 13% of the time on a 5x5 board, which is generally consistent with the expected overlap rate for a 25 choose 3 problem. On an 8x8 that drops to 5%.

if game_board[row][column] == "-" or game_board[row][column] == "X":

elif (row, column) == ship1 or (row, column) == ship2 or (row, column) == ship3

These can both be simplified with in notation, which checks whether something is contained in a list, doubly so if you put the ships into a list like I showed above:

if game_board[row][column] in "-X":  # If board shows either a dash or X
    ...
elif (row, column) in ship_locations:  # If ship at location
    ...

if ships_left == 0:

Numbers are truthy/falsey, so you can instead say

if not ships_left:
    ...
\$\endgroup\$
2
  • 1
    \$\begingroup\$ If, for some reason, you will be creating 24 ships on the 5-by-5 field, your approach will be... slow. \$\endgroup\$
    – svavil
    Nov 9, 2019 at 13:08
  • \$\begingroup\$ @svavil It would be prudent to set a limit in the function. In fact, 6 ships is enough to give a 47% retry rate on average. At 10 ships it will take on average 5 attempts to find an appropriate layout. You can also hard cap this on a 5x5 by inverting the formula at the middle point: above 13 ships, just calculate empty spaces and then return the inverse. This caps our average tries at around 31 per game (98% collision rate at 13 ships), which is still basically imperceptible. Note 25 choose 13 has 5.2M results, so this is still doing a small fraction of the work of searching the whole space. \$\endgroup\$ Nov 12, 2019 at 21:53
0
\$\begingroup\$

This is how i have fixed the bug:

def create_random_ships():
    ship_coordinates = []

    while len(ship_coordinates) < 3:
        ship = [random.randrange(5), random.randrange(5)]
        if ship not in ship_coordinates:
            ship_coordinates += [ship]

    return ship_coordinates

First, i have redefined the ship creation function like above.

Then,

        elif [row, column] in ship_coordinates:
            print("\nBoom! You hit! A ship has exploded! You were granted a new ammo!\n")
            game_board[row][column] = "X"
            ships_left -= 1
            if not ships_left:
                print("My my, i didn't know you were a sharpshooter! Congratz, you won!")
                break

I have changed the coordinate checking statement like above. Now, the bug is fixed and my code is much more improved compared to the previous code. Thank you all.

\$\endgroup\$
1
  • \$\begingroup\$ I’m disappointed to see you’re still using magic numbers in your fix. \$\endgroup\$
    – AJNeufeld
    Nov 16, 2019 at 17:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.