I'm relatively new to Python, I decided to make a simple battleship game. Is there anything I should do to make it shorter, or in practice, "better"?

from pprint import pprint as pp
import random

Rows = 0
Columns = 0
turns = 0
Answer = "NaN"

print("Welcome to battleship!")

while (Rows > 10) or (Columns > 10) or (Rows <= 0) or (Columns <= 0):
    Rows = int(input("Please enter the number of rows you want. \n"))
    Columns = int(input("Please enter the number of columns you want. \n"))

def create_grid(Rows, Columns):
    #Creates the 2D Data Grid
    grid = []
    for row in range(Rows):
        row = []
        for col in range(Columns):
            row.append(' ')
        grid.append(row)
    return grid

grid = create_grid(Rows,Columns)

def display_grid(grid, Columns):
    #Prints the labels for the grid
    column_names = 'abcdefghijklmnopqrstuvwxyz'[:Columns]
    print('  | ' + ' | '.join(column_names.upper()) + ' |')
    for number, row in enumerate(grid):
       print(number + 1, '| ' + ' | '.join(row) + ' |')

grid = create_grid(Rows, Columns)
display_grid(grid, Columns)

def random_row(grid):
    #Makes a random row integer
    return random.randint(1,len(grid))

def random_col(grid):
    #Makes a random column integer
    return random.randint(1,len(grid[0]))

def update_gridHit(grid, GuessRow, GuessColumn):
    grid[GuessRow-1][GuessColumn-1] = 'O'

def update_gridMiss(grid, GuessRow, GuessColumn):
    grid[GuessRow-1][GuessColumn-1] = 'X'

ShipRow = random_row(grid)
ShipColumn = random_col(grid)

#Testing purposes only, comment out if needed.
print(ShipRow)
print(ShipColumn)

while (turns != 5):
    GuessRow = int(input("What row do you guess? \n"))
    GuessColumn = int(input("What column do you guess? \n"))

    if (GuessRow == ShipRow) and (GuessColumn == ShipColumn):
        turns += 1
        update_gridHit(grid, GuessRow, GuessColumn)
        display_grid(grid, Columns)
        print("You hit the battleship! Congratulations!")
        break

    else:
        if (GuessRow < 1 or GuessRow > Rows) or (GuessColumn < 1 or GuessColumn > Columns):
            #Warning if the guess is out of the board
            print("Outside the set grid. Please pick a number within it your Rows and Columns.")

        elif (grid[GuessRow-1][GuessColumn-1] == "X"):
            #If "X" is there than print that it missed
            print("You guessed that already.")

        else:
            #Updates the grid with an "X" saying that you missed the ship
            turns += 1
            print("You missed the ship.")
            update_gridMiss(grid, GuessRow, GuessColumn)
            display_grid(grid, Columns)

    if (turns >= 5):
        print("Game over! You ran out of tries")
up vote 9 down vote accepted

Variable naming

In general, you should follow PEP8, the Python style guide. In particular, it suggests using lowercase for variable names. That is, rows instead of Rows.

User input conditions

while (Rows > 10) or (Columns > 10) or (Rows <= 0) or (Columns <= 0):
    Rows = int(input("Please enter the number of rows you want. \n"))
    Columns = int(input("Please enter the number of columns you want. \n"))

So, if I enter a correct row, but a wrong column, it'll ask again. Without telling me what I did wrong.

I'd suggest

rows = 0
while not (0 < rows <= 10):
    rows = int(input("Please enter the number of rows you want [1-10]."))

And a similar loop for columns. This has one problem: What if the user does not enter a number but something else? 'Pizza' or 'one'. In that case, the int throws an exception, and that should be handled as well.

rows = 0
while not (0 < rows <= 10):
    try:
        rows = int(input("Please enter the number of rows you want [1-10]."))
    except ValueError:
        pass  # Just re-ask the question.

Mixing statements and definitions

The code as given has the form

define function
run some statements
define function
run some statements

In general, you should use

define function
define function
define function
define function main:
    statements
    statements
if __name__ == "__main__":
   main()

Re-using variables

Notice how in create_grid, the variable row has two purposes: It is both a number (due to the for row in range(Rows)), and a list. Please don't do that. Because the number row is unused, we should indicate that to the readers by using the conventional _ as variable name.

def create_grid(Rows, Columns):
    #Creates the 2D Data Grid
    grid = []
    for _ in range(Rows):
        row = []
        for _ in range(Columns):
            row.append(' ')
        grid.append(row)
    return grid

(Note: I myself prefer using a double underscore __ instead, but convention is _.)

Encapsulation

Not sure if you are at the level of classes yet, but I'd suggest learning about them.

Instead of

def create_grid(Rows, Columns):
    #Creates the 2D Data Grid
    grid = []
    for row in range(Rows):
        row = []
        for col in range(Columns):
            row.append(' ')
        grid.append(row)
    return grid

grid = create_grid(Rows,Columns)

You can write

class Grid(object):
    def __init__(self, columns, rows):
        self.columns = columns
        self.rows = rows
        self._grid = []
        for __ in range(rows):
            row = []
            for __ in range(columns):
                row.append(' ')
            self._grid.append(row)

Now, that last part (assigning self._grid) can be done 'nicer' (and faster!).

self._grid = [ [' ' for __ in range(columns)] for __ in range(rows)]

Useless call to .upper.

In display_grid, you've written

    column_names = 'abcdefghijklmnopqrstuvwxyz'[:Columns]
    print('  | ' + ' | '.join(column_names.upper()) + ' |')

