Prośba o ocenę skryptu JS

pytanie zadane 23 sierpnia 2015 w JavaScript przez Klik Obywatel (1,540 p.)

Witam.

Skrypt w założeniu ma sprawdzić czas reakcji użytkownika.

Link: http://doomini2.linuxpl.info/time/

Po naciśnięciu przycisku start skrypt dokonuje losowania kwadratu który będzie aktywny. Po wylosowaniu kwadratu jego kolor zostaje zmieniony na zielony i uruchomiony jest licznik czasu. Licznik zostaje zatrzymany gdy kwadrat zostanie kliknięty. Następnie wyniki zostają wyświetlone w polach oboko.

Im niższy wynik tym lepiej.

Proszę o ocenę kodu JS. Czy zastosowane przezemnie rozwiązania są najlepsze? Czy można pewne rzeczy zrobić inaczej a przez to lepiej? W szczególności interesuje mnie czy sposób obliczanie czasu który minął do kliknięcia jest prawidłowy.

Z góry dziękuję za konstruktywne opinie.

5 odpowiedzi

odpowiedź 23 sierpnia 2015 przez Comandeer Guru (601,590 p.)
wybrane 23 sierpnia 2015 przez Klik

Najlepsza

Nic Ci nie powinno wyciekać do globalnego scope. Poczytaj o IIFE: http://benalman.com/news/2010/11/immediately-invoked-function-expression/ i o pisaniu modularnego JS: http://addyosmani.com/writing-modular-js/

Zamiast się bawić w mozolne tworzenie elementów przez DOM API, po prostu wstaw je do dokumentu HTML i zastosuj coś typu: http://www.paulirish.com/2009/avoiding-the-fouc-v3/
Względnie elem.innerHTML

Zamiast bawić się w przypinanie i odpinanie zdarzeń warto zastanowić się nad event delegation: http://tutorials.comandeer.pl/js-beauty.html#delegation

for(x=0;x<30;x++)

Tym samym x staje się tzw. implied global, co w ES5+ jest traktowane jako syntax error. KAŻDA zmienna powinna być deklarowana przy pomocy słowa kluczowego var. BTW takich błędów pomaga unikać strict mode → http://tutorials.comandeer.pl/js-beauty.html#strict

btn_start.setAttribute("type","button");

Polecam tego typu rzeczy zapisać jako własności DOM, a nie atrybuty.

btn_start.type = 'button';

Warto zwrócić przy tym uwagę, że elem.setAttribute('value') nie ustawia własności elem.value, lecz elem.defaultValue (tak oto W3C spieprzyło ponad 20 lat temu DOM API formularzy i tak już zostało…).

komentarz 23 sierpnia 2015 przez Klik Obywatel (1,540 p.)

Dzięki za obszerną recenzję. Teraz czeka mnie długa lektura.

komentarz 24 sierpnia 2015 przez Klik Obywatel (1,540 p.)
edycja 24 sierpnia 2015 przez Klik

Dobra trochę poprawiłem ten skrypt. Ale nie wszystko jest jasne i oczywiste.

Nic Ci nie powinno wyciekać do globalnego scope.

Czyli rozumiem że te zmienne nie powinny być globalne:

var game_results = [];
var cont = document.getElementById("content");

Ale jak inaczej zdeklarować zmienne na początku skryptu a potem zmieniać wartości wewnątrz funkcji?

Jeśli utworzę te zmienne w funkcji to za każdym wywołaniem ich wartość będzie wracała do początkowej wartości.

Czy cały skrypt powiniem zamknąć w jeszcze jednej funkcji? Coś mi się wydaje że to chyba nie jest najlepsze rozwiązanie. Jeśli nie to jak to powinno być zrobione?

Zamiast bawić się w przypinanie i odpinanie zdarzeń warto zastanowić się nad event delegation:

Zrobiłem to "Event delegation" ale pojawiły się nowe problemy.

Przypinanie zdarzeń działa jak należy ale jak zrobić odpięcie?

cont.removeEventListener("mousedown",function(e){
	
			if(e.target.id=="active"){countTime()}
			},false);

Napisałem coś takiego ale nie działa. Kod w linku jest zmieniony więc możesz zerknąć.

