Mалко JavaScript refactoring част 2

Наскоро ми се наложи да работя по един стар проект на Pixeldepo, по който нямах участие до сега. Промените бяха главно козметични, така че реално аз нямах много работа там. Така че реших да разчистя малко javascript-а в сайта, който въпреки, че работеше без проблемибеше доста стар.

Така едно парче код ми се стори доста интересно (кода използва prototype.js и script.aculo.us):

$('.leftmenu li.header a').each(function(dropa){
	dropa.observe('click', function(){
		var title = dropa.className;
		var theul = $(title);
		if (theul) {
			if (theul.style.display == "none") {
				Effect.BlindDown(theul, {
					duration: 1.0
				});
				var theli = dropa.up(0);
				new Effect.Morph(theli,{
					style: 'padding-bottom: 20px;',
					duration: 1.0
				});
			}
			else {
				Effect.BlindUp(theul, {
					duration: 1.0,
					queue: 'end'
				});
				var theli = dropa.up(0);
					new Effect.Morph(theli,{
						style: 'padding-bottom: 20px;',
						duration: 0.1
				});
			}
		} else {
		}
	});
});

Кода е представлява просто accordion меню. И от кода му е ясно, че се нуждае от почистване.

Първата промяна е на тези редове:

$('.leftmenu li.header a').each(function(dropa){
	dropa.observe('click', function(){
// става ->
$$('.leftmenu li.header a').invoke('observe', 'click', function(){

Тук освен, че става доста по-четимо и малко като код, се спестява предефинирането на анонимната функция за всеки елемент от ‘.leftmenu li a’, ‘. Защото при повечето браузъри, ще се създава нова функция за всеки event-handler, докато при новата версия ще имаме само една функция която ще се използва за всички елементи. Нужно е и да се отбележи, че навсякъде в “dropa” ще трябва да се промени с this.

После променяме тези редове:

var title = dropa.className;
var theul = $(title);

// става ->
var ul = $(this.className);

Не е много добре да се дефинира променлива, която се ползва само един път, излишно хабене на код и памет е, и затова по-добре да се “var title = dropa.className” да се съкрати. Също така dropa трябва да се замени с this, тъй като вече сме  в event метода, а не във closure-a. И накрая theul го правя просто ul защото ми изглежда по-добре.

След това идва ред на основната част от кода:

if (theul) {
	if (theul.style.display == "none") {
		Effect.BlindDown(theul, {
			duration: 1.0
		});
		var theli = dropa.up(0);
		new Effect.Morph(theli,{
			style: 'padding-bottom: 20px;',
			duration: 1.0
		});
	}
	else {
		Effect.BlindUp(theul, {
			duration: 1.0,
			queue: 'end'
		});
		var theli = dropa.up(0);
		new Effect.Morph(theli,{
			style: 'padding-bottom: 20px;',
			duration: 0.1
		});
	}
} else {
}

Първо тук theul го заменям с ul, после ” ul.style.display == “none” ” го правя просто ” ul.visible() “, защото като бързодействие няма голямо значение, но пък изглежда доста по-добре. Махам от ефектите опцията “duration:  1″, защото  по подразбиране за всички ефекти от Script.aculo.us duration е 1, така че няма смисъл да се оказва. Другата голяма промяна в кода е че променливата “theli = dropa.up(0);”, се премества директно в morph ефекта,  защото като и с title променливата се ползва само един път и следователно няма нужда да се дефинира (тук също така махам и нулата (0 ), която се подава като аргумент, защото това е аргумента по подразбиране).  И накрая съм махнал празния else, защото той нищо не прави.

Следващата стъпка  е ясна, morph ефекта го има два пъти, и винаги се изпълнява ако “ul” елемента съществува

if (ul) {
	if (ul.visible()) {
		Effect.BlindDown(ul);
	} else {
		Effect.BlindUp(ul, {queue: 'end'});
	}

	new Effect.Morph(this.up(),{
		style: 'padding-bottom: 20px;'
	})
}

След малко разглеждане на ефекта, видях, че опцията на “BlindUp” – queue: ‘end’, не е нужна и затова мога целия блок “if (ul.visible())”, който става да стане просто:

ul[ul.visible() ? 'blindDown' : 'blindUp']();

Напоследък се усещам че предпочитам да използвам Script.aculo.us ефектите във формат element.effect(), а не във Effect.name(element), защото изглежда по-добре, а и до сега не съм се забелязал да забавя приложенията ми.

Така кода, който почнах да “почиствам” изглежда така:

$$('.leftmenu li.header a').invoke('observe', 'click', function(){
	var ul = $(this.className);
	if (ul) {
		ul[ul.visible() ? 'bindDown' : 'bindUp']();
		this.up().morph('padding-bottom: 20px;');
	}
});

Това си е същия код който и преди, поне функционално погледнато. Всички промени, които направих главно козметични и не променят цялостната идея на кода. Сега като цяло кода изглежда доста добре, но винаги може да е по-добре

CSS стила с който се избират елементите ‘.leftmenu li.header a’, е доста бавен, защото трябва да се намерят елементите първо по клас “leftmenu” после да се намеря всички в li-та в тези елементи, а поле и всички а елементи. След лек преглед на html кода се оказа че CSS класа .leftmeu се използва само за лявото меню и никъде другаде. В такива случай е добре класа да стане на id, освен че самия CSS ще стане по-чист, ще може и по-лесно да се работи с javascript-a, който управлява менюто, защото ще може да се ползва доста по-бързия CSS селектор – “#leftmenu li.header a”.

Друго нещо, което ме тревожеше в кода беше че по името на класа на а елемента се определяше кой ul елемент да се отваря или затваря при натискане. От една страна това не е хубаво защото CSS частта става твърде зависима от логиката на менюто, да не говорим че самите връзки в менюто са динамични и могат да се променят. HTML кода на менюто беше нещо такова:

<ul id="leftmenu">
	<li class="header">
		<a class="section-id">Section name</a>
		<ul id="section-id">
			<li><a>...</a></li>
			<li><a>...</a></li>
			...
		</ul>
	</li>
	...
</ul>

Затова ” var ul = $(this.className);” може без проблем да се промени на “var ul = this.next(‘ul’);” и да се махне цялата гадна зависимост между класа на връзката и id-to на списъка с връзки

Друга част излишен код, се оказа – ” this.up().morph(‘padding-bottom: 20px;’); “, който явно се е ползвал в старата версия на дизайна, но сега е непотребен.

Така целия код на това accordion меню, става просто това:

$$('#leftmenu li.header a').invoke('observe', 'click', function(){
	var ul = this.next('ul');
	if (ul) ul[ul.visible() ? 'bindDown' : 'bindUp']();
});

Надявам се този пост да е бил полезен на някого, опитал съм се максимално да обясня стъпките през които съм минал за да оптимизирам този код. Ако има нещо неясно или някой има идея как може още да се оптимизира този код ще се радвам да ги чуя като коментари

п. п. Този код може да се оптимизира още, ако се ползва моята CD3.Behaviours библиотека

п.п. 2 Преди малко се сетих за Effect.toggle, който все го забравям, а с него кода би изглеждал така:

$$('#leftmenu li.header a').invoke('observe', 'click', function(){
	var ul = this.next('ul');
	if (ul) Effect.toggle(ul, 'blind');
});