2

I'm trying to improve upon the "battleship" game you make in the codecademy course for python and I decided my first step would be to implement classes.

My expected output from the code is five rows of O O O O O. This is achieve when I use just a print statement inside of the for loop but it throws an error stating: 

Traceback (most recent call last):
File "D:/PythonProjects/Battleship/Main.py", line 21, in <module>
    print(board)
TypeError: __str__ returned non-string (type NoneType)

And when I leave the code as is it doesn't throw and errors but only prints one row of O's

Code in question:

class Board(object):
    """Creates the game board"""
    board = []

    def __init__(self, size):
        self.size = size
        for x in range(size):
            self.board.append(["O"] * size)

    def __str__(self):
        for row in self.board:
            display = (" ".join(row))

        return display

board = Board(5)
print(board)
vaultah
  • 44,105
  • 12
  • 114
  • 143
Bitterguy
  • 19
  • 1
  • 5

2 Answers2

2

your __str__ method assigns a new value to display with every iteration of the loop. Instead you could do something like:

def __str__(self):
    accumulator = ""
    for row in self.board:
        accumulator += " ".join(row) + "\n"
    return accumulator

Or more succinctly

def __str__(self):
    return "\n".join([" ".join(row) for row in self.board])

As a complete aside from someone who's designed this over and over again, helping every new set of CS students going into Intro to OOP, this class becomes much easier if it inherits from list. It's a container for lists that describe a game board -- make it a list itself....

class Board(list):
    def __init__(self, size):
        """Square board of length `size`"""
        for row in range(size):
            self.append(["O"] * size)

    def __str__(self):
        return "\n".join([" ".join(row) for row in self])
Adam Smith
  • 52,157
  • 12
  • 73
  • 112
  • 1
    *“It's a container for lists that describe a game board”* – Just because it *uses* a list to store its data that doesn’t make it a list. A board, having a fixed number of rows and a special set of allowed interactions, is not a list. By inheriting from list, you make it a list and inherit the whole list interface with all its manipulation methods which are all not appropriate here. – poke Jun 07 '15 at 19:54
  • I'll second that. A container of lists is _not_ a specialized list. Shame on you for teaching noob CS students this. – martineau Jun 07 '15 at 23:12
2

First of all, you shouldn’t use a class property for the board. Doing so will share the instance across all Board instances, so you keep adding to the same list. Instead, create a new list for each object you create:

class Board(object):
    def __init__(self, size):
        self.size = size
        self.board = []
        for x in range(size):
            self.board.append(["O"] * size)

Now, to answer your problem, you shouldn’t actually get that type error because while you keep overwriting display in the loop in __str__, it does get at least one value that is not None, assuming that board is not empty (which it isn’t in your example).

But what you want to do instead is collect all rows and join them with a new line character:

def __str__(self):
    display = []
    for row in self.board:
        display.append(" ".join(row))
    return '\n'.join(display)

Or in one line:

def __str__(self):
    return '\n'.join([' '.join(row) for row in self.board])
poke
  • 369,085
  • 72
  • 557
  • 602
  • I think you should have answered the question first. It's quite possible the OP will never notice the second issue — even though you're correct about it — since it's unlike they have more than one instance of a `Board` in the game. Lastly, `__str__()` can be optimized slightly to just `return "\n".join((" ".join(row) for row in self.board))` — there's no need to create a temporary list of strings just to feed to the outer `join()`. – martineau Jun 07 '15 at 23:18
  • @martineau That’s not correct. Joining on a list comprehension is [more efficient than joining on a generator expression](http://stackoverflow.com/a/9061024/216074); you should always use a list comprehension there. And while I agree, I can’t help that I didn’t manage to post my answer first; longer answers tend to take more time to write. – poke Jun 07 '15 at 23:26
  • I wasn't aware that `join()` with a generator was an anomaly to the general rule of them being more efficient. Thanks for point that out. However I am well aware of the fact that often takes longer to produce more comprehensive (and better) answers. In this case what I was referring to was _within your answer_, I think you should have answered the OP's question first, then mentioned the class design issue. – martineau Jun 07 '15 at 23:58