We don’t even have 200 lines of code yet. How can refactoring make any sense already?

This little program started out as a Spike just to get something on the screen. It seems to be drifting toward becoming the production version. But isn’t it way too soon to start refactoring? Well, let’s see. Why do we refactor?

Reasons

Improve the Design

Refactoring, by definition, is the practice of improving the design of existing code, so in principle, any time the design could use a little improvement, refactoring applies. In principle.

Remove Duplication

My personal practice in refactoring is primarily driven by observing and removing duplication. When something is done more than once, it’s a very good sign that there’s a common idea being used, and it’s certainly good practice to create objects and functions that manage commonly-used ideas for us. Many of my betters recommend waiting until the issue has arisen three times before refactoring, but my own practice is worse than that in at least two ways.

First, I often get on a roll and allow duplication to arise far more than three times. That can only make the refactoring job harder, although it might help us see what the idea actually is. But my betters would try not to wait for four or five or eleventy-leven duplications before acting.

Second, I often remove duplication when there are only two duplicates. My betters would argue that this is often too soon, in that there’s not enough evidence regarding what the idea behind the duplication might be. They’re probably right, and I don’t care, because part of my shtick is to make mistakes and show that they’re generally readily corrected without big problems. So if it’s too soon, no problem, we’ll refactor again later.

Express Ideas

Sometimes I’ll refactor even before there’s any duplication, because I’ve noticed that there is a baby idea trying to be born in the code. Once I begin to see that idea, I will often pull it out, plant it somewhere suitable, and try to nurture it until it grows up into a real idea.

Prepare a Spot

Sometimes when we set out to make some change, the code doesn’t seem amenable to the change. It seems almost to resist. One common case is that some change needs to be made in multiple locations. Maybe those locations don’t look like duplication, even though when we do the change, parts of each location will have the change, thus creating duplication. Sometimes if we refactor a bit we can make the installation of our new change easier.

This case doesn’t come up for me very often. I am perhaps a bit insensitive to it, or perhaps I don’t see quite what to do. So I’ll go ahead and make the change, which may then show me how to improve the code according to the other headings here. And sometimes, well, I just let it happen.

Improvement, not Perfection

The point of refactoring is to improve the code, not to make it perfect. Oh, if you can get it perfect quickly, do go for it, but I have never managed perfection. I can often manage improvement, and that makes my job easier, helps me go faster, and makes me feel better about my work.

We’re going to work a lot of hours, and it’s pretty rare for someone else to make us feel good about our work. I figure we have to take care of ourselves, and, for me, doing a decent workmanlike job of programming makes me feel better about myself.

That’s probably the real reason why I do any of this.

Opportunities

Before I look at the code, just from memory, I can think of these things that might use improvement.

  1. The creation of surfaces could be centralized and generalized. We could use the maximum and minimum x and y values of the points arrays to tell us how to size and center our objects, and we could parameterize the adjustment of points by specifying the magnification we want to apply as a parameter.

  2. The objects will all draw themselves similarly, at least once they’ve selected the surface they want to display. That common element might be factored out, if it seems worthwhile.

  3. The objects all move similarly, once they set direction or accelerate: they add their scaled velocity to their position to get a new position. This might be factored out, again if it seems worthwhile.

Now let’s review some code to see what we notice, either under those possibilities or new ones that we might notice.

I’m comparing asteroid and ship side by side in PyCharm:

side by side

The first thing I notice is two very similar methods, adjust, both of which are flagged by PyCharm as being able to be “static”, by which it means they could be moved to the top level. Of course, since they are not identical that would lead to issues:

class Ship ...
    def adjust(self, point):
        center_adjustment = vector2(28, 16)
        return point*4 + center_adjustment

class Asteroid ...
    def adjust(self, point):
        center_adjustment = vector2(4, 4)
        return (point + center_adjustment) * 16

I notice that the two functions, which are used for exactly the same thing, expanding a set of corners to a larger size, are not quite the same. I can make them more similar, changing Ship thus:

    def adjust(self, point):
        center_adjustment = vector2(7, 4)
        return (point + center_adjustment)*4

