Battleship in Python TKinter

Question

I am very new to GUI programming, and I'm pretty sure I'm doing it wrong. Please take a look at my code, and suggest some changes. I feel it's way too complicated for what it's actually doing.

For example, here is my main class:

'''
Written by blacksheep
March 4, 2013
'''

#################
#   IMPORTS     #
#################

from Tkinter import *
from ship_model import Ship, ShipLoader
import time
from grid_model import GridModel
from ship_ai import ShipAI
from ship_placement_panel import ShipPlacementPanel
from ship_grid import ShipGrid
from ship_war_panel import ShipWarPanel
from ship_panel import ShipPanel
from player_controller import PlayerController

#################
#   MAIN CLASS  #
#################

class Game(Frame):
    '''Top-level Frame managing top-level events. Interact directly with user.'''

    ############ geometry ###############
    X_PADDING = 25
    Y_PADDING = 25
    SHIP_PANEL_WIDTH = 150
    #####################################

    ########### states ##################
    PLACING = 0
    PLAYING = 1
    GAME_OVER = 2
    #####################################

    ############ players ################
    AI_PLAYER = 0
    HUMAN_PLAYER = 1
    #####################################

    def __init__(self, master):
        '''Create the UI for a game of battleship.'''

        Frame.__init__(self, master)

        self._create_ui()

        # these are 'controller' elements that should really be in another class
        self.ai = ShipAI(self._their_grid._model, self._my_grid._model)
        self.reset()

    def _create_ui(self):
        '''Create all UI elements for the game.'''

        self._add_grids()
        self._add_staging_panel()
        self._add_ship_panels()
        self._make_buttons()

        # here 50 is an estimate for the size of the button
        self.config(height=self.Y_PADDING * 3 + self._my_grid.size + 50)
        self.set_all_bgs("white", self)

    def _add_staging_panel(self):
        '''Create the placement/ship staging panel.'''

        self._my_grid_frame._staging_panel = ShipPlacementPanel(self)
        self._my_grid_frame._staging_panel.place(
            x=self.X_PADDING * 2 + self.SHIP_PANEL_WIDTH + self._my_grid.size,
            y=self.Y_PADDING
        )

    def set_all_bgs(self, color, parent):
        '''Set all the backgrounds of the child widgets to a certain color.'''

        parent.config(background=color)

        for child in parent.winfo_children():
            self.set_all_bgs(color, child)

    def _add_ship_panels(self):
        '''Add a list of ships to select from, for adding.
        Note that staging area must be added FIRST'''

        ############################## ShipPanel ########################
        self._my_grid_frame._ship_panel = ShipPanel(self)
        self._my_grid_frame._ship_panel.place(x=self.X_PADDING, y=self.Y_PADDING * 4)

        for ship in Ship.SHORT_NAMES:
            self._my_grid_frame._ship_panel._ship_buttons[ship].config(command=self._stage_current_ship)

        self.unselect_ship()
        ##################################################################

        ###################### ShipWarPanel ##############################
        self._my_grid_frame._ship_war_panel = ShipWarPanel(self)
        self._my_grid_frame._ship_war_panel.place(x=self.X_PADDING, y=self.Y_PADDING * 2)
        ##################################################################

        ###################### ShipWarPanel for Adversary ################
        self._their_grid_frame._ship_panel = ShipPanel(self)
        self._their_grid_frame._ship_panel.place(x=self._my_grid.size * 2 + self.X_PADDING * 3 + self.SHIP_PANEL_WIDTH, y=self.Y_PADDING * 4)
        ##################################################################

    def unselect_ship(self):
        '''Deselect all ships in the placement and staging GUIs.'''

        self._my_grid_frame._ship_panel._ship_var.set(10)
        self._my_grid_frame._staging_panel.reset()

    def _stage_current_ship(self):
        '''Stage the currently selected ship.'''

        if self.get_current_ship() is not None:
            # the x and y coordinates don't matter in this case
            # stage the ship vertically by default
            s = Ship(0, 0, self.get_current_ship(), True)
            self._my_grid_frame._staging_panel.add_ship(s)

    def _hide_frame(self, frame):
        '''Since you can't hide a frame per se, 'unpack' the frame's child widgets.
        WARNING: this removes all packing directions for children'''

        frame.lower()

        for child in frame.winfo_children():
            child.pack_forget()

    def process_state(self):
        '''Simple state controller to enable and disable certain widgets depending on the state.
        For now, there are 2 states:
            - 0: ship placement
            - 1: playing battleship with opponent
        '''

        if self._state == self.PLACING:
            self.config(width=self.X_PADDING * 3 + self._my_grid.size + self.SHIP_PANEL_WIDTH + self._my_grid_frame._staging_panel.CANVAS_WIDTH)
            # show staging panel
            self._my_grid_frame._staging_panel.pack_ui()
            self._my_grid_frame._staging_panel.lift(aboveThis=self._their_grid_frame)

            # enable placement
            self._my_grid_frame._autoplace_button.config(state=NORMAL)

            self._play_game_button.config(state=DISABLED)
            self._hide_frame(self._their_grid_frame)

            self._hide_frame(self._my_grid_frame._ship_war_panel)
            self._my_grid_frame._ship_panel.lift(aboveThis=self._my_grid_frame._ship_war_panel)

            # allow the AI to place ships
            self.ai.place_ships()
        elif self._state == self.PLAYING:
            self.config(width=self.X_PADDING * 4 + self._my_grid.size * 2 + self.SHIP_PANEL_WIDTH * 3)
            self._my_grid._model.finalize()
            self._their_grid._model.finalize()
            self._hide_frame(self._my_grid_frame._staging_panel)

            self._their_grid.config(state=NORMAL)
            self._their_grid.enable()

            for ship in Ship.SHORT_NAMES:
                self._their_grid_frame._ship_panel.set_placed(ship)

            self._play_game_button.config(state=DISABLED)

            # disable placement
            self._my_grid_frame._autoplace_button.config(state=DISABLED)

            self.unselect_ship()

            self._my_grid_frame._ship_war_panel.pack_ui()
            self._my_grid_frame._ship_war_panel.lift(aboveThis=self._my_grid_frame._ship_panel)

            # show opponent's grid
            self._their_grid_frame.lift(aboveThis=self._my_grid_frame._staging_panel)
            self._their_grid_label.pack()
            self._their_grid.pack(side=LEFT, pady=20)
        elif self._state == self.GAME_OVER:
            # disable everything except for the reset button
            self._their_grid.disable()
            self.master.title("Battleship (Game Over)")
            self.show_game_over_popup()
            print "GAME OVER"
            print "The %s player won" % self.get_winning_player()

    def show_game_over_popup(self):
        '''Show a popup with a dialog saying the game is over, and showing the winning player.'''

        popup = Toplevel(self)
        popup.title("Game Over")
        f = Frame(popup, width=500)
        #f.pack_propagate(0)
        f.pack()

        if self._winner == self.HUMAN_PLAYER:
            msg = Message(f, text="You win!")
        else:
            msg = Message(f, text="Game over. You lose.")
        msg.pack()
        b = Button(f, text="OK", command=popup.destroy)
        b.pack()


    def get_winning_player(self):
        '''Return textual representation of winning player.'''

        return {
            self.HUMAN_PLAYER: "human",
            self.AI_PLAYER : "ai"
        } [self._winner]

    def _process_ai_shot(self):
        '''Get the shot from the AI.
        Process the given shot by the AI.
        Return the result of the shot'''

        shot = self.ai.get_shot()
        tag_id = self._my_grid._get_tile_name(*shot)
        id = self._my_grid.find_withtag(tag_id)[0]
        result = self._my_grid.process_shot(id)

        if result == Ship.HIT or result == Ship.SUNK:
            self._set_ship_hit(self._my_grid._model.get_ship_at(*shot))

        if result == Ship.SUNK:
            self._set_ship_sunk(self._my_grid._model.get_sunk_ship(*shot).get_short_name())

            if self._my_grid._model.all_sunk():
                self._winner = self.AI_PLAYER

        # update the AI with the shot's result
        self.ai.set_shot_result(result)

        return result

    def _shot(self, event):
        '''Process a shooting event.
        event should be the Tkinter event triggered by tag_bind
        This is a callback function.'''

        id = self._their_grid.find_withtag(CURRENT)[0]
        # here we can safely process the shot
        result = self._their_grid.process_shot(id)
        # disable square regardless of result=
        self._their_grid.itemconfig(id, state=DISABLED)
        shot = self._their_grid._tiles[id]

        if result == Ship.SUNK:
            ship = self._their_grid._model.get_sunk_ship(*shot)
            self._their_grid_frame._ship_panel.set_sunk(ship.get_short_name())

            if self._their_grid._model.all_sunk():
                self._winner = self.HUMAN_PLAYER

        if result != Ship.HIT and result != Ship.SUNK:
            # disable opponent's grid during their turn
            result = Ship.NULL
            self._their_grid.disable()
            while result != Ship.MISS and self._winner is None:
                result = self._process_ai_shot()

            # re-enable their grid
            self._their_grid.enable()

        if self._winner is not None:
            self._state = self.GAME_OVER
            self.process_state()

    def _add_grid_events(self):
        '''Add events to the grids.'''

        self._their_grid.tag_bind("tile", "<Button-1>", self._shot)

    def _add_grids(self):
        '''Create UI containers for the player grids.'''

        self._my_grid_frame = PlayerController(self)
        self._my_grid_frame.place(x=self.X_PADDING + self.SHIP_PANEL_WIDTH, y=self.Y_PADDING)
        l1 = Label(self._my_grid_frame, text="Your Grid")
        l1.pack()
        self._my_grid = ShipGrid(self._my_grid_frame, True)
        self._my_grid.pack(side=LEFT, pady=20)

        self._their_grid_frame = PlayerController(self)
        self._their_grid_frame.place(x=self._my_grid.size + self.X_PADDING * 2 + self.SHIP_PANEL_WIDTH, y=self.Y_PADDING)
        self._their_grid_label = Label(self._their_grid_frame, text="Opponent's Grid")
        self._their_grid_label.pack()
        self._their_grid = ShipGrid(self._their_grid_frame, False)
        self._their_grid.pack(side=LEFT, pady=20)

        self._add_grid_events()

    def reset(self):
        '''New game!'''

        self.master.title("Battleship")
        self._winner = None

        # reset both grids
        self._my_grid.reset()
        self._their_grid.reset()

        # reset selected ship
        self.unselect_ship()

        # reset staging area
        self._my_grid_frame._staging_panel.reset()

        # reset AI
        self.ai.reset()

        # reset indicators on ships in panels
        self._my_grid_frame._ship_war_panel.reset()
        for ship, button in self._my_grid_frame._ship_panel._ship_buttons.iteritems():
            button.config(foreground="black")

        self.ai.read_stat_model("stat")
        self._set_ships = {ship : False for ship in Ship.SIZES.keys()}

        for x, y in self._my_grid.get_tiles():
            self.reset_closure(x, y)

        self._state = self.PLACING
        self.process_state()

    def reset_closure(self, x, y):
        '''Add a placement event to the given tile.
        TODO this is badly named'''

        tag_id = self._my_grid._get_tile_name(x, y)
        c = self.get_add_ship_callback()
        f = lambda event: self.add_staged_ship(x, y, c)
        self._my_grid.tag_bind(tag_id, "<Button-1>", f)

    def add_staged_ship(self, x, y, callback):
        '''Take the stage from the staging area, and place it on the board at position (x, y).
        After ship has been placed, execute the function <callback>.'''

        s = self._my_grid_frame._staging_panel.get_staged_ship()

        if s is not None:
            self._my_grid.add_ship(x, y, s.get_short_name(), s.is_vertical(), callback)

    def get_add_ship_callback(self):
        '''Return the callback function for adding a ship.'''

        return lambda: self.ship_set(self.get_current_ship())

    def _set_ship_sunk(self, ship):
        '''This is a callback, to be called when a ship has been sunk.
        TODO for now only called when one of MY ships is sunk.
        UI shows that the given ship has been sunk.'''

        self._my_grid_frame._ship_panel.set_sunk(ship)

    def _set_ship_hit(self, ship):
        '''This is a callback, to be called when a ship has been hit.
        TODO for now only called when one of MY ships is hit.
        UI shows that the given ship has been hit.'''

        self._my_grid_frame._ship_war_panel.update(ship)

    def ship_set(self, ship):
        '''This is a callback, to be called when a ship has been placed.
        UI shows that the given ship has been placed.'''

        self._set_ships[ship] = True
        self._my_grid_frame._ship_panel.set_placed(ship)

        if all(self._set_ships.values()):
            self._play_game_button.config(state=NORMAL)

    def get_current_ship(self):
        '''Return the current ship.'''

        return self._my_grid_frame._ship_panel.get_current_ship()

    def play_game(self):
        '''Process the event to stop placement and start playing the game.
        '''

        #TODO sanity check

        self._state = self.PLAYING
        self.process_state()

    def auto_place(self):
        '''Automatically place the ships according to a preset configuration.
        This should only be enabled in debugging mode.'''

        ships = ShipLoader.read("sample_ship_config.txt")

        for ship in ships:
            self._my_grid_frame._ship_panel._ship_buttons[ship._type].invoke()
            self._my_grid.add_ship(*ship.coords(), ship=ship._type, vertical=ship._vertical=="v", callback=self.get_add_ship_callback())
        self.unselect_ship()

    def _make_buttons(self):
        '''Create action buttons at the bottom.'''

        button_row = self._my_grid.size + self.Y_PADDING + 40
        button_frame = Frame(self)
        button_frame.place(x=self._my_grid.size - self.X_PADDING, y=button_row)

        reset_button = Button(button_frame, text="Reset", command=self.reset)
        reset_button.pack(side=LEFT, padx=5, pady=5)

        self._play_game_button = Button(button_frame, text="Play", command=self.play_game)
        self._play_game_button.pack(side=LEFT, padx=5, pady=5)

        self._my_grid_frame._autoplace_button = Button(button_frame, text="Auto-place ships", command=self.auto_place)
        self._my_grid_frame._autoplace_button.pack(side=LEFT, padx=5, pady=5)


