Oh my, that code… where to begin!?
Before you read on, perhaps you should have a 30 sec look at what I’m talking about. I’ll wait here, go ahead!
…
Current thoughts about this project
TimeLine’s at prototype status, but hopefully it’s clear what the app was meant for: visualizing experience over time, using grouped, parallel timelines.
I look back at this mini-project, and directly notice a few things. First and foremost: I still like the idea of such an app! Visualizing work experience is great for a résumé I think, it allows the reader to instantly get a “feel” for what you’ve been working on. Much better than dreary text.
The second thing I realize is that I clearly had no idea and/or time to quickly scaffold an editor (“yes”, I thus resorted to using a textarea with JSON; so sue me!). Today I’d use KnockoutJS for this, even in a mockup. Or, barring KO, I’d use some other plugin for it. But then again: “first make it work, then make it work better”, so I’m happy I chose something and moved on.
The final thing I realized: it’s a decent prototype, and to get there I sacrificed a lot of code quality. With 140 SLOC it’s not much code, but what’s there is rather bad.
Finishing things
This leaves me a bit in a tight spot. What am I to do with this project?
First I thought about deleting the thing entirely, praying no one would ever find out about the JavaScrippaghetti I had unleashed on the world. Then again, that would be silly: everyone needs to write a fair share of bad code to get better at writing code.
My second thought was to rewrite the thing from scratch: right here, right now. Nearly started working on it too. Guess I wasn’t lying when I said I still like the idea. Then again, that would be silly: I still have two other projects left to “wrap up”, one of which is much more deserving of my time.
My third and final thought was to wrap up by doing a loose “code review” of my own code. In addition it feels good to “stamp” it with a disclaimer and message that it’s “wrapped up”. And that actually seems like the best idea.
Code review
The html is not really worth going through. It’s a plain html-reset-based file, that uses semantic markup (header, nav, article, etc), and has a placeholder div where Raphaël will draw the timelines. The html contains a link to timeline.js, and some basic bootstrapping code to call timeline jQuery-plugin-style on the placeholder div.
Let’s dive into that one code file. It’s starts off with some good signs:
|
1 2 3 4 5 6 |
/*jshint strict: true */ // http://www.jshint.com (function( $ ){ 'use strict'; // ... |
Apparently the code’s being linted with a more sane version of Crockford’s jslint, and the code declares ES5 “strict mode”. Guess the author did forget to run the linter recently, because currently it seems to still cause a few errors. Anyways, moving on we see:
|
1 2 |
var minDate, maxDate; var paper, paperPadding = 10; |
Not exactly globals, but a definite hint the author’s struggling with JavaScript closures. Except for paperPadding perhaps, which hints at a feature for settings that’s just not implemented yet.
Moving on the code follows a typical jQuery plugin template, first with all the plugin’s methods defined within a seperate closure, then at the end of the file exported in a typical fashion:
|
1 2 3 4 5 6 7 8 9 |
$.fn.timeline = function( method ) { if ( methods[method] ) { return methods[method].apply( this, Array.prototype.slice.call(arguments, 1) ); } else if ( typeof method === 'object' || ! method ) { return methods.init.apply( this, arguments ); } else { $.error('Method ' + method + ' does not exist on plugin.'); } }; |
What you can already see here though is that the plugin relies on one single god method: init. This method in itself isn’t all too interesting. It just loops through the grouped timelines, and renders them one by one. To find some things that are of note I went through a mix of Dutch and English comments (not a good thing for something intended to be public). Here’s one that’s particularly interesting:
|
1 |
// TODO chop this one big spaghetti function in pieces |
Phew, at least the author realized the code could not go on like this for much longer. This is further clarified by the presence of this kind of code (abbreviated):
|
1 2 3 4 5 6 7 8 |
for (i=0; i // ...determine "found" boolean based on data... if (found) { lanes[i].segments.push(segmentToInsert); break; } } |
Yikes! The break statement is IMHO often a code smell (smells of goto), surely there’s an OO-approach for the problem being solved here.
Let’s finish on a positive note though, having a look at some of the variable names in use:
categoryHighwaylanesegment
This makes me smile. Having the UI of the app in mind I instantly know what these mean. A segment is one piece of timeline, a lane is a horizontal area where segments can be placed, and a catgory highway is a set of lanes for segments from one category. Close-reading the code I find this is in fact mostly correct. And with that I feel there’s a great OO (or more prototypical) way to reboot the code for this app.
In Conclusion
In conclusion, or perhaps more appropriately “in summary”, I can say this is a fine and above-all inspiring prototype. The code’s short and bad, but not even that bad for a quick prototype.
With a proper disclaimer and “wrap up” message I feel can safely leave TimeLine be for now. Perhaps one day I’ll revisit it.