Ocena kodu - TicTacToe

pytanie zadane 4 maja 2017 w JavaScript przez Radekol Bywalec (2,880 p.)

Oceńcie proszę mój kod. Nie oszczędzajcie go. Bądźcie szczerzy i jak najbardziej krytyczni. Jakie złe praktyki zastosowałem w nim, jakie błędy popełniłem itp.

<html>
<head>
	<link rel="stylesheet" href="style.css">
</head>
<body>
	<header>TicTacToe</header>
	   <div id="list">
        <div class="tile"></div> 
        <div class="tile"></div> 
        <div class="tile"></div>
           <div style="clear:both;"></div>
        <div class="tile"></div> 
        <div class="tile"></div> 
        <div class="tile"></div>
           <div style="clear:both;"></div>
        <div class="tile"></div> 
        <div class="tile"></div> 
        <div class="tile"></div>
           <div style="clear:both;"></div>
       </div>
    <span id="tura" >Player: X</span>
<script src="java.js"></script>
</body>
</html>

//zmienne globalne
var grab_list = document.getElementById("list"),
    grab_tura = document.getElementById("tura"),
    flag=0,
    znakX="X",
    znakO="O",
    koniec = false,
    counter=0;
//bierzemy target eventu, element jaki bedzie klikniety
function getTarget(e){
    return e.target;
}
//funkcja wypisujaca  odpowiedni content w odpowiednim elemencie
function podmien(e) {
    
    var target, klasa, kontent;
    
    target = getTarget(e);
    kontent = target.textContent;
    
    if(kontent==""){
    if(flag==0){
      target.textContent = znakX;
      flag++;
      counter++;
      grab_tura.textContent = "Player: "+znakO;
        wygrana();
    }
    else if(flag==1){
      target.textContent = znakO;
      flag--; 
      counter++;
      grab_tura.textContent = "Player: "+znakX;
        wygrana();
    }     
   }
  }

//funkcja sprawdzajaca wygrana
function wygrana(){
    var grab_kafelki = document.getElementsByClassName("tile"),
        grab_k1 = grab_kafelki[0].innerHTML,
        grab_k2 = grab_kafelki[1].innerHTML,
        grab_k3 = grab_kafelki[2].innerHTML,
        grab_k4 = grab_kafelki[3].innerHTML,
        grab_k5 = grab_kafelki[4].innerHTML,
        grab_k6 = grab_kafelki[5].innerHTML,
        grab_k7 = grab_kafelki[6].innerHTML,
        grab_k8 = grab_kafelki[7].innerHTML,
        grab_k9 = grab_kafelki[8].innerHTML;

    if((grab_k1==znakX&&grab_k2==znakX&&grab_k3==znakX) ||
       (grab_k4==znakX&&grab_k5==znakX&&grab_k6==znakX) ||
       (grab_k7==znakX&&grab_k8==znakX&&grab_k9==znakX) ||
       (grab_k1==znakX&&grab_k4==znakX&&grab_k7==znakX) ||
       (grab_k2==znakX&&grab_k5==znakX&&grab_k8==znakX) ||
       (grab_k3==znakX&&grab_k6==znakX&&grab_k9==znakX) ||
       (grab_k1==znakX&&grab_k5==znakX&&grab_k9==znakX) ||
       (grab_k3==znakX&&grab_k5==znakX&&grab_k7==znakX)){
       grab_tura.textContent = "Wygrał X!"; 
       koniec = true;
       ending();
    } 
else if((grab_k1==znakO&&grab_k2==znakO&&grab_k3==znakO) ||
        (grab_k4==znakO&&grab_k5==znakO&&grab_k6==znakO) ||
        (grab_k7==znakO&&grab_k8==znakO&&grab_k9==znakO) ||
        (grab_k1==znakO&&grab_k4==znakO&&grab_k7==znakO) ||
        (grab_k2==znakO&&grab_k5==znakO&&grab_k8==znakO) ||
        (grab_k3==znakO&&grab_k6==znakO&&grab_k9==znakO) ||
        (grab_k1==znakO&&grab_k5==znakO&&grab_k9==znakO) ||
        (grab_k3==znakO&&grab_k5==znakO&&grab_k7==znakO)){
        grab_tura.textContent = "Wygrał O!"; 
        koniec = true;
        ending();
    }  
else if(counter==9){
        grab_tura.textContent = "Remis!"; 
        koniec = true; 
        ending();
    }
}
//funkcja resetujaca gre i cala strone
function ending(){
    var g_logo = document.getElementsByTagName("header")[0];
    g_logo.innerHTML="<a href=\"index.html\">Play again</a>";
}
// event object dla naszych elemntow li wolajacy funckcje podmien
grab_list.addEventListener('click', function(e){
    if(koniec==false) podmien(e);
}, false);