Why not

    column_names = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ'[:columns]
    print('  | ' + ' | '.join(column_names) + ' |')

That's a bit nicer, right?

Using enumerate

The builtin enumerate has a second argument: start. You can use it to signal counting should start at 1 instead:

    for number, row in enumerate(grid, 1):
       print(number, '| ' + ' | '.join(row) + ' |')

display_grid as a method on the class

If you follow my suggestion of turning the grid into a class, you can write

def display(self):
    """
    Displays a grid.
    """
    column_names = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ'[:self.columns]
    print('  | ' + ' | '.join(column_names) + ' |')
    for number, row in enumerate(self._grid, 1):
       print(number, '| ' + ' | '.join(row) + ' |')

But that's just a suggestion.

grid = create_grid(Rows, Columns)
display_grid(grid, Columns)

Docstrings

In Python, it is the convention to add the documentation of a method/function in a docstring.

Instead of

def random_row(grid):
    #Makes a random row integer
    return random.randint(1,len(grid))

write

def random_row(grid):
    """
    Makes a random row integer
    """

(There are also a lot of conventions on how to phrase a docstring. I won't get into that for now.)

Parenthesis in conditional

The code says

while (turns != 5):

It could (and should) be

while turns != 5:

That is, you don't need the ( and ) there.

Weird order in the guess loop

Now, the following is just preference. But to me, the final loop is unclear. I've written it as pseudo-code below.

The final part looks like this (pseudocode)

while (turns != 5):
    get_guess
    if hit:
        turns += 1
        notify_success
        break
    else:
        if out_of_bounds:
            # Print out-of-bounds message
        elif already_guessed:
            # Print already guessed message
        else:
            # Update miss
if (turns >= 5):
    # Print 'you lost'

What I notice is that it first checks for hit, before checking if the input is valid (in bounds, and not already guessed).

I'd suggest cleaning it up as follows:

while turns != 5:
    while True:
        get_guess()
        if out_of_bounds:
            # Print out-of-bounds message
        elif already_guessed:
            # Print already guessed message
        else:
            break
    turns += 1
    if hit:
        # Update grit hit.
        # Print hit message.
        break
    else:
        # Update grid miss
        # Print miss message.
    display_grid()

There's a way that you can reduce the amount of code in create_grid. If you use the * operator on a list/string and a number then it duplicates it.
Take the following:

>>> 'na ' * 16 + 'Batman'
'na na na na na na na na na na na na na na na na Batman'
>>> [' '] * 5
[' ', ' ', ' ', ' ', ' ']

This will allow you to remove the entire for col in range(Columns): part. You may think that you could simplify the entire code to say:

return [[' '] * columns] * rows

But that'll not work as intended. This is as all the rows will be the same object, and so if you change one, you'll change them all.

>>> a = [[' '] * 5] * 5
>>> a
[[' ', ' ', ' ', ' ', ' '], [' ', ' ', ' ', ' ', ' '], [' ', ' ', ' ', ' ', ' '], [' ', ' ', ' ', ' ', ' '], [' ', ' ', ' ', ' ', ' ']]
>>> a[0][0] = 'b'
>>> a
[['b', ' ', ' ', ' ', ' '], ['b', ' ', ' ', ' ', ' '], ['b', ' ', ' ', ' ', ' '], ['b', ' ', ' ', ' ', ' '], ['b', ' ', ' ', ' ', ' ']]
>>> a[0] is a[1]
True
>>> a[0] == a[1]
True
>>> [' '] is [' ']
False
>>> [' '] == [' ']
True

To overcome this, you need to create the outer list manually. You can do this the way you are currently, or with a comprehension.

Using a comprehension here would be better, but could be confusing to learn.

Say we have the list [0, 1, 2, 3, 4], but we want to multiply everything in it by two, to get [0, 2, 4, 6, 8] you can do:

>>> my_list = [0, 1, 2, 3, 4]
>>> output_list = []
>>> for i in my_list:
        output_list.append(i * 2)
>>> output_list
[0, 2, 4, 6, 8]

Alternately if you:

  1. wrap the for loop in brackets [].
  2. Remove the output_list.append and :.
  3. move the i * 2 to the beginning.

You can make it easier to read.

>>> my_list = [0, 1, 2, 3, 4]
>>> output_list = [i * 2 for i in my_list]
>>> output_list
[0, 2, 4, 6, 8]

And so this function could be:

def create_grid(rows, columns):
    return [[' '] * columns for _ in range(rows)]

The only other glaring 'issue' Sjoerd didn't go over is how you format the board for print. Currently you format it as:

print(number + 1, '| ' + ' | '.join(row) + ' |')
# Or joining the main chunks:
'{number} | {row} |'

Python allows you to use this notation to format output. So all you need to use is it's .format passing the number and row.

'{0} | {1} |'.format(number + 1, ' | '.join(row))

I mostly suggest this as from a glance your prints seemed to be one space out. And so you could change display_grid to:

import string
def display_grid(grid):
    column_names = string.ascii_uppercase[:len(grid[0])]
    print('{0} | {1} |'.format(' ', ' | '.join(column_names)))
    for number, row in enumerate(grid, 1):
        print('{0} | {1} |'.format(number, ' | '.join(row)))

As Sjoerd Job Postmus correctly said you should follow PEP8, and should definitely follow their point on "Mixing statements and definitions".

Your Answer

 

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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