Refactoring with Prototype
I’m not surprised to see prototype code that sucks. Been there, done that (and I’m sure still doing it) – nobody is perfect. What surprises me though is how ridiculous some explanations (of writing crappy code) sound. “I don’t really know javascript. I do PHP professionally” is my favorite. Don’t get me wrong, I understand that experience and deep understanding of language is essential for writing smarter code. It could be a real challenge to wrap your mind around some of the dynamic concepts of such languages as javascript. That I can understand. What I don’t understand, though, is how people don’t follow some of the fundamental principles of programming. Do we really need to know javascript to prevent repetition, unnecessary constructions or poor optimization? I seriously doubt it…
I want to take a look at few examples of real life applications and see what we can do to make them… better. Aiming for a smaller footprint, faster execution and prettier code is what makes it a real challenge, so let’s dive in…
Repetition
function hideall() { hidediv('dropdown1'); hidediv('dropdown2'); hidediv('dropdown3'); hidediv('dropdown4'); hidediv('dropdown5'); hidediv('dropdown6'); hidediv('dropdown7'); }
Very basic, ugly (and unfortunately real) example. The only part that varies here are numbers so all we need to do is loop through them. Prototype provides us with a quite convenient utility function for generating ranges – $R takes 2 parameters – starting and ending values and creates a range object by calling succ method on each value. What’s really cool about this object is that it extends prototype’s Enumerable methods – iteration has never been easier:
$R(1,7).each(function(n) { hidediv('dropdown' + n); })
Not only we decreased the overall size of this snippet, but notice how more generic and flexible it has become.
Repetition 2
function resetnotes() { $('left1').removeClassName('learn'); $('left1').removeClassName('play'); $('left1').removeClassName('grow'); $('left1').removeClassName('join'); }
This one is similar but is even worse, as we’re calling $ function multiple times, decreasing overall performance. What varies here are plain strings and that’s when $w helper could come in handy. Inspired by Ruby, $w takes a string of space-separated values and creates an array out of it. Remember that arrays are automatically extended with Enumerable methods:
// storing element's reference for faster access var left1 = $('left1'); // creating array of strings and iterating over them $w('learn play grow join').each(function(s){ left1.removeClassName(s); })
Another solution would be to wrap (augment) removeClassName to accept multiple arguments (classNames).
I don’t suggest to use the following snippet in a production environment as I haven’t tested it thoroughly. The idea is to show you basic way of augmenting prototype’s existent methods.
Element.addMethods({ removeClassName: Element.Methods.removeClassName.wrap(function() { var args = $A(arguments), proceed = args.shift(), element = $(args.shift()); if (args.length > 1) { // this is our modified behavior (only if more than 1 argument was passed) element.className = element.className.replace(new RegExp("\s*(" + args.join('|') + ")\s*"), ''); return element; } // and this is a default one return element.proceed(); }) })
Attention: Try not to fall into a trap of “generic-madness”. Yes, it’s beautiful. Yes, it encapsulates certain logic into its own layer and creates reusable chunks of code, but no matter how great it is, think twice before taking this approach. Does it pay to spend time creating another abstraction layer? Will it actually be used in the future? If it doesn’t simplify, just drop it. Keep it simple and move on.
Using Element.addMethods we redefine prototype’s removeClassName to suit our needs. Notice how unobtrusive yet powerful this solution is. By wrapping already existent method with or own behaviour, we keep same logic in same place, and making sure augmented method does not break anything (Take a look at a previous post about Function#wrap if you’re not familiar with it)
Ternary operators
if (typeof this.prevbutton == 'object') { $(prevmonth).appendChild(this.prevbutton); } else { $(prevmonth).update(this.prevbutton); }
Using “if … else … ” for only two conditions is usually a bad thing to do. Selecting one of two actions is even worse.
Ternary operator, on the other hand, is a perfect replacement for “if … else … ” clause. Remember, that we can use bracket notation to call one of two object’s methods:
$(prevmonth)[typeof(this.prevbutton) == 'object' ? 'appendChild' : 'update'](this.prevbutton);
Chaining
$(nextmonth).addClassName('calcontrol'); $(nextmonth).addClassName('calnextmonth'); $(nextmonth).observe('click',this.nextmonth);
Taking advantage of chaining is easy – most of the methods in prototype return reference to the element they were invoked on. Another thing to note is that addClassName is simply appending it’s argument to the className of an element. There’s really no need invoking it more than once:
$(nextmonth).addClassName('calcontrol calnextmonth').observe('click', this.nextmonth);
Those are very simple yet essential tips for writing more concise, elegant code and I would really want to see more people using them.
Enjoy your ride and happy prototyping…
Crispen Smith said:
#I’ve got a model challenge that I’m not sure how to handle, and ironically you’ve posted this today.
What’s the best way in prototype to handle the situation where you’ve got the functional equivalant of a hash?
given an onload event that populates lengthy text into 10 divs I’m currently working with the (admitedly) bad model of creating a series of $(div).update(‘new_text’) calls. (well to be exact:
Var = $(div); var.update(new_text); var.show; var.observe(mousemove, function(event) { if(Event.within(var, positionX, PositionY) { var.hide()})});
) calls (forgive the psuedo code).
So this comes down to… do you call a single function with the div name and a great big variable for the update text? do you set up an array and test against the array? Or do you just call the nearly identical code on each individual div?
Hope that this can be helpful for anyone who has similar situations and that you can help me.
cheers,
Crispen
vectoroc said:
#good article
about
$w(‘learn play grow join’).each(function(s){
left1.removeClassName(s);
})
i think
$w(‘learn play grow join’).each(left1.removeClassName, left1))
or
$w(‘learn play grow join’).each(Element.removeClassName.curry(left1))
more better way ;)
kangax (article author) said:
#@Crispen Smith
I’m not sure I understand what exactly you’re trying to do, but you can find me in #prototype irc channel and I’m sure we can figure something out.
@vectoroc
Yep, I completely forgot that most (if not all) enumerable methods take context as a second argument
Thanks for reminding : )
Matt Foster said:
#This is great stuff, Enumerables can certainly clean up sloppy syntax.
For Crispen, I’d recommend not using a variable named var as its a reserved word.
Crispen Smith said:
#Kangax:
Not on IRC, but solution I’ve come up with so far is:
Event.observe(window, ‘load’, function() {
Event.observe(document, ‘click’, function() {
var e_self = Event.findElement(event, ‘a’);
if(e_self !=document) {
for(var i=0; i
Aleem said:
#Regarding the first example, I’d shoot myself before using $R/.each() approach. The smart reader should note:
1. Sure you can flex your prototype.js knowledge and lazy-programming skills, you are sacrificing performance for compactness here. Just have a look at the code for .each() {
each: function(iterator, context) {
var index = 0;
iterator = iterator.bind(context);
try {
this._each(function(value) {
iterator(value, index++);
});
} catch (e) {
if (e != $break) throw e;
}
return this;
}
Read more here: http://peter.michaux.ca/article/48
2. Not only is the $R/.each() loop a performance no-no but it’s a lot more obfuscated and harder to comprehend what is actually happening. I would classify it at best, as a cool trick but nothing more. A simple for-loop is actually the best approach (they’re universal across most programming languages, easy to comprehend and have faster execution).
Fabian said:
#Do you really think the ternary thing is worth refactoring?
When doing refactoring you can achieve many things:
– Making code more performant
– I doubt that your way is more performant
– Making it easier to read
– You are doing reflection introspection here and the line is longer and its harder to see which method is invoked on which criteria
– Making code easier to extend
– The easiest code to extend is the one that has all brackets for if and else in place, preventing nesting errors. Adding a second condition here is impossible requiren a rewrite of the line again back to the original.
So why do you refactor here? I also disagree that using if else for two conditions is a bad choice, but thats more a matter of taste. I tend to use ternary operators only for assignments, because i think thats easier to read and maintain.
Nightfly said:
#Good examples, a really neccessary wrap-up. I too have seen my bunch of really ugly JavaScript.
Still, “elegant” is not always fast, and sometimes fast code is just plain ugly.
The only thing I have to criticize is your example of the use of the ternary operator. Sure, your code is shorter, but the original code is just perfect IMO, as it is semantically more to the point and easier to understand and modify.
Consider this:
if (el == li) {
el.addClassName('active');
contents.show();
} else {
el.removeClassName('active');
contents.hide();
}
Using the ternary operator is now not elegant anymore, and you have to recreate the original code again.