Drugi problem z tym związany to taki że nie zeruje licznika czasu z jakiegoś powodu. Przy pierwszym kliknięciu jest ok ale przy drugim wywołaniu oprócz właściwego czasu otrzymuję też czas jaki minął od pierwszego wywołania. Potem przy trzecim otrzymuję już 3 czasy itd. Nie mam pojęcia dlaczego tak się dzieje. Przy zastoswaniu zwykłego addEventListener ten problem nie istnieje.

1

btn_start.setAttribute("type","button");

Polecam tego typu rzeczy zapisać jako własności DOM, a nie atrybuty.

?

1

btn_start.type = 'button';

A czy to jest jakaś różnica dla przeglądarki? W przeglądarce oba sposoby generują taki sam kod.

Warto zwrócić przy tym uwagę, że elem.setAttribute('value') nie ustawia własności elem.value, lecz elem.defaultValue (tak oto W3C spieprzyło ponad 20 lat temu DOM API formularzy i tak już zostało…).

Rzeczywiście, bardzo ciekawa uwaga.

Poczytaj o IIFE: http://benalman.com/news/2010/11/immediately-invoked-function-expression/ i o pisaniu modularnego JS: http://addyosmani.com/writing-modular-js/

A czy coś źle zrobiłem w kodzie używając IIFE? Tekst przeczytałem i jest bardzo ciekawy ale nie widzę błędu w moim kodzie?

http://www.paulirish.com/2009/avoiding-the-fouc-v3/

Całkowicie nie rozumiem co ten kod robi?

Dziękuję

komentarz 24 sierpnia 2015 przez Comandeer Guru (601,590 p.)

Ale jak inaczej zdeklarować zmienne na początku skryptu a potem zmieniać wartości wewnątrz funkcji?

Odpowiedzią jest IIFE lub moduł.

(function()
{
    //tutaj cały kod programu
}());

IIFE jest odpalane tylko raz, więc nic Ci się nie zmieni samo z siebie.

Przypinanie zdarzeń działa jak należy ale jak zrobić odpięcie?

Nie ma odpięcia. Po prostu w niektórych przypadkach nie odpalasz funkcji (np jak jakaś zmienna ma inną wartość).

Przy pierwszym kliknięciu jest ok ale przy drugim wywołaniu oprócz właściwego czasu otrzymuję też czas jaki minął od pierwszego wywołania. Potem przy trzecim otrzymuję już 3 czasy itd. Nie mam pojęcia dlaczego tak się dzieje

Pewnie nigdzie nie zerujesz timera, tylko za każdym razem po prostu przypinasz nowy.

A czy to jest jakaś różnica dla przeglądarki? W przeglądarce oba sposoby generują taki sam kod.

Różnica jest i to spora: http://jsfiddle.net/Comandeer/06cqnwo6/ → zwłaszcza przy formularza. Tak w skrócie: atrybuty to to, co umieszczamy w kodzie HTML. Własności DOM to to, w jaki sposób przeglądarki te atrybuty parsują.

A czy coś źle zrobiłem w kodzie używając IIFE?

Chodzi mi o to, że dzięki IIFE można uciec od globalnego scope.

Całkowicie nie rozumiem co ten kod robi?

Chodzi o klasę .no-js na html. Ten skrypt zamienia ją na .js. Dzięki temu można wykryć kiedy JS jest obecne i odpowiednio ostylować elementy, które nie działają bez JS. Jak mamy dużo elementów zależnych od JS, to IMO wygodniej użyć takiej sztuczki niż tworzyć wszystko przez skrypt.

komentarz 24 sierpnia 2015 przez Klik Obywatel (1,540 p.)

OK załapałem już wszystko co napisałeś.:)

Niestety nie mogę dociec przyczyny, dlaczego nie zeruje mi wywoływanego czasu. Tak jak pisałem gdy użyłem normalnego przypięcia zdarzenia to wszystko grało dopiero po tej zmianie na "event delegation" pojawiły się kłopoty. A nic nie zmieniałem w części kodu odpowiedzilnego za obliczanie czasu.

Przy każdym wywołaniu funkcji zmienne są nadpisywane więc czas powininie być liczony od nowa. Dlaczego to nie działa teraz to nie wiem :(.

komentarz 24 sierpnia 2015 przez Comandeer Guru (601,590 p.)

Hmm… Też nie widzę w tej chwili, ALE ;) znalazłem co innego:

var time2 = new Date();
var click_time = time2.getTime();

Czyżbyś nie słyszał o Date.now? ;)

