3 dimensional python battleship game

Question

I have an assignment in a python course. We were instructed to make a 3D Battleships game for 2 players on the same computer. I've written a code that works, according to the assignment instructions, but I think it looks a little messy. I'm a beginner. Any suggestions to make the code simpler and tidier?

import numpy as np
from enum import Enum
class Input(Enum):
 MISS = 'Miss'
 HIT = 'Hit'
 KILL = 'Kill'
 END = 'End'
class Pieces:
 """ all of the game pieces base class """
 def __init__(self, item_id: int):
 self.item_id = item_id
 self.outline = np.array([0])
 self.locs = None
 @property
 def shape(self):
 return self.outline.shape
 @property
 def filled_outline(self):
 return self.outline * self.item_id
 def __repr__(self):
 return f"{self.__class__.__name__}{self.item_id}"
class General(Pieces):
 """A number is submitted as input for compatibility, which is the game's determining piece"""
 def __init__(self, item_id: int):
 super().__init__(item_id)
 self.outline = np.array([[1]], dtype=np.int16)
 self.airplane = np.random.randint(3)
 def hit(self, coord):
 return Input.END
class Jet(Pieces):
 """ Class for the airplane vessels """
 def __init__(self, item_id: int):
 super().__init__(item_id)
 self.outline = np.array([[0, 1, 0], [1, 1, 1], [0, 1, 0], [0, 1, 0]], dtype=np.int16)
 self.airplane = 2 # air
 def hit(self, coord):
 return Input.KILL
class Destroyer(Pieces):
 """ class for ships 'Destroyers'"""
 def __init__(self, item_id: int):
 super().__init__(item_id)
 self.outline = np.array([[1], [1], [1], [1]], dtype=np.int16)
 self.airplane = 1 # Sea-level
 def hit(self, cord: tuple):
 cord = list(cord)
 try:
 self.locs.remove(cord)
 except ValueError:
 print(f"This cordination is not found or already targeted -{cord}")
 return Input.MISS
 else:
 if len(self.locs) == 0:
 return Input.KILL
 else:
 return Input.HIT
class Submarine(Pieces):
 """ a Submarine vessel """
 def __init__(self, item_id: int):
 super().__init__(item_id)
 self.outline = np.array([[1, 1, 1]], dtype=np.int16)
 self.airplane = 0 
 def hit(self, coord):
 return Input.KILL
class GamePieces(Enum):
 """ the pieces type in the game"""
 General = General
 JETS = Jet
 DESTROYERS = Destroyer
 SUBMARINES = Submarine
class gameBoard(np.ndarray):
 """
 A three dimensional board that holds the pieces.
 """
 def __new__(subtype, shape: tuple=(10, 10, 3), dtype: np.dtype=np.object, *,
 pieces: list):
 if len(shape) != 3:
 return ValueError(f"Shape received was {shape}, but it must have exactly 3 items.")
 obj = super(gameBoard, subtype).__new__(subtype, shape, dtype)
 obj.pieces = pieces
 obj[:] = 0
 return obj
 def __array_finalize__(self, obj):
 """ A unique method which is run after initialization """
 if obj is None: return
 default_pieces = [piece.value(item_id) for item_id, piece in enumerate(GamePieces)]
 self.pieces = getattr(obj, 'pieces', default_pieces)
 def pieces_places(self):
 """
 Verifies that the piece was not put outside the board's bounds and that the cells were empty.
 """
 for piece in self.pieces:
 placed = False
 trial = 0
 while not placed and trial < 50:
 row_idx = np.random.choice(self.shape[0])
 col_idx = np.random.choice(self.shape[1])
 cur_slice = (slice(row_idx, row_idx + piece.shape[0]),
 slice(col_idx, col_idx + piece.shape[1]),
 slice(piece.airplane, piece.airplane + 1))
 try:
 cur_subboard = self[cur_slice].copy()
 assert cur_subboard.shape[:2] == piece.shape
 assert np.all(cur_subboard == 0) 
 placed = True
 except (IndexError, AssertionError):
 pass
 finally:
 trial += 1
 if trial >= 50:
 raise UserWarning("Board is too small for all pieces.")
 self[cur_slice] += np.atleast_3d(piece.filled_outline)
 piece.locs = np.argwhere(self == piece.item_id).tolist()
 self[self == piece.item_id] = piece
 def check_if_hit(self, coord: tuple) -> Input:
 """
 Receive a 3D coordinate of the location that was targeted.
 Then check the board which piece is there and return the Input.
 :param coord: Tuple of 3 coordinates
 :return: Input
 """
 cell = self[coord]
 if cell != 0:
 sig = cell.hit(coord)
 if sig is Input.KILL:
 self.pieces.remove(cell)
 if len(self.pieces) == 1:
 sig = Input.END
 else:
 sig = Input.MISS
 return sig
