Personal tools

Jun 01, 2010

Sorrento Amberjack Sprint: can you please make my Javascript simpler?!

The Javascript source of collective.amberjack need some love. Not a simple task, but something has been done (and something more will be in future).

First of all: the inherited problems

Version 1.0 of collective.amberjack.core is a simple Plone 4 compatibility version of the old 0.9 released.

When we released this we already know that the Javascript code inside was a mess, but before beginning some kind of refactoring procedure we need to know how the project will be changed in future.

The product has a core got from the Amberjack library, in last years many different programmers take part to the develop, meanwhile the product itself (and also Plone) was changing.

Amberjack original library is framework independent while Plone (that is a smart guy) rely on jQuery. Also we must not forget that Amberjack has not exactly the same target of collective.amberjack...

Wow... how many problems!

Not all those problems will be fixed in the 1.1 version, but I managed to simplify a little the code of two of the main method of amberjackPlone.js source.

How the old amberjackPlone.js was done

Saying "the code is ugly but works" is not a valid excuse... amberjackPlone.js contains two important methods:

  • highlightStep is the method that highlight all the clickable/usable elements in the page for all steps.
  • doStep is the method called to reproduce the user action on the page

What a user can do in a Plone site is a long list of different actions... list that become something like this for highlightStep:

...
if(type_obj=="checkbox" || type_obj=="radio"){
	obj.parent().addClass(theAJClass);
	obj.addClass(theAJClassBehaviour);
}
else if (type_obj=="select"){
	var highlightThis = jq(obj + " option[value="+ AjSteps[num].getValue() +"]");
	highlightThis.addClass(theAJClass);
	obj.addClass(theAJClassBehaviour);
}
else if (type_obj=="multiple_select") {
...
...going on and on...
...
else{
	obj.addClass(theAJClass);
	obj.addClass(theAJClassBehaviour);
}
...

...and like this for doStep:

...
if (type_obj == 'link') {
	AmberjackPlone.setAmberjackCookies();
	location.href = obj.attr('href');
}
else if (type_obj == 'button')
	obj.click();
else if (type_obj == 'collapsible') {
...
...going on and on and on and on...
...

Ugly enough? Do you think that the AntiIfCampaign hate us? Yes... probably...

So the first step is to remove this mess and try to port in Javascript the concept of adapter (unluckily not so flexible as Zope ones).

Even worst: many of the if statement in both doStep and highlightStep was testing the same expressions (like the "select")!

All this code has been removed.

Now both doStep and highlightStep are simply calling a sort of adapter built in the stepAdapters.js file:

AmberjackPlone.stepAdapters = {
	
	link: {
		highlight: null,
		checkStep: null,
		step: function(obj, type_obj, jq_obj, value) {
			AmberjackPlone.setAmberjackCookies();
			location.href = obj.attr('href');
		}
	},
	button: {
		highlight: null,
		checkStep: null,
		step: function(obj, type_obj, jq_obj, value) {
			obj.click();
		}
	},
	collapsible: {
...
... going on and on, but more readable!
...

For every possible action know, highlightStep and doStep will check if an handler is present in this structure (in this example the code is from the doStep):

if (AmberjackPlone.stepAdapters[type_obj] && AmberjackPlone.stepAdapters[type_obj].step)
	AmberjackPlone.stepAdapters[type_obj].step(obj, type_obj, jq_obj, value);

The highlightStep will check for highlight function.

If not if found, perform the old final else statement.

What is the checkStep section? In this sprint another group (Mirco and Simone Orsi) take a look to the feature that test if a user has completed all the steps before can go to the next page. Again, for some possible action the tour can try to test if you have finished your work.

In the old approach this will lead us to a new big and monolithic function. Now is only a new tiny method that call an handler.

What's next?

Adding some documentation, for sure...

Also other part of the code can be changed to become more clear...

...to be continued!

comments powered by Disqus