We could compute the center adjustment: I did it by hand, i.e. by counting on my fingers. The Ship’s points range from -7 to 7 in x and from -4 to 4 in y. The Asteroid ranges from -4 to 4 in both x and y. The adjustment we want in each case is (w/2, h/2) where w and h are the width and height of the pattern provided.

We could compute that from the pattern, and pass it in to this function. For now, let’s just change one of these to accept the parameters (x_adj, y_adj), and scale and go from there. I think I’ll pass in a Vector2, as I seem to be converging on that.

Let’s try a Change Signature and see what happens.

    def adjust(self, point, center_adjustment, scale_factor):
        return (point + center_adjustment)*scale_factor

PyCharm asked me more questions than I wanted to answer but that’s what it gave me. Now to find the calls.

Meh. Have to do something a bit tricky here:

    def get_flare_points(self):
        raw_flare = [vector2(-3.0, -2.0), vector2(-7.0, 0.0), vector2(-3.0, 2.0)]

        def adjust(point): self.adjust(point, vector2(7, 4), 4)
        return list(map(adjust, raw_flare))

    def get_ship_points(self):
        raw_ship = [vector2(-3.0, -2.0), vector2(-3.0, 2.0), vector2(-5.0, 4.0),
                    vector2(7.0, 0.0), vector2(-5.0, -4.0), vector2(-3.0, -2.0)]
        
        def adjust(point): self.adjust(point, vector2(7, 4), 4)
        return list(map(adjust, raw_ship))

Since we’re passing in more parameters, we can’t just refer to adjust, we have to get more than a little tricky. I discover the itertools.repeat function, that just returns the same thing over and over:

    def get_flare_points(self):
        raw_flare = [vector2(-3.0, -2.0), vector2(-7.0, 0.0), vector2(-3.0, 2.0)]
        return list(map(self.adjust, raw_flare, repeat(vector2(7, 4)), repeat(4)))

    def get_ship_points(self):
        raw_ship = [vector2(-3.0, -2.0), vector2(-3.0, 2.0), vector2(-5.0, 4.0),
                    vector2(7.0, 0.0), vector2(-5.0, -4.0), vector2(-3.0, -2.0)]
        return list(map(self.adjust, raw_ship, repeat(vector2(7, 4)), repeat(4)))

This works, though I can’t say that I love it. Let’s rename those two source lists, that might help.

    def get_flare_points(self):
        flare_points = [vector2(-3.0, -2.0), vector2(-7.0, 0.0), vector2(-3.0, 2.0)]
        return list(map(self.adjust, flare_points, repeat(vector2(7, 4)), repeat(4)))

    def get_ship_points(self):
        ship_points = [vector2(-3.0, -2.0), vector2(-3.0, 2.0), vector2(-5.0, 4.0),
                       vector2(7.0, 0.0), vector2(-5.0, -4.0), vector2(-3.0, -2.0)]
        return list(map(self.adjust, ship_points, repeat(vector2(7, 4)), repeat(4)))

This helps, by making it more clear that the thing after self.adjust is a collection, so the map makes a bit more sense.

This is marginally better, and I’ll commit it on that basis, but then we need to reflect about where we’re going. Commit: refactoring adjust on way to improving surface creation.

Reflection

OK. Where do we really want to go here? I think we have a few related possible goals:

  1. Provide a single adjust and use it in all surface creation. The new one we just did will clearly work for Asteroid and probably for any other usage.
  2. Calculate the center adjustment automatically.

    (See below.)

  3. Move all the surface creation over to some new object that takes points and perhaps a scale factor and gives us back a surface all nice and pretty.

What can we say about the center adjustment? We happen to know that our input points are all centered around (0,0), and we could take advantage of that to use (width/2,height/2). We could also just record the minimum x and y values and adjust by the negative of those, moving the least values to top left. It turns out that either of those calculations would give us the same result, if I’m not mistaken. In our program, I’d use the former. In a general program with pictures that aren’t centered … the latter calculation might be more general. But we’re not in a general program. No point building something that we don’t need.

Oh! It turns out that we can’t quite calculate the adjustment anyway: the ship only extends from -7 to 7 when the flare is showing. Otherwise it spans -5 to 7. I’m glad I stopped to think: it so often pays off.

