patternpythonMinor
Battleship coordinate picker
Viewed 0 times
pickercoordinatebattleship
Problem
I present to you... my battleship location picker! This will be part of a larger battleship program. The coordinates that are important are
```
from operator import itemgetter
import getch
import skilstak.colors as c #colors
BOARDLENGTH = 10
BOARDHEIGHT = 10
board = [['_'] * BOARDLENGTH for _ in range(BOARDHEIGHT)]
class Ship():
def __init__(self, size, symbol):
self.size = size
self.symbol = symbol
self.coords = [(x,0) for x in range(size)]
self.previous_coords = [(None,None) for _ in range(size)]
def shift(self,direction):
x_values = list(map(itemgetter(0), self.coords))
y_values = list(map(itemgetter(1), self.coords))
if direction == 'R' and 0 not in x_values:
change = (-1,0)
elif direction == 'D' and BOARDHEIGHT-1 not in y_values:
change = (0,1)
elif direction == 'U' and 0 not in y_values:
change = (0,-1)
elif direction == 'L' and BOARDLENGTH-1 not in x_values:
change = (1,0)
else:
change = (0,0)
for i, coord in enumerate(self.coords):
self.previous_coords[i] = self.coords[i]
self.coords[i] = (coord[0] + change[0], coord[1] + change[1])
def rotate(self):
a, b = self.coords[0][0], self.coords[0][1]
x, y = self.coords[-1][0], self.coords[-1][1]
newx, newy = -(y-b)+a, (x-a)+b
if newy >= 0 and newx >= 0 and newy <= BOARDHEIGHT-1 and newx <= BOARDLENGTH-1:
for i, coord in enumerate(self.coords):
self.previous_coords[i] = self.coords[i]
self.coords[i] = (-(self.coords[i][1]-b)+a, (self.coords[i][0]-a)+b)
def show(self, board, hasprevious=True):
if hasprevious:
for pcoord in self.previous_coords:
board[pcoord[1]][pcoord[0]] = '_'
for coord in self.coords:
board[coord[1]][coord[0]] = self.symbol
display_board = '\n'
finalcoords.```
from operator import itemgetter
import getch
import skilstak.colors as c #colors
BOARDLENGTH = 10
BOARDHEIGHT = 10
board = [['_'] * BOARDLENGTH for _ in range(BOARDHEIGHT)]
class Ship():
def __init__(self, size, symbol):
self.size = size
self.symbol = symbol
self.coords = [(x,0) for x in range(size)]
self.previous_coords = [(None,None) for _ in range(size)]
def shift(self,direction):
x_values = list(map(itemgetter(0), self.coords))
y_values = list(map(itemgetter(1), self.coords))
if direction == 'R' and 0 not in x_values:
change = (-1,0)
elif direction == 'D' and BOARDHEIGHT-1 not in y_values:
change = (0,1)
elif direction == 'U' and 0 not in y_values:
change = (0,-1)
elif direction == 'L' and BOARDLENGTH-1 not in x_values:
change = (1,0)
else:
change = (0,0)
for i, coord in enumerate(self.coords):
self.previous_coords[i] = self.coords[i]
self.coords[i] = (coord[0] + change[0], coord[1] + change[1])
def rotate(self):
a, b = self.coords[0][0], self.coords[0][1]
x, y = self.coords[-1][0], self.coords[-1][1]
newx, newy = -(y-b)+a, (x-a)+b
if newy >= 0 and newx >= 0 and newy <= BOARDHEIGHT-1 and newx <= BOARDLENGTH-1:
for i, coord in enumerate(self.coords):
self.previous_coords[i] = self.coords[i]
self.coords[i] = (-(self.coords[i][1]-b)+a, (self.coords[i][0]-a)+b)
def show(self, board, hasprevious=True):
if hasprevious:
for pcoord in self.previous_coords:
board[pcoord[1]][pcoord[0]] = '_'
for coord in self.coords:
board[coord[1]][coord[0]] = self.symbol
display_board = '\n'
Solution
Here are some observations:
-
-
variable naming, if defining a constant, there has to be an underscore between words:
-
a space between arguments after a comma:
-
simplify the if condition inside the
with:
-
other improvements:
-
what if instead of multiple repetitive
Then, we can improve the part determining the shift direction:
- no meaningful comments or docstrings (they are not always needed, but imagine there is that "violent psychopath, who knows where you live" will be reading the code)
-
PEP8 violations and making things Pythonic:- blank line between class methods, two blank lines between top-level functions and classes (source)
- correct import organization
- the inline comments formatting
-
variable naming, if defining a constant, there has to be an underscore between words:
BOARDLENGTH -> BOARD_LENGTH
BOARDHEIGHT -> BOARD_HEIGHT-
a space between arguments after a comma:
ship.show(board, hasprevious=False)
HERE^- no need for extra
()after the class definition
if intersection == []:can be rewritten asif not intersection:
-
simplify the if condition inside the
rotate() method, replace:if newy >= 0 and newx >= 0 and newy <= BOARDHEIGHT-1 and newx <= BOARDLENGTH-1:with:
if 0 <= newx <= BOARDLENGTH - 1 and 0 <= newy <= BOARDHEIGHT - 1:-
other improvements:
- looks like you don't need to make the
sizean instance variable - you only need it in the constructor
-
what if instead of multiple repetitive
if/else blocks inside the choose_locations(), you would create a mapping between action keys and the shift directions, e.g.:ACTION_KEYS = {
'w': 'U',
's': 'D',
'a': 'R',
'd': 'L'
}Then, we can improve the part determining the shift direction:
key = getch.getch()
if key in ACTION_KEYS:
ship.shift(ACTION_KEYS[key])
board = ship.show(board)Code Snippets
BOARDLENGTH -> BOARD_LENGTH
BOARDHEIGHT -> BOARD_HEIGHTship.show(board, hasprevious=False)
HERE^if newy >= 0 and newx >= 0 and newy <= BOARDHEIGHT-1 and newx <= BOARDLENGTH-1:if 0 <= newx <= BOARDLENGTH - 1 and 0 <= newy <= BOARDHEIGHT - 1:ACTION_KEYS = {
'w': 'U',
's': 'D',
'a': 'R',
'd': 'L'
}Context
StackExchange Code Review Q#155049, answer score: 5
Revisions (0)
No revisions yet.