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 --statThat 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).
javascript/raphael-animate_frames.js | 192 +++++++++++++++++++---------------
1 files changed, 108 insertions(+), 84 deletions(-)
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