class SubmarinesGame:
 """
 Play a Submarines game for two players. Run it with the start() method.
 Supply a tuple defining the 3D board size, and adictionary of unit names (from GamePieces) and values,
 corresponding to the number of units you wish to have of that size.
 At each turn you're prompted to enter a length-3 tuple with the coordinate you're targeting.
 You'll be notified if you missed, hit or killed a unit. You can also write "show" to show the board,
 and "quit" to stop the game.
 """
 def __init__(self, board_shape: tuple=(10, 10, 3), pieces: dict=None):
 self.board_shape = board_shape
 self.pieces = pieces
 self.piece_list_1 = []
 self.piece_list_2 = []
 self.__validate_input()
 self.__generate_pieces_list()
 self.board1 = gameBoard(self.board_shape, pieces=self.piece_list_1)
 self.board2 = gameBoard(self.board_shape, pieces=self.piece_list_2)
 self.players = ("Player 1", "Player 2")
 self.boards = (self.board2, self.board1)
 self.move = 0
 def __validate_input(self):
 assert len(self.board_shape) == 3
 assert self.board_shape[2] == 3 # only 3D inputs
 assert self.board_shape[0] >= 4 and self.board_shape[1] >= 4
 if self.pieces is not None:
 for piece, val in self.pieces.items():
 assert piece in GamePieces
 assert isinstance(val, int)
 assert val >= 0 and val <= 50
 assert GamePieces.General in self.pieces
 assert self.pieces[GamePieces.General] == 1
 def __generate_pieces_list(self):
 """ Populate the list of pieces """
 if self.pieces is not None:
 item_id = 1
 for piece, num in self.pieces.items():
 for cur_instance in range(num):
 self.piece_list_1.append(piece.value(item_id))
 self.piece_list_2.append(piece.value(item_id))
 item_id += 1
 def __assert_coor_in_board(self, coord):
 coor_list = []
 for char in coord:
 if char.isdigit():
 coor_list.append(int(char))
 coord = tuple(coor_list)
 try:
 assert len(coord) == 3
 assert coord[0] < self.board1.shape[0] \
 and coord[1] < self.board1.shape[1] \
 and coord[2] < self.board1.shape[2]
 except AssertionError:
 print(f"Coordinate {coord} is located outside the board. Board shape is {self.board1.shape}.")
 return False
 else:
 return coord
 def __parse_input(self):
 """ Helper function to parse the input from the user """
 coor = input(f"{self.players[self.move % 2]} this is your turn. type you target! for example: (1,1,1) ")
 if coor == 'quit':
 raise SystemExit("Exiting")
 if coor == 'show':
 board = self.boards[(self.move-1) % 2]
 print("Under-water:\n", board[..., 0])
 print("Sea-level:\n", board[..., 1])
 print("Air:\n", board[..., 2])
 return False
 coor = self.__assert_coor_in_board(coor)
 return coor
 def start(self):
 """ Battleships game """
 print("This is a game of Battleships\n")
 print(f"The board dimensions are {self.board1.shape} and the pieces are assigned randomly.")
 print("You can type 'show' to show your board, and 'quit' to exit the game.")
 for board in self.boards:
 board.pieces_places()
 ret_Input = 0
 while ret_Input != Input.END:
 coor = False
 while not coor:
 coor = self.__parse_input()
 ret_Input = self.boards[self.move % 2].check_if_hit(coor)
 print(ret_Input)
 self.move += 1
 print(f"The game is over! The winner is {self.players[(self.move - 1) % 2]}.")
if __name__ == '__main__':
 pieces = {GamePieces.General: 1,
 GamePieces.JETS: 1,
 GamePieces.DESTROYERS: 1,
 GamePieces.SUBMARINES: 1}
 subgame = SubmarinesGame(board_shape=(6, 6, 3), pieces=pieces)
 subgame.start()

• Reinderien: Please include the assignment instructions.

Answer 1

StackExchange's Q&A format doesn't lend itself well to discussions, so I'll just point out a few random things, instead:

