I am learning how to program and during the Python course on Codecademy I was encouraged to rewrite it all with new features (2 players, various ship sizes, etc.).

In summary this works as follows:

  1. Board configuration is set
  2. Players and their deployment is initialized
  3. Deployment info is exchanged between players
  4. Each player takes a turn to look at the state of his boards and to take a shot. When all ship tiles are shot, the opposite player wins.

My question is (if anyone got patience to dig through it), how can I improve my coding skills, style and simplify this? Is it good idea to trash it and rewrite with ships and boards as classes?

from textwrap import dedent
from copy import copy

class Player():

    def alternate_pixel(self, index, mode):
        """This method, after receiving a call with pixel index
        will change its graphical representation. Depending on mode ofc.
        Also, this function does not mess with active_indexes."""
        # print('alternating %d' % index)
        if mode == 'deploy':               # . - empty, free space
            self.own_board[index] = '$'  # $ - healthy ship part
        elif mode == 'get miss':
            self.own_board[index] = ' '  #   - hit in empty space
        elif mode == 'get hit':
            self.own_board[index] = 'X'  # X - destroyed ship part
        elif mode == 'make miss':
            if self.shooting_board[index] == 'X':   # do not override X
            self.shooting_board[index] = ' '
        elif mode == 'make hit':
            self.shooting_board[index] = 'X'

    def show_board(board):
        """Processes list of pixels into graph, without messing with them"""
        temp = copy(board)
        c = -1      # compensate for inserted '\n\t' units
        for i in range(board_size):
            c += 1
            temp.insert((i+1)*board_size + c, '\n\t')
        print('\t ' + ' '.join(temp))

    def are_indexes_allowed(occupied, proposed):
        """Periodicity (being on two ends), out of board deploys and
        stacking on tile is disallowed."""
        for i in range(1, board_size):
            if i*board_size - 1 in proposed and i*board_size in proposed:
                return False
        for index in proposed:
            if index not in range(board_size**2) or index in occupied:
                return False
        return True

    def ship_settler(self, ship_size):
        """The back end to deploy_fleet(). This will put given ship size on
        board(horizontaly or vertically)."""
        while True:
            nose_index = int(ask_for_coordinates())
            if nose_index not in self.active_indexes: break  # just faster

        if ship_size == 1:  # mono ships do not need to face the incoming
            Player.alternate_pixel(self, nose_index, 'deploy'); return

        proposed_indexes = []
        direction = input('NSWE?\n')

        for i in range(ship_size):      # for each hull segment
            if direction == 'N' or direction == 'n':
                proposed_indexes.append(nose_index - i*board_size)
            if direction == 'S' or direction == 's':
                proposed_indexes.append(nose_index + i*board_size)
            if direction == 'W' or direction == 'w':
                proposed_indexes.append(nose_index - i)
            if direction == 'E' or direction == 'e':
                proposed_indexes.append(nose_index + i)

        if Player.are_indexes_allowed(self.active_indexes, proposed_indexes):
            for index in proposed_indexes:  # run the updates
                Player.alternate_pixel(self, index, 'deploy')
            print('Invalid, try again.')    # if not met - rerun
            del proposed_indexes [:]    # if not emptied it will stay filled
            Player.ship_settler(self, ship_size)

    def deploy_fleet(self):
        """The front end function that fills active_indexes"""
                Deployment phase of %s\n\
             ------------------------------\n' % (self.name)))
        ship_size = 0   # going form smallest to largest

        for ship_amount in ship_list:
            ship_size += 1

            for i in range(ship_amount):    # for each ship of size
                print('Deploying %d sized ship %d/%d\n'
                % (ship_size, i+1, ship_amount))
                Player.ship_settler(self, ship_size)
        Player.show_board(self.own_board)       # refresh board at the end
        input('Your deployment phase has finished.')

    def __init__(self, name):
        """Each player has 2 boards, first shows own ships and tiles shot by
        the opponent, while second shows the same about him, but active ships
        are hidden. Note that list of active_indexes (which marks non destroyed
        ship parts) has no effect on graphics (in the shooting phase)"""
        self.name = name
        self.active_indexes = []  # filled by deploy(), emptied by get_shot()
        self.own_board = ['.'] * board_size**2
        self.shooting_records = []
        self.shooting_board = copy(self.own_board)
        Player.deploy_fleet(self)       # deployment is part of initialization
        print(chr(27) + "[2J")  # clear terminal screen

    def send_deployment_info(self):
        """Some method of trasfering info whether player hit somthing must be
        in place. This seems suboptimal and maybe it could be resolved
        diffrently, but perhaps rebuilding is nesscesary to do so."""
        active_indexes = copy(self.active_indexes)
        return active_indexes

    def receive_deployment_info(self, opponent_indexes):
        self.opponent_indexes = opponent_indexes

    def make_shot(self):
        """Essentially handles players turn"""
            \tFiring phase of %s\n\
            --------------------------------------\n' % (self.name)))
        while True:               # cannot strike same tile twice
            index = ask_for_coordinates()
            if index not in self.shooting_records: 

        if index in self.opponent_indexes:      # if guessed right
            print('You got him!')
            self.alternate_pixel(index, 'make hit')
        else:                                   # if guessed wrong
            self.alternate_pixel(index, 'make miss')
            print('Bad luck!')

        Player.show_board(self.shooting_board)  # refresh the board
        input('Your turn has finished')
        print(chr(27) + "[2J")      # clears the terminal window
        return index            # pass shot coordinate

    def get_shot(self, index):
        """This has nothing to do with input, it only displays result
        of opponents turn."""
            \tRaport phase phase of %s\n\
            --------------------------------------\n' % (self.name)))
        if index in self.active_indexes:       # if got hit
            self.alternate_pixel(index, 'get hit')
            print('The opponent got your ship hit!\n')
            self.active_indexes.remove(index)   # for finishing the game
        else:                                   # if evaded
            self.alternate_pixel(index, 'get miss')
            print('You have evaded the shot!\n')

