Making a Battleship game

Question

I'm making a Battleship game using python 2.7. However, how to make it is not currently my issue. The main aspect that I'm asking about is the style and method of making the code. Am I following best (or even good) practice? Are the comments too much, too little, unclear? If someone could give me some pointers based on my code on how to make it more readable and standard, it would be highly appreciated.

from random import randint

# Adds the board to use as a list later
board = []

# Numbering my rows and columns
row_num = ["0", "1", "2", "3", "4"]

# "0" string begins a space as the row numbering pushed in the output text.  Could the space be better implemented in print board?
columns = ["  0", "1", "2", "3", "4"]

for x in range(5):
    board.append(["-"] * 5)

# Function for making the board.  Added tabs to make it more in the middle and clearer.
def print_board(board):
    row_num = 0
    print "\t\t", "   ".join(columns)
    for row in board:
        print "\t\t", row_num, "   ".join(row)
        row_num += 1

print "Let's play Battleship!"
print_board(board)

# Functions for playing agasint the computer
def random_row(board):
    return randint(0, len(board) - 1)

def random_col(board):
    return randint(0, len(board[0]) - 1)

print "Would you like to play agasint the computer or against a friend?"

# Choosing the location of the battleship and data validation of raw_input
def game_type(game_input):
    global ship_row
    global ship_col
    if game_input == 1:
        ship_row = random_row(board)
        ship_col = random_col(board)
    elif game_input == 2:
        ship_row = int(raw_input("Choose a Row to place your battleship:"))
        ship_col = int(raw_input("Choose a Column to place your battleship:"))
    else:
        print "That is not a valid input.  Please try again"
        game_type(int(raw_input("To play against the computer press 1, to play against a friend press 2: \n")))

game_type(int(raw_input("To play against the computer press 1, to play against a friend press 2: \n"))) 

# Printing the sollution for easy testing
print ship_row
print ship_col

# Starts the actual game
for turn in range(4):
    turn
    print "Turn", turn + 1

    guess_row = int(raw_input("Guess Row:"))
    guess_col = int(raw_input("Guess Column:"))

    if guess_row == ship_row and guess_col == ship_col:

        print "Congratulations! You sunk my battleship!"
        board[guess_row][guess_col] = "X"
        print_board(board)
        break
    # Data Validation for the inputs.  Maybe this shouldn't make a player miss but rather allow to retry
    else:
        if (guess_row < 0 or guess_row > 4) or (guess_col < 0 or guess_col > 4):
            print "Oops, that's not even in the ocean."
        elif(board[guess_row][guess_col] == "O"):
            print "You guessed that one already."
        else:
            print "You missed my battleship!"
            board[guess_row][guess_col] = "O"
            if turn == 10:
                print "Game Over"
                break
        print_board(board)

Answer 1

Here are some comments to your implementation in no particular order:

• Avoid intermixing top level code and functions – In your code you do mix the top level code like for x in range(5) with the following function declaration of print_board(). It is better to collate all the function definitions, and then call them from within the if __name__ == '__main__': block.
• Avoid magic numbers – In your code you use 5 and later on range(4), and similarily you set row_num and columns based on the magic number of 5. This should rather be a constant BOARD_SIZE = 5 near the top, and all references should be from this. This allows for an easy change of board size, and it is clearer what the other positions refers to.
• Avoid using globals, if possible – You use ship_row and ship_col as globals, when you easily could have returned this somewhere using a returned tuple
• Avoid non-functional recursive loops – In game_type() you possibly call your self recursively, when you should rather have a while True: loop until you are satisfied with the loop. An even worse variant of this is to start a new game at the end of the game, which would be a kind of endless loop as you don't return until you'll end the game, and then it backtracks out of the recursivity.
• BUG/Feature: Premature end of game after 4 turns – In your code you do for turn in range(4) and later on you test for turn == 10 indicating that it should be allowed with 10 tries, but the for loop will always end after 4 turns...
• Add validation to input – If you make a dedicated function validating the input to be allowed coordinates, you can simplify your game logic a little.
• Use docstring not comments in front of functions – The python guidelines indicates that you should use docstrings, i.e. """Print the board.""", after the function definitions, rather than comments in front of them. Also make sure the comments are correct. The comment #Function for making the board. ... is faulty, as the function only prints the board.
• Convert to new style print function – Using the new style print formatting both eases the possible transition to Python 3, as well as introducing some nice formatting option allowing for better printing of the board, if the board sizes changes.
• Consider entering the board size and/or number of tries – These values could easily be inputted instead of begin hard coded, allowing for a more flexible game play and change of difficulty.

Refactored code

Here is some refactored code handling the issues I've mentioned:

from random import randint
from itertools import count


BOARD_SIZE = 5
MAX_TURNS = 10
PLACED_MARKER = "O"


def print_board(board):
    "Print the playing board."""

    print("\t\t  {}".format(" ".join("{:>2}".format(i) for i in xrange(BOARD_SIZE))))
    for row_idx, row in enumerate(board):
        print("\t\t{:>2} {}".format(row_idx, "  ".join(row)))