komentarz 4 maja 2017 przez UltraSF Stary wyjadacz (11,740 p.)

Na 100% da się napisać lepiej, szczególnie strasznie wygląda funkcja wygrana(). Ale niestety moje umiejętności js są zbyt małe żeby ci pomóc, wiec będziesz musiał poczekać na odp, kogoś kompetentniejszego :)

1 odpowiedź

odpowiedź 4 maja 2017 przez Tomek Sochacki Ekspert (227,510 p.)
edycja 5 maja 2017 przez Tomek Sochacki

Najlepsza

<script src="java.js"></script>

Mam tylko cichą nadzieję, że wiesz iż Java !== JS :)

A wracając do samego kodu JS, to poniżej kilka małych uwag (z góry mówię, że są to uwagi bardziej do samego sposobu pisania kodu gdyż nie mam teraz zbyt dużo czasu na analizowanie jego działania i poprawy pod tym kątem):

"zmienne globalne" i do tego jeszcze ten komentarz "nagłaśniający" deklarację :) Nie będę Ci tu dawał przykładu, ale poczytaj proszę o czymś takim jak funkcje natychmiastowe IIFE i zasięg zmiennych w JS. Dlaczego zmienne globalne są złe? Proste pytanie - czy masz 100% pewności, że żaden inny skrypt (w tym np. biblioteka) nie użyje zmiennej o tej samej nazwie zadeklarowanej również jako globalna? W przypadku bibliotek typu jQuery nie ma raczej tej obawy, ale niestety w sieci krąży wiele małych skryptów do animacji, obsługi foto itp. itd. i zdarzało mi się czasami widzieć w nich kod podobny do Twojego, bez IIFE.
Zdecyduj się na nazewnictwo funkcji i zmiennych albo PL albo EN. Polecam wersję angielską. Jeśli zaczynasz przygodę z JS to dobry moment, aby mieć obok słownik EN i na bieżąco uczyć się słownictwa właśnie w celu stworzenia funkcji/zmiennej.
W funkcji podmien() stosujesz najpierw deklarowanie zmiennych undefined (var target === var target=undefined), a dopiero później przypisujesz zmiennym wartości. Na początku kodu z kolei od razu stosujesz deklarację z jednoczesnym przypisaniem. Nie jest to błąd ale osobiście wolę stosować jedno rozwiązanie co sprawia, że kod jest bardziej czytelny. Jest to jednak moja subiektywna ocena.
Stosujesz nagminnie porównania luźne "==". Nie jest to błąd, wręcz przeciwnie w niektórych przypadkach może to być przydatne (jeśli np. spodziewasz się wartości liczbowej ale może ot być number lub string). Radzę jednak ostrożnie do tego podchodzić i w pełni świadomy sposób, a co więcej musisz być pewny co do danych wejściowych do porównania. Polecam poczytać na temat zasad niejawnej konwersji typów w JS i na początku stosować porównanie ścisłe "===". Drugie rozwiązanie porównuje nie tylko wartość lecz również jej typ, czyli 1 !== "1".

Przeanalizujmy poniższy kod:

    var grab_kafelki = document.getElementsByClassName("tile"),
        grab_k1 = grab_kafelki[0].innerHTML,
        grab_k2 = grab_kafelki[1].innerHTML,
        grab_k3 = grab_kafelki[2].innerHTML,
        grab_k4 = grab_kafelki[3].innerHTML,
        grab_k5 = grab_kafelki[4].innerHTML,
        grab_k6 = grab_kafelki[5].innerHTML,
        grab_k7 = grab_kafelki[6].innerHTML,
        grab_k8 = grab_kafelki[7].innerHTML,
        grab_k9 = grab_kafelki[8].innerHTML;

//Po co Ci tyle zmiennych, można to zapisać również inaczej:
let grabKafelki = [...document.querySelectorAll('.title')].map(v => v.textContent);

/* i później porównujesz nie wartości zmiennych lecz elementy tablicy, 
czyli np. w elemencie grabKafelki[0] masz zapisany innerText pierwszego
elementu zwróconego metodą querySelectorAll */