def configure_board_size():
           'What size of board would you like to play on?\n\
            Expected range: 4 to 20')
    size = input()
    if size.isdigit():
        if 4 <= int(size) <= 20:
            return int(size)
    print('Invalid board size')
    return configure_board_size()

def ask_for_ship_type_amount(ship_size, space_avaible):
    """Makes sure that there wont be much problems with fitting ship
    coordinates in deployment phase. It is ensured bu restricting
    the amount of ships size based on left free space."""
    if ship_size > space_avaible:   # speeds up prompting
        return 0
    value = input('How much ships sized %d are to be placed?\n' % ship_size)
    if value.isdigit():
        if int(value)*ship_size <= space_avaible:
            return int(value)
    print('Invalid amount')
    return ask_for_ship_type_amount(ship_size, space_avaible)

def configure_ships(board_size):        # gets called second
    """Gets called second and with help of ask_for_ship_type_amount
    generates list of ships to be placed in deployment phases.
    This ship_list stores amount of each next (in size) ship."""
    ship_list = []
    space_avaible = ((board_size)**2) // 2.2  # preserve arbitrary 60% freespace
    print('Generating ships, ')
    for i in range(1, board_size):
        value = ask_for_ship_type_amount(i, space_avaible)
        space_avaible -= i*value    # each next placed ship takes space
        ship_list.append(value)     # this also stores board_size indirectly
    return ship_list

def ask_for_coordinates():
    """Asks for row and column and outputs the index"""
    coords = str(input('Select tile\n'))
    if coords.isdigit() and len(coords) == 2:
            if 0 < (int(coords[0]) and int(coords[1])) <= board_size:
                index = int(coords[0]) + board_size * (int(coords[1])-1)
                return index - 1      # the above doesnt account for 0th item
    print('Invalid coordinates')
    return ask_for_coordinates()

