[CODE REVIEW] Gra w kółko i krzyżyk. Początki javascript.

pytanie zadane 28 września 2016 w JavaScript przez Codeboy Stary wyjadacz (12,120 p.)
edycja 19 października 2016 przez Codeboy

Zacząłem uczyć się javascriptu i postanowiłem zrobić grę kółko i krzyżyk żeby nie była to sucha nauka.

Proszę o ocenę i wytkniecie wszelkich błędów ;) Co powinienem ulepszyć, czego się douczyć. Dopiero zaczynam z javascriptem wiec będę wdzięczny za wszystkie wskazówki. Najbardziej boli mnie sprawdzanie czy divy w linii są takie same, bo wygląda to co najmniej obszernie(no ale to pierwsze na co wpadłem) ;) Napisałem to w jeden wieczór, także jestem pewien ze jest sporo do poprawy. Dziękuje za poświęcony czas.

W miedzy czasie przejrzę inne tematy z code-review tej gry na forum ;).

https://github.com/NoMoExcuses/Kolko-i-krzyzyk

http://www.kolko-i-krzyzyk.awsome.pl/

1 odpowiedź

odpowiedź 28 września 2016 przez ScriptyChris Mędrzec (190,190 p.)
wybrane 30 września 2016 przez Codeboy

Najlepsza

Co do JavaScriptu:

całość skryptu zamknij w IIFE (chyba, że przepiszesz na ES6, to w bloku) => http://benalman.com/news/2010/11/immediately-invoked-function-expression/ oraz http://exploringjs.com/es6/ch_core-features.html#_from-iifes-to-blocks
skrypt z <head> przenieś na koniec <body> i wtedy możesz zrezygnować z window.onload
w pętlach stosujesz zmienne globalne (nie dodając słowka var, gdy dana nazwa zmiennej nie została wcześniej zadeklarowana, JavaScript domyślnie tworzy daną zmienną w obiekcie window - nie jest to wskazane, zwłaszcza jeśli nie jesteś tego świadom) - nie rób tego. W pętlach typu for stosuj albo var albo const / let => https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Statements/var
zapisy typu:
```
pola = pola +'<div id="'+i+'" class="pole" style="float: left" onclick="postaw('+i+')"><p>'+i+'</p></div>';
```
gdy tylko możesz nie styluj HTMLa w formie inline lecz nadawaj klasy i styluj w CSS => http://www.standardista.com/css3/css-specificity/ dodatkowo http://forum.pasja-informatyki.pl/109776/html-class-czy-id?show=109776#q109776
Nie dawaj w HTMLu atrybutów odpowiedzialnych za obsługę eventów typu onclick / onmouseover - do tego służy JavaScriptowy addEventListener => https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener
```
document.getElementById(nr).setAttribute("class", "off");
```
Jeśli dodajesz klasę, to możesz to zrobić za pomocą element.classList.add('nazwa_klasy') i usuwanie poprzez element.classList.remove('nazwa_klasy') => https://developer.mozilla.org/en-US/docs/Web/API/Element/classList
stosuj potrójny operator porównania === / !== => https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Operators/Comparison_Operators
do tego polecam włączyć strict mode

Pokaż 37 poprzednich komentarzy

komentarz 16 października 2016 przez Codeboy Stary wyjadacz (12,120 p.)

W funkcji insert, szukany jest element, za każdym razem, gdy ktoś się ruszy.

W funkcji ifwin, szukane są elementy x9, prawie za każdym razem, gdy ktoś się ruszy.

Tak, ale zacytowałeś czyszczenie "aktywności" pól, a to wykonywane jest wtedy gdy zajdzie wygrana ;) A to chyba co innego niż to co teraz opisujesz.

Tak - funkcja sprawdzająca powinna sprawdzać kto wygrał, niezależnie od tego, która jest runda.

Ten warunek dałem po to bo jaki sens ma sprawdzanie wygranej gdy runda < 3 ?

komentarz 16 października 2016 przez ScriptyChris Mędrzec (190,190 p.)

Przeszukiwanie DOM za każdym razem - jest 'kosztowne' dla procesora. Znacznie ładniejszym rozwiązaniem byłoby zapakowanie wszystkich pól do tablicy

Co masz na myśli mówiąc "za każdym razem"? To jest wykonywane tylko raz, jeśli zaszła wygrana.

Argeento zapewne miał na myśli to, że z każdą iterację pętli odnosisz się do danego elementu trzykrotnie i w każdej iteracji odnosisz się do DOM. Czyli:

