Elegancki kod., Kod który działa ale jest "brzydki" Opcje

[saund]

Hej!

Jestem początkującym programistą JS i korzystam z framewoka jQuery.

[HTML] pobierz, plaintext 
function maximumZindex () {
	"use strict";
	var highestZindex = 0;
	frame.contents().find('body *').each(
		function zIndexCheck () {
			if (isNaN(Number($(this).css("zIndex")))) ; // Do nothing when .css() return "auto"
			else if ($(this).css("zIndex")>highestZindex) 
				highestZindex = $(this).css("zIndex");
		})
	highestZindex++;
	return highestZindex;
};
//v2 
function maximumZindex () {
	"use strict";
	var highestZindex = 0;
	frame.contents().find('body *').each(
		function zIndexCheck () {
			if (!isNaN(Number($(this).css("zIndex")))) {
				if($(this).css("zIndex")>highestZindex)
				highestZindex = $(this).css("zIndex");
			};
		})
	highestZindex++;
	return highestZindex;
};
 
[HTML] pobierz, plaintext 

Są dwie wersje tego samego skryptu.
Opis działania: Skrypt wyszukuję największy z-index występujący na stronie w ramce iframe ( $("#frame") = frame ). Pierwszy IF sprawdza czy .css() nie zwraca wartości auto (Nie chcę porównywać "auto" z liczbą a parsowanie "auto" na inta wzraca NaN)jeśli tak to nic nie robi jeśli nie to porównuje do ostatniego największego z-index jakiego napotkał i jeśli jest większy to staje się aktualnym największym z indexem.

Postanowiłem zapytać się znajomego, który jest programistą JS czy kod jest poprawny (pokazałem mu pierwszą wersje) powiedział, że pusty IF nie przejdzie nigdzie więc ok napisałem drugą i powiedział, że kod jest beznadziejny i oprócz tego, że brakuję wywołania wewnętrznej funkcji zIndexCheck() wiem bo tylko tego się od niego dowiedziałem. Zasugerował mi też, że $('body *').each(...) jest za wolny w jaki inny sposób(szybszy) mogę sprawdzić jakie z-index mają wszystkie objekty występujące na stronie.

Co mogę zrobić, żeby ten kod był "elegancki" ?

Ten post edytował saund 18.08.2013, 22:43:29

[sunpietro]

Po pierwsze, stosuj jednolity standard formatowania kodu JS.
Po drugie, jeśli chcesz iterować po wszystkich elementach DOM, to lepiej do tego użyć czystego JS:

Kod

var items = iframe.getElementsByTagName("*");
for (var i = items.length; i > 0;  i--;) {
    sprawdzanie
}

[zegarek84]

Po drugie, jeśli chcesz iterować po wszystkich elementach DOM, to lepiej do tego użyć czystego JS:

Kod

var items = iframe.getElementsByTagName("*");
for (var i = items.length; i > 0;  i--;) {
    sprawdzanie
}

sorki, bez urazy, ale nie do końca jest to prawda i masz jeszcze błąd w kodzie... -> powinieneś zrozumieć (w jQ jest to zaimpementowane) http://stackoverflow.com/questions/926916/...e-in-javascript

po za tym nie pamiętam, jak dokładnie jest pisany silnik Sizzle ale tam jest to w miare optymalnie napisane i do prostych przypadków o ile dostępna metoda to korzysta z szybkiego .querySelectorAll, a jak nie dostępne to zapytania CSS są optymalnie przeszukiwane - NAJWIĘKSZY PROBLEM KORZYSTAJĄCYCH Z jQ jest brak "buforowania" referencji do elementów które wielokrotnie wyszukują...

