Ocena kodu źródłowego - mój pierwszy projekt(gra snake)

Question

Witam wszystkich :>

Proszę was o ocenę mojego pierwszego projektu, który napisany jest za pomocą JS w elemencie canvas. Projekt to gra snake. Poniżej zamieszczam kod źródłowy na GitHub i samą grę.

Kod źródłowy - https://github.com/KarolHermanowski/snake/blob/master/snake/script.js

Gra - https://karolhermanowski.github.io/snake/snake/

Comments

Zła kategoria pytania.

---

Poprawione, ale proszę zawsze dobierać poprawne kategorie pytań - w ten sposób można uniknąć bałaganu.

---

Przepraszam, to przez moją nie uwagę. Potem cały dzień w pracy byłem i nie miałem kiedy poprawić. Dziękuję za zmianę.

:>

---

@MrHerman,
Taka drobna uwaga - czasami kolory jabłek, czy tam kwadratów są zbyt ciemne

http://prntscr.com/j04mv2

Answer 1

Fajna gierka :)

A co do oceny kodu to proponuję zwrócić uwagę na kilka kwestii:

1. Canvas fajnie byłoby zrobić responsywny aby możliwa była gra na mobilkach. Można to łatwo osiągnąć np. poprzez ustalanie wymiarów canvasu względem wymiarów okna, np. z jakimiś brzegowymi warunkami na max, i np. informacją o braku możliwości gry przy ekranie mniejszym niż .... Jak to zrobić? Na przykład poprzez odczytanie wymiarów okna width i height i na ich podstawie zdefiniowanie wymiarów. Nie do końca można to robić np. w CSS poprzez 80%, 80vw itp. ponieważ aby na canvas była zachowana dobra ostrość to wymiary powinny być podane z dokładnością do 1px. Łatwo można to jednak zrobić:

   const width = window.innerWidth;
   width; //1318
   
   const canvasWidth1 = width * 0.8;
   const canvasWidth2 = ~~(width * 0.8);
   const canvasWidth3 = Math.floor(width * 0.8);
   
   canvasWidth1; //1054.4
   canvasWidth2; //1054
   canvasWidth3; //1054

   Zwróć uwagę, że musisz zaokrąglić wynik do pełnych PX np. operatorem ~~ albo Math.floor co jest w przypadku liczb całkowitych dodatnich rozwiązaniem zamiennym (tak w skrócie mówiąc).

2. Jak już zrobisz responsywny canvas to może warto dodać jego dynamiczną zmianę np. przy zmniejszaniu okna - zerknij na zdarzenie 'resize' dla window i pamiętaj np. o wskazaniu jakiś warunków brzegowych na max width canvasu i min od którego w ogóle będzie widoczny. Pamiętaj tez, że w takiej grze canvas nie powinien być większy niż wymiar okna, tj canvas W i H nie mogą być większe niż wymiary okna aby w ogóle możliwe było wygodne granie.
3. Masz obsługę wyłącznie klawiaturą. Na PC oki, ale może warto dodać opcję włączenia klawiaturki ekranowej i zrobić np. strzełki do ruszania wężem na mobilkach :) Można też dodać np. opcję ustawiania szybkości węża itp.
4. Kod: https://github.com/KarolHermanowski/snake/blob/master/snake/script.js#L15 dlaczego tablicę deklarujesz przez let? Poczytaj sobie dokładnie o let i const, pisałem też o tym kiedyś na blogu (poradnik.drogimex.pl). Jesli dasz const snake = []; to nadal będzie możliwe dodawanie i usuwanie elementów tablicy. Const nie pozwoli tak w dużym skrócie nadpisać snake nową zmienną, ale pozostawi możliwość modyfikacji istniejącej (co innego gdyby był to np. string).
5. Wszystkie parametry jak kolor, szybkość, rozmiar węża itp. można fajnie wyrzucić jako ustawiane przez usera z parametrami domyślnymi. Oczywiście to tylko sugestia, ale skoro już się napracowałeś nad projektem to fajnie go udoskonalić i masz fajną pozycję w portfolio :) Poćwiczysz jednocześnie pobeiranie wartości z elementów DOM, jakąs prostą walidację itp.
6. Komentarze staraj się pisać po angielsku. To nie wymóg ale ogólnie przyjęta zasada i pozwoli Ci poćwiczyć ang.
7. Kod: https://github.com/KarolHermanowski/snake/blob/master/snake/script.js#L39 znowu mamy let do deklaracji obiektu. Uwaga ta sama co do let dla deklaracji tablicy. Ale jak poczytasz coś więcej o const/let to szybko załapiesz o co chodzi :)
8. stosuj lepsze formatowanie kodu, zacznij pracować z jakimś IDE i np. eslint.
9. W warunkach IF raczej stosuj porównanie ścisłe === a nie ==. W necie jest wiele informacji na ten temat, ja na blogu też chyba kiedyś coś pisałem o tym i o konwersji typów. Generalnie === jest bezpieczniejsze. 
10. Masz kod:

    if(time-lastTime >= 1000/fps)

    Nie analizowałem dokładnie czy fps jest stałe czy zmienne, ale jeśli stałe to po pierwsze daj je jako const, a po drugie ten warunek 1000/fps wrzuć do osobnej stałej bo w Twoim wypadku każdorazowo dokonujesz nowego obliczenia, co przy stałym fps jest niepotrzebne (chyba, że się to zmienia to oki).