if __name__ == "__main__":
    app = Tk()
    app.title("Battleship")

    game = Game(app)
    #game.lift(aboveThis=root)
    #game.pack()
    game.pack(fill=BOTH, expand=1)

    app.mainloop()
    pass

It may be hard to read without the custom classes. If anyone asks, I can post some here, but all of them are in my repo. According to the FAQ, I am not allowed to just post the link to the repo without a code snippet, but Here is a link to the full code in my GitHub repo.

Answer 1

1. Suitability for inspection

Tom Gilb and Dorothy Graham (in their book Software Inspection) recommend that you inspect code in batches of about 250 lines. They observe that in practice, as the batch size increases, the quality of the inspection (in terms of number of defects discovered per line of code) falls, because people have limited powers of concentration.

(Consider, for example, how much work was involved in reviewing this 10-line program.)

So it's not realistic for you to ask us to review all 1,800+ lines of your game at once.

It's also important for the author of the code to go through the inspection process themselves, prior to requesting inspection by others. Gilb and Graham write:

If you would like your own work to be inspected, start thinking about any improvements which you may be able to make before your document is ready to be checked. Have you yourself checked it [against the complete set of rules that apply to your work]? You should make sure that your document is as good as you can reasonably make it before it is submitted.

With this advice in mind, I note that your code contains incomplete TODO items: play_game needs a "sanity check", reset_closure "is badly named" and _set_ship_sunk is "for now only called when one of MY ships is sunk". Also, there are comments like "these are 'controller' elements that should really be in another class" and "here 50 is an estimate for the size of the button" that indicate that you know the code is not complete or correct.