W powyższym kodzie (moja wersja) celowo użyłem sporo składni ECMAScript 6, abyś miał pewnego rodzaju gotowy przykład do nauki ES6 :) Jeśli chcesz to analizować to poczytaj o deklaracjach let vs var, operatorze spread/rest, metodzie querySelectorAll (oraz o różnicach między pseudotablicami z metody getElementsByClassName i querySelectorAll), metodzie Array.prototype.map, funkcjach arrow function ("strzałkowe") oraz o różnicy między innerHTML, a textContent. Wiem, sporo tego :) gdybyś potrzebował dokładniejszego wyjaśnienia któregoś z ww. elementów to daj znać (PS. część z nich omawiałem też na swoim blogu).
Co do rozbudowanych warunków w instrukcji IF. Nie będę teraz dokładnie tego analizował, ale myślę, że może to być dobry przykład abyś poszukał sposobu na zamienienie tego na jakiś jeden algorytm porównujący zamiast ręcznie wypisywać wszystkie możliwości. Czy byłoby to bardziej optymalne? Nie zawsze, ale masz gotowy materiał do nauki :) Polecam zaprzyjaźnić się z problematyką budowania algorytmów, gdyż niestety często mam wrażenie, że ten element programowania jest mało rozpowszechniony wśród programistów "webowych", a pamiętaj, że JS to nie tylko wizytówki i tysiąc animacji ale również dobry język do projektowania zaawansowanych aplikacji analityczno - obliczeniowych konkurujących z desktopowymi (tutaj mamy lepszą możliwość zapewniania pracy na różnych środowiskach). Nie wchodzimy tu oczywiście w kwestie wydajności - nie to jest przedmiotem tej dyskusji.
funkcje getTarget i podmien (i znowu raz PL, raz EN :) mogłyby być umieszczone bezpośrednio z funkcji anonimowej w metodzie addEventListener. Funkcja wygrana() w sumie też, ale tutaj można by ją pozostawić jako "oddzielną". Widzę, że chcesz tworzyć dużą ilość różnych funkcji, ale uważaj, ponieważ to co zrobiłeś to nie tzw. programowanie funkcyjne (tam chodzi nie o dużą liczbę funkcji ale również o ich odpowiednie zachowanie). Ponadto funckaj getTarger nie jest w zasadzie potrzebna. Poczytaj sobie o tym co pobiera za argument funkcja zwrotna metody addEventListener.

Dobra, więcej już mi się nie chce :) Niech inni się też coś wypowiedzą. Gdybyś miał jakieś konkretne pytania to daj znać co dokładniej wyjaśnić.

Pozdrawiam

komentarz 5 maja 2017 przez Radekol Bywalec (2,880 p.)

Dzięki za poświęcony mi czas i obiektywną krytykę:)

Niektóre rzeczy już poprawiłem a o reszcie muszę przeczytać. Mam jednak pytanie co do innerText. Czytałem, że należy raczej unikać tego zapisu gdyż przeglądarki go nie podtrzymują, jest wolny i nie za bardzo zgrywa się z CSS. Czy muszę go używać przy zapisie:

"...map(v => v.innerText);"

PS: Dasz mi linka do twojego bloga? :)

komentarz 5 maja 2017 przez Tomek Sochacki Ekspert (227,510 p.)

Tak, masz rację, trochę nie rozważnie podałem Ci innerText. Jest to właściwość nie będąca standardem i jest implementowana "indywidualnie" przez przeglądarki. W niektórych poradnikach widnieje propozycja sprawdzania czy istnieje innerText i jeśli tak to użycie go, a jeśli nie to przejście na textContent. Wynika to głównie z tego, że textContent jest standardem, ale nie jest obsługiwany przez przeglądarki IE starsze niż IE 9. Obecnie w zasadzie nie trzeba się już nimi chyba przejmować. Osobiście projektuję właśnie dedykowaną aplikację obliczeniową, dla której zakładam obsługę od IE9 (choć teraz sam się zacząłem zastanawiać czy w ogóle nie odpuścić sobie IE... ale to temat na osobną dyskusję).

Stosuj więc lepiej textContent czyli "...map(v => v.textContent);".

Co do szybkości to szczerze mówiąc nie wgłębiałem się w tą tematykę w odniesieniu do metod textContent, innerText, innerHTML itp.

Czyli reasumując, do wyciągania tylko zawartości węzła tekstowego: textContent, a jeśli potrzebujesz pobrać zawartość HTML danego węzła to innerHTML.

Pozdrawiam

ad PS. blog: poradnik.drogimex.pl