komentarz 24 sierpnia 2015 przez Klik Obywatel (1,540 p.)

Słyszałem :) ale mi się zapomniało :) Już zmieniłem.

komentarz 25 sierpnia 2015 przez Klik Obywatel (1,540 p.)

Nie wiem co jest nie tak z tym "event delegation".

Jeśli wpiszę tak:

cont.addEventListener("mousedown",function(e){
	
	if(e.target.id=="active"){countTime()}
	},false);

To wtedy skrypt przy każdym kolejny klkinięciu przycisku "start" mnoży ilość wyników. Kolejnym minusem jest to że użytkownik dalej może nacisnąć podświetlony kwadrat gdyż listener nie jest odpinany.

Więc ok. Dopisałem krótką instrukcję aby id klikniętego kwadratu było usuwane po klinikięciu. Czyli wygląda to teraz tak:

cont.addEventListener("mousedown",function(e){
	
	if(e.target.id=="active"){countTime();e.target.id="";}
	},false);

I w tym momencie zachowanie licznika czasu znowu się zmieniło. Teraz po kliknięciu "start" skrypt nie mnoży już wywołań czasu, za to po każdym kliknięciu startu podaje czas jak upłynął od pierwszego kliknięcia "start" (po załadowaniu strony) a nie od ostatniego!

Co jest grane?

Super sprawa z tym "event delegation" rzeczywiście może to usprawnić przypinanie zdarzeń ale nie rozumiem dlaczego w moim przypadku powoduje to takie komplikacje. Przecież nie ingeruje to w zmienne liczące czas.

Ma ktoś jakiś pomysł dlaczego to tak miesza?

P.S. Jak w tym edytorze włączyć sprawdzanie pisowni?

odpowiedź 23 sierpnia 2015 przez Mizukage Pasjonat (21,750 p.)

Zajebiste, dodal bym do tego miejsce w ktorym trzeba trzymac kursor az do czasu pojawienia sie koloru do kliniecia.

Pokaż 7 poprzednich komentarzy

komentarz 25 sierpnia 2015 przez ScriptyChris Mędrzec (190,190 p.)

Hmm, brzmi to bardziej skomplikowane w napisaniu niż ten drugi skrypt :)

komentarz 25 sierpnia 2015 przez Klik Obywatel (1,540 p.)

Bo tak było :)

komentarz 27 sierpnia 2015 przez ScriptyChris Mędrzec (190,190 p.)

JavaScript to Twój pierwszy język programowania, którego się uczysz?

komentarz 27 sierpnia 2015 przez Klik Obywatel (1,540 p.)

Tak, nie licząc html i css.

komentarz 28 sierpnia 2015 przez ScriptyChris Mędrzec (190,190 p.)

Jak długo się uczysz? Ja JSa od czerwca, razem z HTML i CSS.

odpowiedź 23 sierpnia 2015 przez DL TD Nałogowiec (36,710 p.)

Super gra mój rekord to 0.524 s

komentarz 23 sierpnia 2015 przez DL TD Nałogowiec (36,710 p.)

Poprawka 0.478 s

komentarz 23 sierpnia 2015 przez Artur Rabenda Obywatel (1,300 p.)

0.474 s a grałem tylko pięć prób :)

komentarz 23 sierpnia 2015 przez Artur Rabenda Obywatel (1,300 p.)

0.402 s a jaki jest rekord kliknięcia ?

komentarz 23 sierpnia 2015 przez Klik Obywatel (1,540 p.)

Nie, ma żadnego rekordu to tylko tak zrobione w ramach treningu programowania :).

odpowiedź 23 sierpnia 2015 przez Patrycjerz Mędrzec (192,320 p.)

To chyba jest bardziej sprawdzenie jakości myszy i podkładki, niż reakcji

komentarz 23 sierpnia 2015 przez Klik Obywatel (1,540 p.)

A może coś więcej powiesz?

komentarz 23 sierpnia 2015 przez Artur Rabenda Obywatel (1,300 p.)

ja na 800dpi robiłem to wieć szału nie mam :D

komentarz 23 sierpnia 2015 przez Patrycjerz Mędrzec (192,320 p.)

Oczywiście żarcik. Co do kodu, to się nie odniosę, bo JS znam po łebkach. Co do funkcjonalności, to bardzo fajna aplikacja. Nie mam naprawdę się do czego przyczepić. Robi to, co powinna robić.