My conclusion is that your code is not yet ready for inspection. I would suggest that you go through and fix all the problems that you know about and then re-submit it for inspection when you are ready.

2. Play testing

I played the game and I observed:

1. On the first screen, it's not clear what I should do. It turns out that I have to put my ships on the board and then press "Play". Perhaps an instruction would be good.

2. The interface for placing the ships seems rather awkward. The natural way to manipulate the ships would be to drag them around with the mouse.

3. I would expect some kind of randomness from "Auto-place ships", but every time I press it I get the same layout.

4. There's no outline around the ships, so when they abut in the grid, you can't tell which square belongs to which ship.

5. On the second screen, it's not clear what I have to do. It turns out that I have to click in the opponent's grid to fire a shot. Perhaps an instruction would be good.

6. It's not clear what the buttons on the right or the ship diagrams on the left mean. Perhaps some kind of title is needed? It turns out that the buttons on the right indicate which ships I have sunk. So why are they implemented as buttons?

7. The opponent seems to get multiple shots to my one. At the end of the game, I have made 12 shots (no hits), but my opponent has made 28 shots (17 hits). It looks as though one gets a free shot after a hit. This seems rather surprising: it's not in the Hasbro rules, for example.

8. When you win or lose, a notification box pops up in the corner of the screen saying "You win!" or "Game over. You lose." It would be better to draw this message in the main game window where the player cannot miss it. Also, this window does not disappear when you press "Reset" so that after several game you may find that you have several of these windows stacked up in the corner.

