Proszę o ocene

Wątek przeniesiony 2016-03-06 18:17 z Newbie przez flowCRANE.

  • Rejestracja: dni
  • Ostatnio: dni
0

Witam

Proszę o ocenę pod względem dobrych praktyk / czytelności kodu. Zawsze mam z tym problem więc chciałem napisać coś "nieco" większego żeby ktoś mógł to ocenić.
Napisałem gre "Snake" używając canvas i javascript. Możemy wybierać sobie poziom szybkości i opcje przechodzenia przez ściany :
Link : http://layoutpawelp.comuf.com/snake/

Czekam na wszelkie słowa krytyki :)

PS: Sterowanie wsadem i strzałkami

Patryk27
  • Rejestracja: dni
  • Ostatnio: dni
  • Lokalizacja: Wrocław
  • Postów: 13042
0
  1. Dziwne formatowanie kodu.
  2. Brak komentarzy (JSDoc).
  3. Meal.checkMeal? checkCollisionWithSnake od razu mówi, co ta metoda robi, a checkMeal nic nie daje do zrozumienia.
  4. Raczej nie pisz canvasWidth/canvasHeight, tylko pójdź poziom abstrakcji wyżej i zrób sobie klasę Map z polami width oraz height, i tę klasę wstrzykuj.
Kopiuj
        switch(this.snakeDirection){
			case DIRECTION.LEFT:
				newX--;
			break;
			case DIRECTION.RIGHT:
				newX++;
			break;
			case DIRECTION.UP:
				newY--;
			break;
			case DIRECTION.DOWN:
				newY++;
			break;			
		}

Tak w ramach zadania: napisz to bez wykorzystania jakiejkolwiek instrukcji warunkowej ;-)
6. Musiałem 20 sekund zastanawiać się, co robi metoda Snake.checkWalls. Żyjemy w XXI wieku, istnieją terabajtowe dyski, więc nie oszczędzaj na nazwach metod. checkCollisionWithBoundaryWalls może i wygląda przydługawo, ale za to nie trzeba zerkać do kodu, aby wiedzieć, co ta metoda robi.
7.

Kopiuj
		if (head.x >= width || head.x < 0 || head.y >= height || head.y < 0){
			return true;
		}
		return false;

Nah, nah. return (head.x >= width || ...);.
8. Wciąż nie czaję, co robi metoda Snake.checkSnakeBody.
9. 0 != i warunki w stylu yody są dobre dla początkujących (prościej zauważyć pomyłkę =/==), lecz nie powinieneś ich wykorzystywać - pisz i != 0.
10. Pozbądź się luźnych funkcji - nie clear, tylko CanvasHelper.clear; nie startGame, tylko Game.start (...).
11. window.addEventListener('keyup', function (e) { to w końcu wykorzystujesz to jQuery czy nie?
12. Imho nazwa Meal nie pasuje - prędzej Food.
13. A to, że startGame jako callback przyjmuje funkcję uruchamianą podczas przegrania, to już w ogóle jakieś mistrzostwo. Dlaczego callback? Czy kiedykolwiek będzie potrzeba przekazania tam innej funkcji, niż ta aktualna?

  • Rejestracja: dni
  • Ostatnio: dni
0

Co do punktu 13 : Jak można inaczej zobaczyć kiedy gra się skończy?

Maciej Cąderek
  • Rejestracja: dni
  • Ostatnio: dni
  • Lokalizacja: Warszawa
  • Postów: 1264
0

W event listenerze chyba zapomniałeś o else if (jeśli już zostawić instrukcje warunkową). Poza tym nie powinieneś pchać logiki do event listenera, powinien on przekazać sterowanie do odrębnej metody, np:

Kopiuj
window.addEventListener('keyup', function (e) {
		changeSnakeDirection(e.keyCode);
    });

No i 'keyup' w grze zręcznościowej to chybiony pomysł - zamień na keydown.

Patryk27
  • Rejestracja: dni
  • Ostatnio: dni
  • Lokalizacja: Wrocław
  • Postów: 13042
0

Jak można inaczej zobaczyć kiedy gra się skończy?
No wystarczy przekleić ten kod, który aktualnie masz w callbacku, do miejsca wywołania callbacka - albo lepiej: od razu wydzielić do osobnej metody, ale nie bawić się w funkcje anonimowe w takim błahym przypadku.

Sarrus
  • Rejestracja: dni
  • Ostatnio: dni
  • Postów: 2512
1

Uwagi odnośnie samej gry.

  1. Masz przycisk Nowa gra, ale już poziomy szybkości Slow, Medium i Fast. Zdecyduj się na polski lub angielski.
  2. Poziom szybkości Fast jest dla mnie odrobinę za szybki, a poziom Medium jest ślimaczy i nie daje mi przyjemności z gry.

Zarejestruj się i dołącz do największej społeczności programistów w Polsce.

Otrzymaj wsparcie, dziel się wiedzą i rozwijaj swoje umiejętności z najlepszymi.