def initialize_board(game_type):
    """Create and return the playfield and the battleship position."""

    if (game_type == 1):
        ship_row = randint(0, BOARD_SIZE - 1)
        ship_col = randint(0, BOARD_SIZE - 1)
    else:
        ship_row = get_input("Enter row of battleship: ", 0, BOARD_SIZE -1 )
        ship_col = get_input("Enter column of battleship: ", 0, BOARD_SIZE -1 )

    board = [["-"] * BOARD_SIZE for _ in range(BOARD_SIZE)]

    return board, (ship_row, ship_col)


def get_input(prompt, minimum=None, maximum=None):
    """Get an integer (possibly limited by min and max values)."""

    while True:
        try:
            number = int(raw_input(prompt))

            if minimum is not None and number < minimum:
                print("Number must be greater than or equal to: {}".format(minimum))
                continue

            if maximum is not None and number > maximum:
                print("Number must be less than or equal to: {}".format(maximum))
                continue

            break

        except ValueError:
            print("Please input only numbers")

    return number 


def play_battleship():

    print("Let's play Battleship!")


    game_type = get_input("Should the battleship be placed by the computer (1) or a friend (2)? ", 1, 2)
    board, battleship_position = initialize_board(game_type)

    print("Battleship is at {}".format(battleship_position))
    for turn in count(1):
        row_guess = get_input("Guess row: ", 0, BOARD_SIZE - 1)
        col_guess = get_input("Guess column: ", 0, BOARD_SIZE - 1)


        print("Guessed position is {}".format((row_guess, col_guess)))

        if battleship_position == (row_guess, col_guess):
            print("Congratulations! You sunk the battleship!")
            break

        if board[row_guess][col_guess] == PLACED_MARKER:
            print("You guessed that one before.")
        else:
            print("You missed the battleship!")
            board[row_guess][col_guess] = PLACED_MARKER

        if turn == MAX_TURNS:
            print("Game Over")
            break

        print_board(board)


if __name__ == '__main__':
    play_battleship()

Answer 2

Reduce global variables

The game_type function can return the ship row and column, there is no need to modify them as global variables, my suggestion is easy to implement:

def game_type(game_input):
    if game_input == 1:
        ship_row = random_row(board)
        ship_col = random_col(board)
    elif game_input == 2:
        ship_row = int(raw_input("Choose a Row to place your battleship:"))
        ship_col = int(raw_input("Choose a Column to place your battleship:"))
    else:
        print "That is not a valid input.  Please try again"
        return game_type(int(raw_input("To play against the computer press 1, to play against a friend press 2: \n")))
    return ship_row, ship_col

ship_row, ship_col = game_type(int(raw_input(
    "To play against the computer press 1, to play against a friend press 2: \n"))) 

Also, columns is only ever used inside print_board so I would define it as a default parameter, to make it clear that the rest of the program does not need it:

def print_board(board, columns = ["  0", "1", "2", "3", "4"]):
    print "\t\t", "   ".join(columns)
    for row_num, row in enumerate(board):
        print "\t\t", row_num, "   ".join(row)

First define, then "do stuff"

Organization is a nice property of code. Now you mix function definitions and code that "does things" (namely code that interacts with the user).

I would define at first all of the functions, and then run them.

Write a function for input checking

The main logic (now incorporated into main battleship_game function) is full of checks to make sure that the input is valid, abstracting the input checking to another function will condensate the top-level logic.

Costumize

After you define a battleship_game function, you can have fun customizing it by passing all kinds of different parameters: the board size, the number of turns, whether to run in debugging mode or not...

This will allow you to play slightly different battleship games by using the same code.

Use enumerate

99% of the times manually incrementing a counter is not needed in Python:

def print_board(board, columns = ["  0", "1", "2", "3", "4"]):
    print "\t\t", "   ".join(columns)
    for row_num, row in enumerate(board):
        print "\t\t", row_num, "   ".join(row)

Using built-ins is usually simpler than writing your own.

Avoid do-nothing lines

In Python the line:

value

Where value may be anything, does nothing. In your code the line

turn

Does nothing at all, remove it to simplify the code.

List comprehension + _

A list comprehension is preferred over append in simple cases, _ signifies that you are not interested in the value.

board = [["-"] * 5 for _ in range(5)]

Remove unused definitions

row_num is defined but never used, so delete the line:

row_num = ["0", "1", "2", "3", "4"]

Use the range that fits your needs best

>>> help(range)
Help on class range in module builtins:

class range(object)
 |  range(stop) -> range object
 |  range(start, stop[, step]) -> range object
 |  
 |  Return a sequence of numbers from start to stop by step.
 ...

Using the second version of range will avoid incrementing afterwards:

for turn in range(1, 5):
    print "Turn", turn

I also find it more obvious this way that turns are 1-indexed.