• gameBoard should be GameBoard, by naming convention.
• I can't really tell what/why item_id is used for. (self.outline * self.item_id?)
• I think it'd be nicer to make code read more linearly by getting rid of some syntactical code nesting (if > for, for > while, for > for). (Can't change logical nesting ofc, but can reorganize to aesthetically remove nesting.) This is kinda related to "Don't mix different levels of abstraction".
• I would avoid having GameBoard inherit from np.ndarray b/c Composition Over Inheritance
• You've got a lot of code that's like xyz_1 and xyz_2 (piece lists, boards, players, etc). Would feel more elegant as just players[0] and players[1] with player.board, etc.

Answer 2

The biggest problem with this code is readability. The actual program looks well-structured (good functions, reasonable use of classes, use of appropriate libraries, etc).

There are a large number of very small things to fix.

We'll start with naming, the biggest problem. I've put a complete list of suggested renames, with a very short explanation of why. The reasons are to:

• consistency: Remove inconsistency within your program (Don't use coor, cord and coord. Pick one.)
• convention: Follow common programmer convention (use the singular class name Piece, not Pieces). Read PEP8, mentioned below, to learn common Python conventions.
• english: Learn and write correct English (pieces_places should be place_pieces).
• clarity: Think about clear names (ret_Input becomes guess_outcome). This is the hardest but most important.

In rough order of appearance, rename:

• Input to GuessOutcome (clarity)
• Input.END to GuessOutcome.END_GAME` (clarity)
• Pieces to Piece (convention--singular classes)
• GamePieces.General to GamePieces.GENERAL (consistency)
• gameBoard to GameBoard (convention)
• pieces_places to place_pieces (english)
• check_if_hit to is_hit (convention)
• SubmarinesGame.__validate_input to SubmarinesGame._validate (convention--input is always user input)
• SubmarinesGame._generate_pieces_list to SubmarinesGame._populate_unplaced_pieces (clarity--prefer action verbs)
• SubmarinesGame.__assert_coor_in_board to SubmarinesGame._assert_coordinate_on_board (consistency and english)
• SubmarinesGames.__parse_input to SubmarinesGames._get_guess (clarity and convention)
• Piece: piece.locs to piece.squares_occupied (clarity and convention--avoid abbreviations in python)
• Piece: piece.outline to ?? (clarity, not sure what this is)
• Piece: piece.airplane to piece.height (clarity and maybe english)
• Destroyer: cord to coordinate (consistency, english, and convention--avoid abbreviations in python)
• GameBoard.check_if_hit: coord to coordinate (consistency and convention)
• SubmarinesGame: self.piece_list_1 and self.piece_list_2 to self.players[0].pieces and self.players[1].pieces (clarity and convention--prefer array variables to multiple variables)
• SubmarinesGame: self.board1 and self.board2 to self.players[0].board and self.players[1].board (clarity and convention)
• SubmarinesGame: self.move to self.turn_number (clarity)
• SubmarinesGame: self.pieces to SUBMARINES_PIECES (convention--should be a constant, and clarity that it's a map from type of piece to number of pieces)
• SubmarinesGame.__generate_pieces_list: cur_instance to _ (convention--unused loop variables get a dummy name _ in python to indicate they're not used)
• SubmarinesGame.__assert_coor_in_board: coord to unparsed_coordinate / the second variable called coord to parsed_coordinate (consistency, clarity, convention of not re-using variable names)
• SubmarinesGame.__assert_coor_in_board: coor_list to coordinate_list (consistency)
• SubmarinesGame.start: coor to coordinate (consistency)
• SubmarinesGame.start: ret_Input to guess_outcome (clarity)

The way you've split up functions is pretty good.

Other things to improve, in no particular order:

• Read, understand, and follow PEP8. This is the standard Python style guide.
• Consistently use type hints. Add a type for a single 3-D coordinate
• print in only one place, not across several functions. Printing in the main game loop (start) would be a good single place here.
• This code is hard to read without knowing numpy. A few comments would be helpful.
• Don't have GameBoard inherit from np.ndarray, as mentioned in Kache's review.
• Write docstrings. Yours look copied. Your functions should each do one clear thing, and return something clear. This is what the docstring should explain--what the function does, and what it returns, as a single sentence. Count throwing uncaught errors as a return value for docstring purposes.
• Don't use the backslash continuation in python when you can avoid it. Where you've used it, you can instead use parentheses, or three assert statements.
• Your program should probably print a full rules explanation for the player, let alone the reviewer.