def main():

    global board_size; global ship_list
    board_size = configure_board_size()
    ship_list = configure_ships(board_size)
    #ship_list = [0, 2, 0]
    #board_size = len(ship_list) + 1

    a = Player('A')
    b = Player('B')
    b.receive_deployment_info(a.send_deployment_info())  # deployment info exch.
    #print(a.active_indexes, b.active_indexes)

    while True:
        a.get_shot(b.make_shot())    # Player B turn
        if not a.active_indexes:
            print('B wins!'); break
        b.get_shot(a.make_shot())    # Player A turn
        if not b.active_indexes:
            print('A wins!'); break

  • 1
    \$\begingroup\$ This is good code. Minor: in alternate_pixel() you use if/elif/elif/... whereas in ship_settler() in for i in range(ship_size): you use if without elif (the conditions are mutually exclusive in both cases). Consider dropping the elif from alternate_pixel() or use case/switch in both cases. \$\endgroup\$
    – user1149
    Commented Nov 5, 2017 at 13:06
  • \$\begingroup\$ @BarryCarter Comments are for seeking clarification to the question, and may be deleted. Please put all suggestions for improvements in answers. \$\endgroup\$ Commented Nov 9, 2017 at 6:17

1 Answer 1


I got two basic things to say

You got a typo, in 'Raport' I guess it should be 'Report'. (Actually runned the code, that doesn't count as a thing)

       Raport phase phase of A

1.- I didn't go through the implementation just readed your post and actually runned the code. OOP is supposed to model the world and help us code. In the world you have a single board and two players. Instead of that you got 2 players and 4 boards this is weird. I'm not saying that you should make an object named Board, it could be a just a list but players should not have knowledge of their opponents information. Like in the real world you dont have information about your opponents ships.

2.- You are doing a weird thing when calling methods inside your classes, for instance: (Got rid of some code for redability)

class Player:
def __init__(self, name):
    Player.deploy_fleet(self)       # THIS LINE IS THE PROBLEM

This is a problem because you are not calling 'your' method you are calling the method defined in Player class with 'you' as an argument. This is not how you call your own methods in a class, you do it like this:

class Player:
def __init__(self, name):
    self.deploy_fleet()       # Ou yeah! Much better

You may think it is the same but it isnt. It may be a little bit complex for the level you are in right now (I'm guessing here), but this becomes trouble when someone tries to inherit from your Player class. And again, this may be complex but if you understand it, it is a pretty good tool to have in your back.

I'll try to demostrate it with a little example, when 'you' inherit from a class suddently you become that class, you have the same attributes and the same methods (well in python a method is actually an attribute, but that is WAY out of the scope). And you can choose whether or not you keep your "parent's" attributes, if you dont like it, you can define your own, lets see with an example.

Yes, I'm sorry, but here we go with the animals example.

class Animal:
    def __init__(self, name):
        self.name = name
        Animal.talk(self)  # This is not supposed to be done this way, you'll see
    def talk(self):
        print('Animal talking ' + self.name)

I think a dog is also an animal right? So lets do it!

class Dog(Animal):
    def talk(self):
        print('Dog talking ' + self.name)

I dont like a dog saying that it's just an animal, he's a dog (man's best friend), so I override the method talk while I let the method __init__ be the same as the Animal class. Let's have a look at what happens...

some_animal = Animal('BaseAnimal')
dog = Dog('ADog')

When I do this, this happens:

Animal talking BaseAnimal
Animal talking ADog

I overided the talk method in the Dog class, but in the __init__ method which Dog inherit from Animal, a hardcoded call to the class Animal is preventing the Dog to call its own talk method. To solve this, we do:

class Animal:
    def __init__(self, name):
        self.name = name
    def talk(self):
        print('Animal talking ' + self.name)

Without touching the Dog class, if we repeat the previous statements:

some_animal = Animal('BaseAnimal')
dog = Dog('ADog')

Now the result is:

Animal talking BaseAnimal
Dog talking ADog

Much better right! This is a pretty complex thing, but to summarize, dont call a method within a class with the name of the class, use self (Unless you are looking for that behaviour.)

Hope it helped!


Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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