2 Player Battleship Game

Question

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.surface = surface
        self.squares = squares

    @classmethod
    def create(cls, name):
        while 1:
            squares = input("\n%s, how big would you like your board to be (3-5)? " % name)
            try:
                squares = int(squares)
            except (TypeError, ValueError):
                print("\nPlease enter a number between 3 and 5.")
                continue
            if squares >= 3 and squares <= 5:
                break
        surface = []
        for i in range(squares):
            surface.append((["O"] * squares))
        return cls(surface, squares)

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

    def random_col(surface):
        return randint(1, len(surface[1]))

    def rules():
        print("\nIn this game, you will pick a number")
        print("between 1 and your board length for each row and")
        print("coloumn. Then, if your guess matches")
        print("the randomly generated location. You win.")
        print("Each player has 5 attempts to guess correctly.")

    def one_play(name, turn, surface, row, col, loop):
        print("\nOk, %s. Go ahead and take turn %s." % (name,(len(loop)+1)))
        print_board(name, surface)
        guess_row = (input("Guess Row (1-%s): " % len(surface)))
        guess_col = (input("Guess Col (1-%s): " % len(surface)))
        try:
            guess_row = int(guess_row)
            guess_col = int(guess_col)

            if guess_row == row and guess_col == col:
                print("\nCongrats! You sunk my Battleship!\n")
                surface[(guess_row)-1][(guess_col)-1] = "B"
                loop.extend((1, 2, 3, 4, 5, 6))

            elif ((guess_row < 1 or guess_row > (len(surface))) or
                    (guess_col < 1 or guess_col > (len(surface)))):
                print("\nOops, that's not even on the board.\n")

            elif (surface[(guess_row)-1][(guess_col)-1] == "X"):
                print("\nYou guessed that one already.\n")

            else:
                print("\nYou missed my Battleship!\n")
                surface[(guess_row)-1][(guess_col)-1] = "X"
        except (TypeError, ValueError):
            print("\nYou failed to answer the question correctly.")
        loop.append(1)
        return (surface, loop)

# These are functions, not methods
def print_board(name, surface):
    print("\nHere is the board for %s." % name)
    for i in surface:
        print(" ".join(i))
    print("")

def play_battleship():

    print("\n\n\n\nWelcome to Battleship!") 
    # We take the user input and create the Players
    Player1 = Person.create(1)
    Player2 = Person.create(2)

    # Assign the names to variables
    name1 = Player1.name
    name2 = Player2.name

    # Assign each player's turn.
    turn1 = Player1.turn
    turn2 = Player2.turn

    # It's always good to say, "Hello."
    Person.welcome(name1, turn1, name2, turn2)

    # We create the boards, which are lists.
    Board1 = Board.create(name1)
    Board2 = Board.create(name2)

    # Print the rules.
    Board.rules()

    # Store the random row and column in a variable for each player.
    ship_row1 = Board.random_row(Board1.surface)
    ship_col1 = Board.random_col(Board1.surface)
    ship_row2 = Board.random_row(Board2.surface)
    ship_col2 = Board.random_col(Board2.surface)

    # Place each player's list in a variable.
    surface1 = Board1.surface
    surface2 = Board2.surface

    # Keep track of each player's loop for flow control with a list.
    loop1 = Player1.loop
    loop2 = Player2.loop


while (len(loop1) < 5) and (len(loop2) < 5):
    Board.one_play(name1, turn1, surface1,
                   ship_row1, ship_col1, loop1)
    # Here, we check the length of loop1 to see if Player1 won.
    # If so, we break. the loop.
    if (len(loop1))>= 6:
        break
    else:
        Board.one_play(name2, turn2, surface2,
                       ship_row2, ship_col2, loop2)

Person.salutation(name1, name2, loop1, loop2)
# Ask the player to play again.
while 1:
    again = input("\n\nWould you like to play again: ")
    if "y" in again.lower():
        play_battleship()
    else:
        break

play_battleship()

Answer 1

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.