11. Masz kod:

    if(speedX != 0 || speedY != 0){
    		return true;
    	}else{
    		return false;
    	}

    a dlaczego nie po prostu:

    return (speedX !== 0 || speedY !== 0);

    Moim zdaniem jest to czytelniejsza forma bo od razu widzę, że interesuje mnie true/false, a przy Twoim rozwiązaniu zaczynam się zastanawiać czy nie powinno tam być jakieś dodatkowej logiki, czy to na pewno kompletny kod itp.
12. Kod:

    let arrowUp = 38;
    let arrowDown = 40;
    let arrowRight = 39;
    let arrowLeft = 37;

    to się nie zmienia ,więc tu powinny być const.
13. Funkcję losującą https://github.com/KarolHermanowski/snake/blob/master/snake/script.js#L194 deklarujesz po jej użyciu. W przypadku deklaracji poprzez function fn(){} jest to możliwe, ponieważ zachodzi hoisting funkcji, ale już przy stosowaniu zapisu np. const fn = () => {} taki kod nie zadziała. Generalnie osobiście jestem zwolennikiem, aby używać tylko funkcji, które wcześniej zostały zadeklarowane/zaimportowane. Pozwala to łatwiej czytać i analizować kod.
14. ctx.font = "50pt Arial"; - dlaczego jednostka "pt"?
15. Generalnie jeszcze dwie sugestie - podziel kod na mniejsze pliki i poczytaj np. o webpack (dobry poradnik o tym jest opublikowany np. na stronie nafrontendzie.pl gdzie kolega Bartek opisuje ładnie kolejne etapy konfiguracji). Druga to poczytaj o babel i transpilacji kodu, ale to wyjdzie przy okazji webpacka.

Generalnie projekt fajny, jak dopracujesz parę szczegółów to myślę, że będziesz miał ciekawą pozycję w portfolio. Może dać też więcej opcji konfiguracyjnych np. wybór koloru tła itp. Jest czym się pobawić w takim teoretycznie prostym projekciku, a jak widzisz można z tego zrobić całkiem fajną apkę :)

Powodzenia!

Comments

Dziękuję za te uwagi. Teraz przynajmniej wiem czego powinienem się nauczyć, czego poszukać i pokazałeś że mogą ją troszeczkę rozwinąć. Zastosuje się do tych uwag :>

---

Rozwinąć można zawsze :) To tak samo jak często wiele osób pisze "Tylko nie rób oklepanej, banalnej listy TODO", a niedawno jednemu forumowiczowi udowadniałem, że takie TODO to tak na prawdę może być mocno rozbudowana apka, trzeba tylko pomyśleć co fajnego można dodać :)

Answer 2

Po pierwsze zmień kategorie. Po drugie formatowanie jest spaczone. Albo taby albo spacje. Ewentualnie ide która zniweluje ten błąd. Jeszcze gdybyś użył es6 I class A następnie użył webpacka do podziału kodu. Znacznie lepiej i łatwiej było by się wgryźć w kod. Taka moja opinia :)

Comments

Po za tym gra nie działa na mobilki A powinna :)

Answer 3

Stary, to jest zajebiste! : D
Jeśli jesteś wstanie to wrzuć to gdzieś, gdzie inni będą mogli pobierać na urządzenia mobilne. 

Nie jestem wstanie dokonać oceny merytorycznej, bo wiedzę mam ewidentnie mniejszą, a do tego programuje w zupełnie innym języku, więc mam raczej ogólne pojęcie co się dzieje w Twoim kodzie. Jednak naprawdę mi się to podoba! To co robisz reprezentuje zdecydowanie najlepszy nurt nauki oraz promocji programowania. Bo pokazuje kodowanie od tej fajniejszej, bardziej dostępnej i budzącej zainteresowanie strony.