for(var i=0; i<=8; i++) 
{
    document.getElementById(i).removeEventListener("click", insert);
    document.getElementById(i).classList.remove('field');
    document.getElementById(i).classList.add('off');
}

Argeento wskazał Ci, abyś najpierw zrobił sobie tablicę zawierającą elementy, po których będziesz sobie w pętli iterował (będziesz je mieć już w pamięci, przez co z każdą iteracją JS nie będzie musiał sięgać do DOM, aby je pobrać).

Poza tym, po co trzykrotnie pobierać element, aby wziąc jego classList albo coś innego, skoro możesz pobrać element, umieścić go w zmiennej i na tej zmiennej działać. Czyli:

var element = document.getElementById(i);
element.removeEventListener("click", insert);
element.classList.remove('field');
element.classList.add('off');

, pobierasz element do zmiennej raz, a potem tylko odnosisz się do niego przez referencje (jako, że JS elementy DOM traktuje jako obiekty, a do tych JS z kolei odnosi się przez referencje) poprzez tą zmienną i robisz z tym elementem co tam chcesz - przez co nie musisz za każdym razem odnosić się do DOM, aby sobie coś z elementu odczytać czy w nim zmienić.

rozpoczynasz nowe bloki na 4* różne sposoby, wybierz jeden i bądź konsekwentny

Owszem, wybór jednego stylu pisania jak najbardziej, ale czytałem coś takiego: https://github.com/airbnb/javascript#blocks

i jest tam napisane, że jedno linijkowe instrukcje po if można pisać zarówno w jednej linijce jak i wielu :) Więc IMHO, jak komu wygodniej. Chociaż rzeczywiście lepiej trzymać się jednego stylu zapisu. Tak tylko nawiasem rzucam.

Przypomniałem też sobie, że muszę refaktoryzować kod na GitHub, bo chyba nie spełnia tych wymagań.

komentarz 16 października 2016 przez niezalogowany

if(round>=3 && round<=10) ifWin("o");

Zatem niech sprawdzanie licznika rund będzie pierwszą czynnością w ifwin. Funkcja insert jest od wstawiania, funkcja ifwin od sprawdzania. Niepotrzebnie to mieszasz.

Druga sprawa, po co sprawdzać czy aby na pewno nie jest runda 11 lub dalej

Więc IMHO, jak komu wygodniej

IMO bycie konsekwentnym, to bycie konsekwentnym. Pod ifem jednak zawsze spodziewam się zamknięcia jego bloku.

komentarz 16 października 2016 przez Codeboy Stary wyjadacz (12,120 p.)

Dzięki Krzycho za wytłumaczenie ;)

Przypomniałem też sobie, że muszę refaktoryzować kod na GitHub, bo chyba nie spełnia tych wymagań.

??? ;D

komentarz 16 października 2016 przez Codeboy Stary wyjadacz (12,120 p.)

Przeszukiwanie DOM za każdym razem - jest 'kosztowne' dla procesora. Znacznie ładniejszym rozwiązaniem byłoby zapakowanie wszystkich pól do tablicy

Użyłem pętle globalnie i zastosowałem to do wszystkich możliwych funkcji ;)

*uważaj, zmienna field może Ci namieszać przy tworzeniu tablicy.. albo na odwrót. Takie życie gdy nie masz scopa z funkcji / oddzielnej klasy

Wydaję się, że nie namieszało ;)

*zawsze parsuj zmienne przychodzące z DOM API // parseInt( e.target.id );

Hmmm, ale tylko wtedy kiedy ta zmienna ma być liczbą prawda? ;)
W moim kodzie tylko to co napisałeś do sparsowania, nie mylę się?

Zatem niech sprawdzanie licznika rund będzie pierwszą czynnością w ifwin. Funkcja insert jest od wstawiania, funkcja ifwin od sprawdzania. Niepotrzebnie to mieszasz.

if (round%2==0) {
			
			field[i].innerHTML="<img src='img/krzyzyk.png'>";
			if(round>=3) ifWin("x");
			return;
			
		}

Nie za bardzo rozumiem gdzie tu się miesza, przecież po prostu jest warunek kiedy wywołać sprawdzenie, czyli kiedy runda >3. Chyba, że chodzi Ci o to żeby całkiem wyrzucić, z funkcji wstawiania(insert), wywołanie funkcji sprawdzającej(ifWin) i zrobić to osobno.