9. Why have "Play" and "Auto-place ships" buttons visible during the game when these are always disabled? Why not remove them?

10. Minor drawing infelicities: the main window has a lot of extra whitespace at the right; the box on the left is missing its right-hand edge; the "Minesweeper" touches the bottom edge of the box; the margins around the grids seem uneven because there are letters and numbers only on the top and left (put these on the bottom and right too, or remove them since they play no role in the game); the buttons at the bottom are not centred in the window.

3. Choice of GUI toolkit

The Python wiki has a comparison of Python GUI toolkits. I'm not an expert on this subject, but the key point in favour of Tkinter is that it's built into Python so that there are going to be no difficulties installing or maintaining it.

The downside is that Tkinter is designed for building mostly-static desktop-style user interfaces. This is fine for turn-based board games like Battleships, but my guess is that as you try to incorporate more dynamic elements into the game, Tkinter's static interface model will become a bit of a constraint. If you find yourself in this situation, you might look at PyGame.

4. Comments on your code

1. The organization of functions could be improved. Your Game class claims to be the "top-level" class managing the whole game, but in fact it has 27 methods, and these are full of fiddly details about layout of GUI elements. I would think seriously about reorganizing this class. Note that there's no need for the Game class to be a Tkinter Frame object. You could make the top-level Frame into another object that's a member of the Game class. I'd suggest an initial refactoring like this:

   class Game(object):
       """
       Battleships game. Maintains the game state and manages the
       interaction with the player and the AI.
       """
       def __init__(self, app):
           self.frame = MainFrame(app)
   
   class MainFrame(Frame):
       """
       Top-level window for the Battleships game.
       """
   

   This would still leave quite a lot of methods in each of these classes, so you'd want to think about further refactoring.

