Man, it’s quiet around here. Interested in doing some pimpin’?
Man, it's quiet around here. Interested in doing some pimpin'?
WAIT! COME BACK.
Code pimping. You know? The thing I'd discussed before? Forgive my earlier informality. I see now how my words could have been confusing.
The very first edition of Pimp My Code is special because the code we’ll be looking at will be included in Prototype 1.6.1. (It's a bit like if we were to Pimp [someone's] Ride™, then decide to keep the car for ourselves.) So this is more than just an academic exercise for us — the “pimped” result is now part of the Prototype source code.
The original
The code in question, from Sébastien Grosjean (a.k.a. ZenCocoon), implements element “storage” — attaching of arbitrary data to DOM nodes in a safe and leak-free manner. Other frameworks have had this for a while; jQuery’s $.fn.data
, for instance, is used heavily by jQuery plugin authors to great effect. But Seb’s is based on the similar Mootools API, which I’ve admired since it debuted in Mootools 1.2.
Here’s Seb’s code. It’s a long code block, since he’s been thoughtful enough to comment the hell out of it:
The idea is this: instead of storing arbitrary objects as properties on DOM nodes, create one custom property on the DOM node: an index to a global hashtable. The value of that key in the table will itself be a collection of custom key/value pairs. On top of avoiding nasty IE memory leaks (circular references between DOM objects and JS objects), this has the benefit of encapsulating all of an element’s custom metadata into one place.
Let’s make a first pass at this, line-by-line.
The critique
Already we’ve gotten to something I’d change. Seb is using the Prototype
namespace correctly here, in that he’s storing something that’s of concern only to the framework and should feel “private.” But my own preference is to move this property into the Element.Storage
namespace. I am fickle and my mind is hard to read.
OK, another change jumps out at me. The Element.Storage.init
method gets called in both Element#store
and Element#retrieve
; it handles the case where an element doesn’t have any existing metadata. It creates our custom property on the node and increments the counter.
In other words, store
and retrieve
are the only two places where this method is needed, so I balk at making it public. My first instinct was to make it a private method inside a closure:
I started down this path but quickly stopped. Instead, we’re going to refactor this part so that the init
case is handled without the need for a separate method. Let’s move on for now.
A few things to mention here.
-
Variable naming is important. The ideal name for the third parameter of this function would be
default
, but that’s off-limits;default
is a reserved word in JavaScript. Seb’s opted fordflt
here, which is clear enough. I’d change it todefaultValue
because I like vowels.As an aside: my first instinct was to remove the
defaultValue
thing altogether, because I was surprised by the way it behaved. I didn’t find it very intuitive to giveElement#retrieve
the capability to store properties as well. So I took it out.I changed my mind several minutes later, when I wrote some code that leveraged element metadata. I had assumed I wouldn’t need the “store a default value” feature often enough to warrant the surprising behavior, but I was spectacularly wrong. I put it back in. Consider that a lesson on how your API design needs to be grounded in use cases.
-
The idiom in the first line is used throughout Prototype and script.aculo.us (and, in fact, should be used more consistently). It runs the argument through
$
, but also checks the return value to ensure we got back a DOM node and notnull
(as would happen if you passed a non-existent ID). An emptyreturn
is equivalent toreturn undefined
, which (IMO) is an acceptable failure case. Bonus points, Seb! -
The custom property Seb’s been using is called
uid
. I’m going to change this to something that’s both (a) clearly private; (b) less likely to cause a naming collision. In keeping with existing Prototype convention, we’re going to call it_prototypeUID
. -
Here’s a nitpick:
if (element.uid == undefined)
. The comparison operator (==
) isn’t very precise, so if you’re testing forundefined
, you should use the identity operator (===
). You could also use Prototype’sObject.isUndefined
. In fact, I will.I have a prejudice against the
==
operator. Most of the time the semantics of===
are closer to what you mean. But this has special significance withundefined
, which one encounters often in JavaScript. As an example: when you’re trying to figure out if an optional parameter was passed into a function, you’re looking forundefined
. Any other value, no matter how “falsy” it is, means the parameter was given;undefined
means it was not.(Oh, by the way: I am aware of the code screenshot on our homepage that violates the advice I just gave.)
-
There are other checks against
undefined
in this function. For consistency I’m going to change these to useObject.isUndefined
as well. Also, the check fordflt != undefined
is unnecessary: if that compound conditional passes, it meansretrieve
is going to returnundefined
anyway, so it doesn’t matter which of the twoundefined
values we return.
Man, I’m a bastard, aren’t I? Luckily, Element#store
is similar enough that there’s no new feedback to be given here, so I’m done kvetching.
Before we rewrite this code to reflect the changes I’ve suggested, we’re going to make a couple design decisions.
Feature design
While I was deciding how to replace Element.Storage.init
, I had an idea: rather than use ordinary Object
s to store the data, we should be using Prototype’s Hash
. In other words, we’ll create a global table of Hash
objects, each one representing the custom key-value pairs for a specific element.
This isn’t just a plumbing change; it’s quite useful to be able to deal with the custom properties in a group rather than just one-by-one. And since Hash
mixes in Enumerable
, interesting use cases emerge: e.g., looping through all properties and acting on those that begin with a certain “namespace.”
So let’s envision a new method: Element#getStorage
. Given an element, it will return the Hash
object associated with that element. If there isn’t one, it can “initialize” the storage on that element, thus making Element.Storage.init
unnecessary.
This new method also establishes some elegant parallels: the store
and retrieve
methods are really just aliases for set
and get
on the hash itself. Actually, retrieve
will be a bit more complicated because of the “default value” feature, but we’ll be able to condense store
down to two lines.
The rewrite
Enough blathering. Here’s the rewrite:
As promised, I’ve moved the UID
counter. The Element.Storage
object also acts as our global hashtable, but all its keys will be numeric, so the UID
property won’t get in anyone’s way.
Element#getStorage
assumes the duties of Element.Storage.get
and Element.Storage.init
, thereby making them obsolete. We’ve removed them.
The new getStorage
method checks for the presence of _prototypeUID
. If it’s not there, it gets defined on the node.
It then looks for the corresponding Hash
object in Element.Storage
, creating an empty Hash
if there’s nothing there.
As I said before, Element#store
is much simpler now:
I thought about returning the stored value, to make it behave exactly like Hash#set
, but some feedback from others suggested it was better to return the element itself for chaining purposes (as we do with many methods on Element
).
And Element#retrieve
is nearly as simple:
And we’re done.
Further refinements
In fact, we’re not done. This is roughly what the code looked like when I first checked in this feature, but some further improvements have been made.
Since we’d been using a system similar to this to associate event handlers with nodes, we had to rewrite that code to use the new storage API. In doing so, we found that we needed to include window
in our storage system, since it has events of its own. Rather than define a _prototypeUID
property on the global object, we give window
a UID of 0
and check for it specifically in Element#getStorage
.
Also, based on an excellent suggestion, we changed Element#store
so that it could accept an object full of key/value pairs, much like Hash#update
.
In summation
I was happy to come across Sébastien's submission. It was the perfect length for a drive-by refactoring; it made sense as a standalone piece of code, without need for an accompanying screenshot or block of HTML; and it implemented a feature we'd already had on the 1.6.1 roadmap.
You can get the bleeding-edge Prototype if you want to try out the code we wrote. Or you can grab this gist if you want to drop the new functionality in alongside 1.6.0.3.
We're further grateful to Mootools for the API we're stealing. And to Wil Shipley for the recurring blog article series we're stealing.