Battleship homework

Question

It's a few weeks old homework, and it's already graded. How can I improve my code?

Write function empty(n), which gets the board size as an argument, and returns an empty board with the given size. For instance, empty(4) must return ['', '', '', ''].

Write function isEmpty(board) returns True if the given board is empty and False if it's not.

Write function inSeries(board) that returns a string representation of the given board. Call inSeries([' ', 'a', '', '', '', 'x', 'x', 'x', '', 'y', 'y', '', '']) must return ' a xxx yy '.

Write function space(board, position, length), which returns True if a ship of the given length can be put (starting) at the position on the given (possibly already non-empty) board. If not, the function returns False. Ships must not touch each other (but the can touch the border of the board).

Write function placed(board, position, length, label), which puts the ship with the given label and the given length at the given position on the board -- if this is possible -- and returns True. If putting the ship at this position is illegal, the function returns False.

Write function exists(board, label) that return True if there is a ship with the given label on the board, and False if there's not.

Write function allTags(board) that return a list of labels of all ships on the board. The labels can be in arbitrary order. Call allTags(['a', '', 'x', 'x', 'x', '', 'y', 'y', '', '']) can return, for instance, ['y', 'a', 'x'].

Write function shot(board, position), which simulates targeting the given position on the board. The function returns 0, in case of a miss, 1, if a ship is hit, but not yet completely sunk, 2, if the ship is sunk (e.g. the player hit the last piece of the ship), but the game is not over yet, 3, if the last piece of the last ship is sunk and the game is over. Besides, the function must of course update the state of the board.

This is my code, working:

def empty(n):
    return ['']*n
def isEmpty(board):
    return all(a=='' for a in board)
def inSeries(board):
    board = map(lambda x:x if x!= '' else ' ',board)
    return "".join(board)
def space(board, position, lenght):
    a=False
    if board[position] == '' and isEmpty(board[position:position+lenght+1]) and len(board) >= position+lenght:
        a=True
    if position+lenght+1 < len(board) and board[lenght+position] != '':
        return False
    if position != 0 and board[position-1] != '':
        return False
    return a
def placed(board, position, lenght, label):
    if space(board, position, lenght):
        for i in range(lenght):
            board[position+i] = label
        return True
    else:
        return False
def exists(board, label):
    return label in inSeries(board)
def allTags(board):
    return list(set("".join(board)))
def shot(board, position):
    if board[position] == '':
        return 0
    elif board[position] != '' and board.count(board[position]) ==1 and len(allTags(board)) >1:
        board[position] = ''
        return 2
    elif board[position] != '' and board.count(board[position]) >1:
        board[position] = ''
        return 1
    else:
        board[position] =''
        return 3

Answer 1

First Priority: White Space for Readability and Logical Section Separations, and PEP8 Rules

It's defined in the PEP8 style guide, and also makes it much MUCH easier to read your code and have logical separation between differing sections of code. This makes it so others can read the code without straining our eyes or being forced to try and figure out things. (Keep in mind the blank lines rules).

This is your program with decent bits of white space and some whitespace around logical 'separation' points*, and anywhere else that helps serve readability. None of my other recommendations are reflected in this code block here.

_{* Separation points, to me, are places such as an extra white line between the end of one logical block (an if statement, an else statement, etc.), sections of code (logical checks vs. returns, etc.), between functions, etc.}

def empty(n):
    return [''] * n


def isEmpty(board):
    return all(a == '' for a in board)


def inSeries(board):
    board = map(lambda x: x if x != '' else ' ', board)
    return "".join(board)


def space(board, position, lenght):
    a = False
    if board[position] == '' and isEmpty(board[position:position + lenght + 1]) and len(
            board) >= position + lenght:
        a = True

    if position + lenght + 1 < len(board) and board[lenght + position] != '':
        return False

    if position != 0 and board[position - 1] != '':
        return False

    return a


def placed(board, position, lenght, label):
    if space(board, position, lenght):
        for i in range(lenght):
            board[position + i] = label

        return True

    else:
        return False


def exists(board, label):
    return label in inSeries(board)


def allTags(board):
    return list(set("".join(board)))


def shot(board, position):
    if board[position] == '':
        return 0

    elif board[position] != '' and board.count(board[position]) == 1 and len(allTags(board)) > 1:
        board[position] = ''
        return 2

    elif board[position] != '' and board.count(board[position]) > 1:
        board[position] = ''
        return 1

    else:
        board[position] = ''
        return 3

---

Function names

According to PEP8, function names should be lowercase, and if comprised of multiple words, should be separated by underscores between the words. (NOTE: If you still run things through your instructor's unit tests, then you may need to leave the names alone and skip this recommendation)

Rename the following functions as per these suggestions:

isEmpty  -->  is_empty
inSeries -->  in_series
allTags  -->  all_tags

---

Simplify "If" Logic in shot()

This is what you have now:

def shot(board, position):
    if board[position] == '':
        return 0

    elif board[position] != '' and board.count(board[position]) == 1 and len(allTags(board)) > 1:
        board[position] = ''
        return 2

    elif board[position] != '' and board.count(board[position]) > 1:
        board[position] = ''
        return 1

    else:
        board[position] = ''
        return 3

You're doing too many checks here on the elifs. Both else conditions check if a location is not 'empty' in be board. So let's simplify these statements a bit.

Step 1: Take the first if statement, make it if/else. Step 2: Remove the first of the two conditional parts in your elifs, and make it its own nested if statement in the else clause of the first conditional.

Those two steps end you up with something like this:

def shot(board, position):
    if board[position] == '':
        return 0

    else:
        if board.count(board[position]) == 1 and len(alltags(board)) > 1:
            board[position] = ''
            return 2

        elif board.count(board[position]) > 1:
            board[position] = ''
            return 1

        else:
            board[position] = ''
            return 3

Answer 2

Consider the concept of "return once", sometimes called "single entry, single exit". It is discussed in several places on the SE network. Here is one example..

I'll expand on one of Thomas Wards examples to show how this might make your code better:

def shot(board, position):

    retval = -1; #this is the default, I've chosen -1 to represent an error case where none of the if/else branches ran

    if board[position] == '':
        retval = 0

    elif board[position] != '' and board.count(board[position]) == 1 and len(allTags(board)) > 1:
        board[position] = ''
        retval = 2

    elif board[position] != '' and board.count(board[position]) > 1:
        board[position] = ''
        retval = 1

    else:
        board[position] = ''
        retval = 3

    return retval

This is an improvement because it makes your return style consistent. Even in the original question there is a mixed pattern of setting a or outright returning.

As a commenter points out, the use of "return once" is debatable. I happen to prefer it, you may not. Wether you do or don't use it, being consistent will help.