The goal of the code below is limited to the creation of the initial map only and not to the whole game mechanics itself.
The rules are that there are 4 ships of length {2,3,4,5} and no ship may be placed adjacent or diagonally adjacent to another ship.
For now, I plan to stay with the 10x10 grid with 4 ships. In my implementation, a 1 means that there is a ship in that cell, and 0 otherwise.
I am not a Python developer, but since I am interested in learning it, I decided to program this project using this language. Please scrutinize and nitpick my code. I want to get into the so called pythonic way.
This is the code for now. According to my tests, I think it is working as intended:
import numpy as np
import random
grid_x = 10
grid_y = 10
number_of_ships = 4
class Ship:
def __init__(self, x, y, direction, length):
self.x = x
self.y = y
self.direction = direction
self.length = length
def generate_random_ship(self, rows, columns, length):
self.x = random.randrange(rows)
self.y = random.randrange(columns)
self.direction = random.randrange(4)
self.length = length
def print_ship(self):
print('x: %d, y: %d, direction: %d, length: %d' %(self.x,self.y,self.direction,self.length))
class Grid:
def __init__(self, rows, columns):
self.rows = rows
self.columns = columns
self.grid = np.zeros((rows, columns))
#Assume ship is already valid
def place_ship(self,ship):
for i in range(ship.length):
if ship.direction == 0: #up
self.grid[ship.x - i][ship.y] = 1
elif ship.direction == 1: #right
self.grid[ship.x][ship.y + i] = 1
elif ship.direction == 2: #down
self.grid[ship.x + i][ship.y] = 1
elif ship.direction == 3: #left
self.grid[ship.x][ship.y - i] = 1
def check_for_ship(self, x, y):
if x >= self.rows or x < 0 or y >= self.columns or y < 0:
return False
elif self.grid[x][y] == 1:
return True
else:
return False
def is_ship_valid(self, ship):
#up
if ship.direction == 0:
#if the ship won't fit
if ship.length > (ship.x + 1):
return False
else:
#Check for ships in the neighborhood
for i in range(ship.x, ship.x-ship.length, -1):
if (self.check_for_ship(i+1, ship.y-1) or self.check_for_ship(i+1, ship.y)
or self.check_for_ship(i+1, ship.y+1) or self.check_for_ship(i, ship.y-1)
or self.check_for_ship(i, ship.y) or self.check_for_ship(i, ship.y+1)
or self.check_for_ship(i-1, ship.y-1) or self.check_for_ship(i-1, ship.y)
or self.check_for_ship(i-1, ship.y+1)):
return False
#right
elif ship.direction == 1:
#if the ship won't fit
if ship.length > (self.columns - ship.y):
return False
else:
#Check for ships in the neighborhood
for i in range(ship.y, ship.y+ship.length):
if (self.check_for_ship(ship.x-1, i-1) or self.check_for_ship(ship.x, i-1)
or self.check_for_ship(ship.x+1, i-1) or self.check_for_ship(ship.x-1, i)
or self.check_for_ship(ship.x, i) or self.check_for_ship(ship.x+1, i)
or self.check_for_ship(ship.x-1, i+1) or self.check_for_ship(ship.x, i+1)
or self.check_for_ship(ship.x+1, i+1)):
return False
#down
elif ship.direction == 2:
if ship.length > (self.rows - ship.x):
return False
else:
#Check for ships in the neighborhood
for i in range(ship.x, ship.length+ship.x):
if (self.check_for_ship(i+1, ship.y-1) or self.check_for_ship(i+1, ship.y)
or self.check_for_ship(i+1, ship.y+1) or self.check_for_ship(i, ship.y-1)
or self.check_for_ship(i, ship.y) or self.check_for_ship(i, ship.y+1)
or self.check_for_ship(i-1, ship.y-1) or self.check_for_ship(i-1, ship.y)
or self.check_for_ship(i-1, ship.y+1)):
return False
#left
elif ship.direction == 3:
if ship.length > (ship.y+1):
return False
else:
#Check for ships in the neighborhood
for i in range(ship.y, ship.y-ship.length, -1):
if (self.check_for_ship(ship.x-1, i-1) or self.check_for_ship(ship.x, i-1)
or self.check_for_ship(ship.x+1, i-1) or self.check_for_ship(ship.x-1, i)
or self.check_for_ship(ship.x, i) or self.check_for_ship(ship.x+1, i)
or self.check_for_ship(ship.x-1, i+1) or self.check_for_ship(ship.x, i+1)
or self.check_for_ship(ship.x+1, i+1)):
return False
return True
battlefield = Grid(grid_x, grid_y)
ships = []
ship_length = 2
for _ in range(number_of_ships):
ship = Ship(random.randrange(grid_x), random.randrange(grid_y),
random.randrange(4), ship_length)
ships.append(ship)
ship_length += 1
for ship in ships:
valid_ship = battlefield.is_ship_valid(ship)
if valid_ship:
battlefield.place_ship(ship)
else:
while valid_ship == False:
ship.generate_random_ship(grid_x, grid_y, ship.length)
valid_ship = battlefield.is_ship_valid(ship)
battlefield.place_ship(ship)
print(battlefield.grid)
print()
for ship in ships:
ship.print_ship()
2 Answers 2
Naming
The PEP 8 style guide recommends using upper case for constants. For example, change:
grid_x = 10
to:
GRID_X = 10
Enum
Consider using an Enum for the direction variable instead of assigning
the 4 values 0-3. This will make the code self-documenting and eliminate the
need for comments like these:
if ship.direction == 0: #up
#up
if ship.direction == 0:
__str__
It is more common to use the __str__ function for printing a class, instead of
creating a print_ship function:
def __str__(self):
return f"x: {self.x}, y: {self.y}, direction: {self.direction}, length: {self.length}"
Instead of calling print, just return the string. Note that I simplified
the string using an f-string.
Then, instead of calling:
ship.print_ship()
you can simply print the class handle:
print(ship)
Documentation
You should add a dcostring to the top of the code to summarize its purpose. You can start by copying a lot of the text from the question:
"""
Battleship Game
The goal of the code is limited to the creation of the initial map only
and not to the whole game mechanics itself.
"""
The PEP-8 guide also recommends doctrings for classes and functions. It would definitely be beneficial to explain the distinction between these 2 similarly-named functions:
def check_for_ship(self, x, y):
def is_ship_valid(self, ship):
Tools
There is some inconsistent whitespace usage in the code. The black program can be used to automatically format the code with consistency.
This has the benefit that it splits all the or expressions in the is_ship_valid
function out onto separate lines. When viewing the code this way, it looks
like the code for directions 0 and 2 might be duplicated. If that is the case,
then there may be an opportunity for simplification.
I understand why you need 2 different orientations for the ships (horizontal and vertical), but it is not clear why you need 4 directions. I don't think it matters much if a horizontal ship is pointed to the right or the left, for example.
ruff finds this:
E712 Avoid equality comparisons to `False`; use `if not valid_ship:` for false checks
|
| battlefield.place_ship(ship)
| else:
| while valid_ship == False:
| ^^^^^^^^^^^^^^^^^^^ E712
|
= help: Replace with `not valid_ship`
Use dataclasses.dataclass
class Ship:
def __init__(self, x, y, direction, length):
self.x = x
self.y = y
self.direction = direction
self.length = length
This repetitive declaration and assignment code can be replaced with a dataclass, and then you can include the other methods afterward:
import dataclasses
@dataclasses.dataclass
class Ship:
x: int
y: int
direction: int
length: int
def generate_random_ship ...
def print_ship ...
Don't mutate
def generate_random_ship(self, rows, columns, length):
self.x = random.randrange(rows)
self.y = random.randrange(columns)
self.direction = random.randrange(4)
self.length = length
The design of this method is questionable - it changes the field values of this existing ship. It is much clearer to make it a static method that creates a new ship:
class Ship:
@staticmethod
def generate_random_ship(rows, columns, length):
return Ship(
x = random.randrange(rows),
y = random.randrange(columns),
direction = random.randrange(4),
length = length)
Pattern matching
if ship.direction == 0: #up
self.grid[ship.x - i][ship.y] = 1
elif ship.direction == 1: #right
self.grid[ship.x][ship.y + i] = 1
elif ship.direction == 2: #down
self.grid[ship.x + i][ship.y] = 1
elif ship.direction == 3: #left
self.grid[ship.x][ship.y - i] = 1
If you're comfortable with new features, you can reduce repetition by using structural pattern matching in Python 3.10+:
match ship.direction:
case 0: #up
self.grid[ship.x - i][ship.y] = 1
case 1: #right
self.grid[ship.x][ship.y + i] = 1
case 2: #down
self.grid[ship.x + i][ship.y] = 1
case 3: #left
self.grid[ship.x][ship.y - i] = 1
Multiple simplifications
def check_for_ship(self, x, y):
if x >= self.rows or x < 0 or y >= self.columns or y < 0:
return False
elif self.grid[x][y] == 1:
return True
else:
return False
The top if condition is equivalent to: not (0 <= x < self.rows and 0 <= y < self.columns). This is a few characters shorter and takes advantage of chained comparisons.
Next, it's usually bad form to write if (...): return True; else return False; it's better to just return the Boolean expression directly. Hence, we can rewrite the entire method as:
def check_for_ship(self, x, y):
return (
0 <= x < self.rows and
0 <= y < self.columns and
self.grid[x][y] == 1)
Of course, this relies on the fact that and is short-circuiting, so this guarantees no out-of-bounds accesses on self.grid.
Dedent after early return
if ship.length > (ship.x + 1):
return False
else:
#Check for ships in the neighborhood
for i in range(ship.x, ship.x-ship.length, -1):
You can optionally reduce a level of code indention after an early return, like this:
if ship.length > (ship.x + 1):
return False
#Check for ships in the neighborhood
for i in range(ship.x, ship.x-ship.length, -1):
Shorten code by generalizing
def is_ship_valid(self, ship):
...
Oh boy is this one long method with lots of duplication. I'm just going to rewrite the whole thing. Warning - my code is untested - might have bugs and might behave differently from yours:
def is_ship_valid(self, ship):
dirx = [0, 1, 0, -1][ship.direction]
diry = [-1, 0, 1, 0][ship.direction]
for i in range(ship.length):
x = ship.x + i * dirx
y = ship.y + i * diry
if not (0 <= x < self.columns and 0 <= y < self.rows):
return False
for neighx in range(-1, 2):
for neighy in range(-1, 2):
if self.check_for_ship(x + neighx, y + neighy):
return False
return True