The Great (Lucky) Refactor

‹prev | My Chain | next›

I got a new feature added to animate-frames last night. Getting there made me realize that the code is a bit messy so tonight I refactor!

Animate-frames is a plugin for raphaël.js that iterates through a series of raphaël/SVG frames as the frames move about the paper. I am using this in my (fab) game to draw and animate players as they move about the game room. I am going to refactor in the game before extracting the changes back out into the plugin.

The refactoring is necessary because many of the methods in the plugin look like:

     stop: function() {
       var frames = this.list;
       for (var i=0; i<frames.length; i++) {
         for (var j=0; j<frames[i].length; j++) {
           frames[i][j].stop();
         }
       }
     }

The class is responsible for managing the frames and the parts that make up the frames:



(the parts that make up the frames are the players' body, legs, and feet — each is a different SVG path)

I could make that much cleaner if the frames methods were just responsible for knowing about the frames, but made a separate class responsible for dealing with the pieces. In that case, this:

     stop: function() {
       var frames = this.list;
       for (var i=0; i<frames.length; i++) {
         for (var j=0; j<frames[i].length; j++) {
           frames[i][j].stop();
         }
       }
     }

...would become this:

    stop: function() {
      for (var i=0; i<this.list.length; i++) {
        this.list[i].stop();
      }
    }

Ah, much better. No more worrying about what that j variable was iterating over. No more trying to discern which of those 6 lines are important. Just one clear iteration through each frame in the animation, telling each frame to stop().

The corresponding stop method on my Frame object is then:

  Frame.prototype.stop = function() {
    for (var i=0; i<this.elements.length; i++) {
      this.elements[i].stop();
    };
  };

Again, nice and compact. Just one iteration making it very clear what is going—working through each part that makes up the frame and telling it to stop.

I end up working through half a dozen similar methods. All sorts of weird naming becomes easier (translate_object() becomes just translate() on the Frame object). Initialization code gets separated in much easier to read ways. It is great.

And then something amazing happens....

I load it up in the browser and it just works! I have no tests (note to self: future idea for a chain post) that helped me support the refactoring. Git diff reports:

cstrom@whitefall:~/repos/my_fab_game$ gd --stat
 javascript/raphael-animate_frames.js |  192 +++++++++++++++++++---------------
 1 files changed, 108 insertions(+), 84 deletions(-)

That is a lot of non-trivial change. I had to intentionally break the code to make sure I was running the updated code (I was).

Amazing? Am I that good?

Absolutely not. That was pure luck and I damn well know it. The coding gods should have punished me severely for even attempting it. I give them thanks for going easy on me tonight, but I know I have a doozy of a refactoring fail in my future.

One thing I do notice is that I repeat myself in the simple raphael methods:

  Frame.prototype.remove = function () {
    for (var i=0; i<this.elements.length; i++) {
      this.elements[i].remove();
    };
  };

  Frame.prototype.show = function() {
    for (var i=0; i<this.elements.length; i++) {
      this.elements[i].show();
    };
  };

  Frame.prototype.hide = function() {
    for (var i=0; i<this.elements.length; i++) {
      this.elements[i].hide();
    };
  };

  Frame.prototype.stop = function() {
    for (var i=0; i<this.elements.length; i++) {
      this.elements[i].stop();
    };
  };

I also notice that my players become disjointed after colliding too often:



I will pick back up tomorrow with DRYing up my code after the great refactor and trying to solve the detached limb problem.


Day #142