2. In _add_grids the code for creating the two grids is similar. It would be nice to parameterize this: for example instead of having _my_grid and _their_grid, have _player_grid[HUMAN_PLAYER] and _player_grid[AI_PLAYER]. Then it should be possible to refactor some of this duplicated code into loops over the _player_grid array.

   Similar remarks apply to _my_grid_frame and _their_grid_frame.

   This suggests that you might consider a futher refactoring step: have an abstract Player class (with concrete AIPlayer and HumanPlayer subclasses) which has grid and a grid_frame members.

3. If you look at _process_ai_shot and _process_human_shot you'll see that although these share a couple of lines of code, these are quite different operations. It's normally good practice to implement a game in such a way that player actions are processed in exactly the same way as AI actions. This reduces the amount of code, and ensure that the same rules are being applied to the player and AI. (Obviously the results of the action may be presented in different ways to the player depending on whose action it is.)

4. You use a lot of "private" names (starting with underscores). This would be important if you were writing code with a public interface (for example, a general-purpose library), but here all the code is yours: there's no distinction between public and private interface. So it's probably not worthwhile spending the effort to make this distinction. In any case, the distinction does not seem to be applied systematically: for example, set_all_bgs, process_state, show_game_over_popup, get_winning_player, reset_closure, add_staged_ship, get_add_ship_callback (among other methods) are called only from methods on the Game class, but lack the initial underscore.

5. You have docstrings for your methods (good!), but they are sometimes a bit cryptic, and sometimes fail to explain the role of the arguments, what the function does, or what result is returned. For example:

   def _process_human_shot(self, id):
       '''Given the shot from the human player, react to it.
       Return the result.'''
   

   The questions that this docstring leaves unanswered are:

   1. What is the role of the argument id? (I think it's the Tk handle for the square that was shot.)
   2. In what way does the method "react to it"? (It disables the square, sinks ships on that square, and updates the game over condition.)
   3. What kind of result is returned? (It's one of the enumerated values Ship.MISS, Ship.HIT, Ship.SUNK.)

6. In the constructor for your Game class, you call the constructor for the superclass Frame directly:

   Frame.__init__(self, master)
   

   This looks odd to programmers like me who are used to modern Python's new-style classes, so I would put a comment:

   # Can't use super() since tkinter.Frame is an old-style class.
   Frame.__init__(self, master)
   

7. Your mechanism for implementing callback functions is over-complicated. Instead of writing a method that returns a callback:

   def get_add_ship_callback(self):
       '''Return the callback function for adding a ship.'''
   
       return lambda: self.ship_set(self.get_current_ship())
   

   you can write the callback directly as a method:

   def add_current_ship(self):
       '''Place current ship.'''
       self.ship_set(self.get_current_ship())
   

   and then use a "method object" as the callback:

   c = self.add_current_ship
   

8. It's slightly outside the scope for this review, but I think that some of the trouble you have to go to with these callbacks is caused by the fact that you don't have objects representing tiles in the grids.

   Suppose for example that a Grid object contains a bunch of Tile objects. The grid's get_tiles method generates the tile objects (instead of coordinate pairs, as now). Each Tile object could have methods giving the grid it belongs to, its coordinates, which ship (if any) is placed on that tile, and a callback function for handling mouse clicks. This would simplify a lot of the tile-handling code. (Anywhere you currently have a grid, x, y triple you could have a Tile object instead.)