Refactoring, refactoring

I've spent some time refactoring the program's code lately. The GUI class is getting really bloated with features and functionality that should be handled by a separate class. Thus, I've created a Menu class to create, bind and query the menu. Pretty good...

Also in the utility class, my save method was growing to over 100 lines, with many temporary variables being used. I managed to extract this into its own class, and split up the method into several smaller ones, passing parameters between them as needed. My temporary variables from the giant class became instance variables in the extracted class.

The end result is more code, but ultimately an easier to read, more abstract higher level save method - you just read the method calls it makes, as opposed to having to iterate over objects, save values etc inside the method.

I need to apply this to many more places in the program. I want to refactor out tab control to its own class, so that it can be shared by the Notes, thumbs and gui classes; these all share common data but it's duplicated per-class. Also the code is really hard to test - refactoring it out will definitely help.

The Law of Demeter

I've been learning a lot about refactoring over the past few days, and have made an effort to put these practices into effect. One interesting concept is the Law of Demeter - classes should know as little about their interacting classes as possible. This means restricting access on instance variable, and method arguments.

Suppose we have an instance variable of type B inside A. Class B contains a reference to class C. The Law of Demeter states we should not access properties of C through B; instead we should tell B on how to interact with C.

Here's a before/after example of some code from Whyteboard I've modified, involving selecting a shape using the Select tool:

class Select:
    def check_for_hit(self, shape, x, y):
        """
        Sees if a shape is underneath the mouse coords, and allows the shape to
        be re-dragged to place
        """
        found = False
        handle = shape.handle_hit_test(x, y)  # test handle before area

        if handle:
            self.handle = handle
            found = True
        elif shape.hit_test(x, y):
            found = True

        if found:
            self.canvas.overlay = wx.Overlay()
            self.shape = shape
            self.dragging = True
            self.offset = self.shape.offset(x, y)

            if self.canvas.selected:
                self.canvas.deselect()
            self.canvas.selected = shape
            shape.selected = True

            pub.sendMessage('shape.selected', shape=shape)
        return found


class Gui:
    # bind a handler for shape.selected
    pub.subscribe(self.shape_selected, 'shape.selected')

    def shape_selected(self, shape):
        """
        Shape getting selected (by Select tool)
        """
        x = self.canvas.shapes.index(shape)
        self.canvas.shapes.pop(x)
        self.canvas.redraw_all()  # hide 'original'
        self.canvas.shapes.insert(x, shape)
        shape.draw(self.canvas.get_dc(), False)  # draw 'new'

        ctrl, menu = True, True
        if not shape.background == wx.TRANSPARENT:
            ctrl, menu = False, False

        self.control.transparent.SetValue(ctrl)
        self.menu.Check(ID_TRANSPARENT, menu)

In this code, we see the Select tool (a "model" is manipulating the canvas directly, when it really shouldn't be doing so. We can delegate this to the canvas itself, through the GUI.

class Select:
    def check_for_hit(self, shape, x, y):
        """
        Sees if a shape is underneath the mouse coords, and allows the shape to
        be re-dragged to place
        """
        found = False
        handle = shape.handle_hit_test(x, y)  # test handle before area

        if handle:
            self.handle = handle
            found = True
        elif shape.hit_test(x, y):
            found = True

        if found:
            self.shape = shape
            self.dragging = True
            self.offset = self.shape.offset(x, y)
            pub.sendMessage('shape.selected', shape=shape)
        return found


class Gui:
    # bind a handler for shape.selected
    pub.subscribe(self.shape_selected, 'shape.selected')

    def shape_selected(self, shape):
        """
        Shape getting selected (by Select tool)
        """
        self.canvas.select_shape(shape)

        ctrl, menu = True, True
        if not shape.background == wx.TRANSPARENT:
            ctrl, menu = False, False

        self.control.transparent.SetValue(ctrl)
        self.menu.Check(ID_TRANSPARENT, menu)


class Canvas:
    def select_shape(self, shape):
        """Selects the selected shape"""
        self.overlay = wx.Overlay()
        if self.selected:
            self.deselect_shape()

        self.selected = shape
        shape.selected = True
        x = self.shapes.index(shape)
        self.shapes.pop(x)
        self.redraw_all()  # hide 'original'
        self.shapes.insert(x, shape)
        shape.draw(self.get_dc(), False)  # draw 'new'

We can now see that the appropriate classes are performing their correct responsibilities. The model sends out a message to indicate something's changed, without caring how the event is handled, and the GUI is updating its controls and menus. The GUI delegates a method call to the canvas which updates itself.

There are no longer many "access levels" (this.that.other.do_something) as all operations are on variables within a small scope. This is better code that's easier to test.