7
\$\begingroup\$

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
\$\endgroup\$
8
\$\begingroup\$

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
\$\endgroup\$
  • \$\begingroup\$ Oh, I learnt about that. But I didn't correct it in this code. Thanks. \$\endgroup\$ – Blue Monday Dec 12 '16 at 20:16
  • \$\begingroup\$ @BlueMonday I added more to my review. I needed to get that whitespace rant posted before anything else, because when getting code reviewed added whitespace is useful. \$\endgroup\$ – Thomas Ward Dec 12 '16 at 20:29
  • \$\begingroup\$ Even though I usually agree with comments about snake_case vs camelCase, the names here we part of the asignment and can't be changed (professor using unit tests). \$\endgroup\$ – Mathias Ettinger Dec 12 '16 at 21:41
  • \$\begingroup\$ @MathiasEttinger Ah, you're right. Except that since the OP has already turned it in for a grade, and now want to know how to improve the code, that makes such comments regarding case valid, in my opinion. (It's no longer for an assignment, and it's not going to go through the professor's Unit Tests anymore... at least as far as we know) \$\endgroup\$ – Thomas Ward Dec 12 '16 at 21:42
  • \$\begingroup\$ @ThomasWard True that! \$\endgroup\$ – Mathias Ettinger Dec 12 '16 at 21:44
0
\$\begingroup\$

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.

\$\endgroup\$
  • 5
    \$\begingroup\$ I find it humorous that the top answer on the linked question says that they think this style is "pointless or even counterproductive". \$\endgroup\$ – Peilonrayz Dec 12 '16 at 22:30
  • \$\begingroup\$ I'll expand on one of Thomas Wards examples - it might be helpful to provide some additional explanation beyond simply a block of code. Perhaps explain the code you've given and how it is an improvement? \$\endgroup\$ – avojak Dec 12 '16 at 22:38
  • \$\begingroup\$ @avojak - I just edited as you posted. The merits of 'return once' are debatable. OP had an inconsistent usage in the shot method where OP sometimes set a value and sometimes returned immediately. Making that consistent is a good general improvement wether or not OP chooses to return once. \$\endgroup\$ – Freiheit Dec 12 '16 at 22:40
  • \$\begingroup\$ I find it funny that you break Python by failing to indent your code here. The 'style' of return once is being countermanded by the fact you de-indent everything so it sits logically "Outside" of the shot function. (This will break) \$\endgroup\$ – Thomas Ward Dec 12 '16 at 22:53
  • 1
    \$\begingroup\$ I have removed my downvote on this answer after it was updated to rev. 3. I still disagree with using a retval variable in this case, but the advice about being consistent is a good point. (Also the code is now formatted correctly, which was another reason for me to downvote.) \$\endgroup\$ – mkrieger1 Dec 13 '16 at 10:27

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.