2 Player Battleship Game

This is an adaption of a single player Battleship game. I made it two player, and I also tried to implement OOP and DRY principles. I would like my code to be reviewed for OOP, DRY, PEP8, and overall best practices. I am new to development (about 1 month strong), so any constructive feedback would be greatly appreciated.

One of my biggest light-bulb moments was when I realized that I was stuck in a loop because my loop method was a numeric value outside of the class method. Once I put it in the class method and made it a list, I could then pass the data back and forth and get a count of each players tries by using len(loop).

```
from random import randint

class Person(object):

def __init__(self, name, turn, loop):
self.name = name
self.turn = turn
self.loop = loop

@classmethod
def create(cls, turn):
while True:
name = input("\nWhat is the name of Player %s? " % turn)
if name.isalpha():
break
print("\nPlease share your name with me.\n")
print("\nNice to meet you %s. " % name)
print("It will be fun to play Battleship!\n")
loop = []
return cls(name, turn, loop)

@staticmethod
def welcome(name1, turn1, name2, turn2):
print("It's decided that")
print("%s will take the %sst turn" % (name1, turn1))
print("and %s will take the %snd turn." % (name2, turn2))

def salutation(name1, name2, loop1, loop2):
if (len(loop1)) and (len(loop2)) == 5:
print("Thanks for playing %s and %s." % (name1, name2))
print("Hopefully we will play again, soon!")
elif (len(loop1)) > (len(loop2)):
print("Excellent win, %s!" % name1)
print("Better luck next time, %s." % name2)
else:
print("Excellent win, %s!" % name2)
print("Better luck next time, %s.\n" % name1)

class Board(object):

def __init__(self, surface, squares):
self

Some high-level comments first:

-
There are lots of strings, but no docstrings or comments. That makes it very hard to tell what should be happening. Writing good documentation makes it much easier to read, review and maintain code – get into that habit.

-
Your Person class knows too much about Battleship. It’s printing things very specific to a game of battleship. Ideally it should be a self-contained class.

Among other things:

• It prints “It will be fun to play Battleship!”

• The welcome() method assumes a two player game. This class would be more useful if it could be used for games with an arbitrary number of players, and then the game implements a welcome() method and knows the list of players. That allows different games to have different numbers of players and/or different ordering schemes.

• Likewise, the salutation() method doesn’t really belong on this class. And how does it know to stop when loop1 is empty and loop2 is five long?

• The turn and loop attributes feel like something that should be managed in a game class, not by a person.

-
Custom classes should implement a __repr__() method. This is really helpful for debugging. As a simple case, something that can be eval’d to get an equivalent object. For example:

class Person:
    def __repr__(self):
        return '%s(%r, %r, %r)' % (self.__class__.__name__,
                                   self.name,
                                   self.turn,
                                   self.loop)

-
Your Board class is quite weird. I don’t see any instance methods (methods whose first argument is self), just an assortment of disconnected functions and class methods. This can be tidied up. For example:

def random_row(self):
    return randint(1, len(self.surface))

One of the purposes of OOP is to keep shared state together, but as far as I can tell, the shared state initialised in __init__ is never actually used.

This class also knows too much about battleship.

I would make Board into a generic game-board class, which supports a 2D grid of points (possibly not square) of arbitrary size. Then have a BattleshipBoard or BattleshipGame class which has the specialised logic (e.g. 3 ≤ size ≤ 5) for a game of battleship.

-
The play_battleship() code should be a method of this BattleshipGame class, so that you can share state about the players and boards between different calls. It’s quite messy at the moment.

Some smaller suggestions:

-
The comment

# These are functions, not methods

is clear from reading the code. You don’t need it.

-
Be careful with validating names. It’s very hard to determine what is and isn’t a valid name. Your simple isalpha() check will exclude names that are perfectly valid:

>>> 'Jean-Luc Picard'.isalpha()
False
>>> 'علاء الدين'.isalpha()
False
>>> '岩田 聡'.isalpha()
False

Names are hard to get right. It may well be easier to skip trying to validate, and just check that the user enters something printable.

-
In the salutation() method, it’s not obvious what the logic is supposed to be – a comment would help. Also, if (len(loop1)) would be more idiomatically written as if loop1.

-
In the rules() method of Board, you’ve misspelt “column” as “coloumn”.

-
Prefer longer, more expressive variable names over single letters. It usually makes your code more readable. For example:

def print_board(name, surface):
    for row in surface:
        print(' '.join(row))

Although it would be even better if the Board class implemented a __str__ method which gave a pretty-printed representation of a board.

StackExchange Code Review Q#113694, answer score: 7