I think what I’ll do is this:

  1. Move the points definitions for the ship and asteroid off to a file by themselves, where we’ll put the saucer when we do it.
  2. Move the expanded adjust function over there.
  3. See whether we can create a class over there, and use it.

If this pays off, we’ll get a start at the surface making object. If it doesn’t, we’ll still have things together. I’ll tell Python that I’m creating a class, and I’ll put the data into it, probably make the adjust a method even if it’s the only one.

Let’s go.

SurfaceMaker

import pygame

vector2 = pygame.Vector2

raw_rocks = [
    [
        vector2(4.0, 2.0), vector2(3.0, 0.0), vector2(4.0, -2.0),
        ...

This should work just fine, I just moved the rocks. Except that I need to import the name. Should I be using package here? Does Python even have that notion? I just import it.

Now I’ll move the adjust from ship.py over.

First I let PyCharm move it from method to function. PyCharm politely moves it to SurfaceMaker and imports it. Should be green, and should be committing. Commit: Refactoring over to SurfaceMaker.

PyCharm thinks that my other two methods could be functions as well. Following my nose, it makes all of these into top-level functions:

def get_ship_points():
    ship_points = [vector2(-3.0, -2.0), vector2(-3.0, 2.0), vector2(-5.0, 4.0),
                   vector2(7.0, 0.0), vector2(-5.0, -4.0), vector2(-3.0, -2.0)]
    return list(map(adjust, ship_points, repeat(vector2(7, 4)), repeat(4)))


def get_flare_points():
    flare_points = [vector2(-3.0, -2.0), vector2(-7.0, 0.0), vector2(-3.0, 2.0)]
    return list(map(adjust, flare_points, repeat(vector2(7, 4)), repeat(4)))


def make_ship_surface(ship_points):
    ship_surface = pygame.Surface((60, 36))
    ship_surface.set_colorkey((0, 0, 0))
    pygame.draw.lines(ship_surface, "white", False, ship_points, 3)
    return ship_surface


def make_accelerating_surface(flare_points, ship_points):
    ship_accelerating_surface = make_ship_surface(ship_points)
    pygame.draw.lines(ship_accelerating_surface, "white", False, flare_points, 3)
    return ship_accelerating_surface


def prepare_surfaces():
    ship_points = get_ship_points()
    flare_points = get_flare_points()
    return (make_ship_surface(ship_points)), (make_accelerating_surface(flare_points, ship_points))

I’ll move all those over as well. PyCharm even has a “bulk move” and knows which ones I might want to move. Now it’s all over in SurfaceMaker. Is it OK to name my files with upper case names? What is the Python guidance on that? Let’s test. Commit: moving to SurfaceMaker.

Now I reckon I can use the adjust from SurfaceMaker in asteroid, and probably more.

class Asteroid ...
    def prepare_surface(self):
        surface = pygame.Surface((128, 128))
        surface.set_colorkey((0, 0, 0))
        adjusted = list(map(adjust, raw_rocks[0], repeat(vector2(4,4)), repeat(16)))
        pygame.draw.lines(surface, "white", False, adjusted, 3)
        return surface

Should work. Does. Commit: moving from asteroid over to SurfaceMaker.

Now it thinks it can make prepare_surface a top-level function, and it can, because my other one is called prepare_surfaces. We’ll improve that shortly, but let’s boost it and move it over.

It keeps trying to import vector2. I should do better with that than my current trick:

vector2 = pygame.Vector2

It’ll do for now. We are green. Commit: Surface prep moved to SurfaceMaker.

Now ship and asteroid are quite simple:

class Asteroid:
    def __init__(self):
        self.position = vector2(u.SCREEN_SIZE/2, u.SCREEN_SIZE/2)
        self.surface = prepare_surface()
        rotation = random.randint(0,360)
        self.velocity = u.ASTEROID_SPEED.rotate(rotation)

    def move(self, dt):
        self.position += self.velocity*dt
        self.position.x = self.position.x % u.SCREEN_SIZE
        self.position.y = self.position.y % u.SCREEN_SIZE

    def draw(self, screen):
        half = vector2(self.surface.get_size()) / 2
        screen.blit(self.surface, self.position - half)

class Ship:
    def __init__(self, position):
        self.position = position
        self.velocity = vector2(0, 0)
        self.angle = 0
        self.acceleration = u.SHIP_ACCELERATION
        self.accelerating = False
        self.ship_surface, self.ship_accelerating_surface = prepare_surfaces()

    def draw(self, screen):
        ship_source = self.select_ship_source()
        rotated = pygame.transform.rotate(ship_source.copy(), self.angle)
        half = pygame.Vector2(rotated.get_size()) / 2
        screen.blit(rotated, self.position - half)

    def move(self, dt):
        self.position += self.velocity * dt
        self.position.x = self.position.x % u.SCREEN_SIZE
        self.position.y = self.position.y % u.SCREEN_SIZE

    def power_on(self, dt):
        self.accelerating = True
        accel = dt * self.acceleration.rotate(-self.angle)
        self.velocity += accel

    def power_off(self, dt):
        self.accelerating = False

    def select_ship_source(self):
        if self.accelerating and random.random() >= 0.66:
            return self.ship_accelerating_surface
        else:
            return self.ship_surface

    def turn_left(self, dt):
        self.angle -= u.SHIP_ROTATION_STEP * dt

    def turn_right(self, dt):
        self.angle += u.SHIP_ROTATION_STEP * dt

Clearly we have a controls thing happening in Ship that could be moved off. Not today, though.

We could change the surface making functions into methods on a SurfaceMaker class. I think we should do that, because top-level functions are all global and that’s just not cool.

This should be pretty straightforward. I think I can just put a class definition at the top and tab all those guys into it.

Having done that, the calls won’t hook up. Run to see what happens. The test collector goes nuts. I’ll fix up the calls manually.

That was a bit more tedious than I had anticipated. I had to add self parameters throughout, and had to call self. this and that, and I also messed up the imports. Even so it was easy and now we have this:

class SurfaceMaker:
    def __init__(self):
        pass

    def adjust(self, point, center_adjustment, scale_factor):
        return (point + center_adjustment) * scale_factor


    def get_ship_points(self):
        ship_points = [vector2(-3.0, -2.0), vector2(-3.0, 2.0), vector2(-5.0, 4.0),
                       vector2(7.0, 0.0), vector2(-5.0, -4.0), vector2(-3.0, -2.0)]
        return list(map(self.adjust, ship_points, repeat(vector2(7, 4)), repeat(4)))


    def get_flare_points(self):
        flare_points = [vector2(-3.0, -2.0), vector2(-7.0, 0.0), vector2(-3.0, 2.0)]
        return list(map(self.adjust, flare_points, repeat(vector2(7, 4)), repeat(4)))


    def make_ship_surface(self, ship_points):
        ship_surface = pygame.Surface((60, 36))
        ship_surface.set_colorkey((0, 0, 0))
        pygame.draw.lines(ship_surface, "white", False, ship_points, 3)
        return ship_surface


    def make_accelerating_surface(self, flare_points, ship_points):
        ship_accelerating_surface = self.make_ship_surface(ship_points)
        pygame.draw.lines(ship_accelerating_surface, "white", False, flare_points, 3)
        return ship_accelerating_surface


    def prepare_surfaces(self):
        ship_points = self.get_ship_points()
        flare_points = self.get_flare_points()
        return (self.make_ship_surface(ship_points)), (self.make_accelerating_surface(flare_points, ship_points))


    def prepare_surface(self):
        surface = pygame.Surface((128, 128))
        surface.set_colorkey((0, 0, 0))
        adjusted = list(map(self.adjust, raw_rocks[0], repeat(vector2(4,4)), repeat(16)))
        pygame.draw.lines(surface, "white", False, adjusted, 3)
        return surface

Python, of course, still thinks these can all be top level functions. We’ll get to that next time, when we’ll pass in the various points things as construction parameters or some such thing. For now, we’ve simplified ship and asteroid, and isolated all our surface creation over in SurfaceMaker. So that’s pretty fine, and good enough for a morning that has already been full of adventures.

See you next time! And let me know if you think I should GitHub the code. I will probably do it on general principles, once I figure out the right way to do it after the fact.