wracając do tematu czemu zwiększasz o 1 highestZindex, po za tym z samym zIndex o ile pamiętam to nie maiłbyś tak łatwo jak to się wydaje po samej wartości ;p, o ile pamiętam jeśli rodzice i inne elementy mają nadane zIndex to w sumie masz wartości pośrednie a nie po cyfrze ;p... jeśli nie nadasz zIndex to w przeglądarce elementy na tym samym poziomie i tak mają jakby nadany domyślny zIndex którego się nie odczytuje w kolejności występowania (raz musiałem odwracać elementy do jednego tricku ;p)

co do komentarza tamtego "programisty" to się czepiał szczegółów... ale po części miał rację gdyż po co pisać pierwszy warunek gdzie nic nie robisz?? ale stosuje się takie ify w trochę innym wykonaniu, w pętli do break i w grubszej funkcji przy odwróconych warunkach dla czytelności kodu do przerwania dalszego wykonywania kodu przez if(true) return wartosc_lub_nic;

z tym "za wolny" już napisałem co i jak, fakt masz lekki narzut na szybkości ale nie za wielki... jeśli chcesz zyskać na szybkości to popraw przykład podany przez @sunpietro (pętla poprawna choć iteracja od tyłu - choć iteracja od tyłu fajnie przydaje się do super szybkiego usuwania elementów z listy ;p), a gdzie drobny błąd podałem w linku wyżej...

nigdy nie programowałem zawodowo jednak 3 lata temu współpracowałem z manifo przy tworzeniu edytora WWW (swoją drogą zastanawiam się, czemu tego wpisu jeszcze nie zaktualizowali)
http://pl.manifo.com/o_nas.html - Grzegorz Nowak, zajmowałem się głównie JS, w tym edytorem w iframe na bazie modyfikacji CLEditor'a, różnicami przeglądarkowymi

[lukasz1985]

Nie nadawałbym nazwy funkcjom anonimowym, wskazuje na to logika i dodatkowo problemy z IE, gdzie ta funkcja może wyciec do zakresu globalnego. Funkcje anonimowe to fragmenty kodu - używane są jako zwykłe instrukcje, więc ich nazywanie jest niespójne z resztą kodu (nie nadajesz nazwy pętli for, while, czy instrukcji if albo switch - poza przypadkiem używania zakładek ale to rzadki przypadek).

Poza tym raczej unikam wstawiania funkcji anonimowych, jeśli nie jest to trywialna funkcja, wtedy definiuję funkcję na zewnątrz zakresu i odwołuję się do jej referencji. Twoja "anonimowa" funkcja zIndexCheck jest trywialna, więc ok.

Jeśli funkcja jest bardziej rozbudowana - to raczej powinna być zaimplementowana w obiekcie - w tym przypadku - przydaje się funkcja "bind" obiektu "Function". Jeśli znajduje się w globalnej przestrzeni nazw, to staje się zbędne ale kod, który nie jest obiektowy może sprawiać problemy.

O wydajności nie mam dużo do powiedzenia - jedyne na co mogę zwrócić uwagę, to tworzenie łańcuchów wywołań. Więcej zmiennych powinno być na stosie, zamiast zmuszać komputer do ciągłego wywoływania metod pobierających. Chodzi o to, że zamiast pisać x razy "cosTam($(this).css("zIndex"))" czy "$(this).css("zIndex") > cosTam" - należałoby przechwycić wartość "$(this).css("zIndex")" do zmiennej i potem jej używać, zamiast za każdym razem ją pobierać. Kod staje się czytelniejszy i bardziej wydajny.

Generalnie napisałbym Twój kod tak:

[JAVASCRIPT] pobierz, plaintext 
function maximumZindex () 
{
	"use strict";
	var highestZindex = 0;
        var allElements = frame.contents().find('body *');
 
	allElements.each(
		function  () {
                        var currentZIndex = $(this).css("zIndex");
			if (!isNaN(Number(currentZIndex))) {
				if(currentZIndex >highestZindex)
				highestZindex = currentZIndex;
			};
                        highestZindex++;
		})
 
        return highestZindex;
};
[JAVASCRIPT] pobierz, plaintext 

Ten post edytował lukasz1985 2.09.2013, 14:12:17