3
\$\begingroup\$

I present to you... my battleship location picker! This will be part of a larger battleship program. The coordinates that are important are 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'.join(' '.join(line) for line in board)
        print(c.clear + display_board) #c.clear clears the screen
        return board
def hide(board,coords):
    for coord in coords:
        board[coord[1]][coord[0]] = '_'
    return board

def choose_locations(board):
    ships = [Ship(5,'A'),Ship(4,'B'),Ship(3,'c'),Ship(3,'C'),Ship(2,'D')] 
    finalcoords = []
    for ship in ships:
        board = ship.show(board,hasprevious=False)
        while True:
            key = getch.getch()
            if key == 'w':
                ship.shift('U')
                board = ship.show(board)
            elif key == 's':
                ship.shift('D') 
                board = ship.show(board)
            elif key == 'a':
                ship.shift('R') 
                board = ship.show(board)
            elif key == 'd':    
                ship.shift('L') 
                board = ship.show(board)
            elif key == 'r':
                ship.rotate()
                board = ship.show(board)
            elif key == 'q':
                exit()
            elif key == '\r':
                intersection = list(set(ship.coords).intersection(finalcoords))
                if intersection == []:
                    finalcoords += ship.coords
                    board = hide(board,ship.coords)
                    break
                else:
                    print('''You already have a ship or part of a ship there.Your current coordinates are {}
You have placed a ship on these coords, {}. 
You overlapped on these coords: {}'''.format(ship.coords,finalcoords,intersection))
            else:
                print('press w,a,s or d to move, and r to rotate. Press q to quit.')
    return finalcoords
if __name__ == '__main__':                
    coords = choose_locations(board)
\$\endgroup\$
  • 2
    \$\begingroup\$ What is skilstak? I'm not familiar with that library. \$\endgroup\$ – Mast Feb 10 '17 at 21:12
  • \$\begingroup\$ it's something somebody I know made. the actual package is solarized. \$\endgroup\$ – lol Feb 10 '17 at 22:27
5
\$\begingroup\$

Here are some observations:

  • 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 as if 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 size an 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)
      
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.