Tetris w js. Ocena kodu

Question

Witam,
Niedawno skończyłem pisać aplikację/grę tetris. Postanowiłem to zrobić w stylu javy, czyli każdą klasę zapisywać w osobnym pliku oraz pilnować widoczność metod i całość połączyć w jednym skrypcie. Starałem się, aby cały kod był czytelny oraz napisany w es6 (ale nie zastosowałem klas z es6, powinienem?). Proszę o każdą opinie (nawet surową) abym mógł poprawić się w przyszłości 
Pozdrawiam!!
Test: Tetris
kod: kod

Answer 1

1. TetrisComputing.js #L134 - L135 - default parameters - w całym projekcie ich brakuje
2. TetrisComputing.js #L173 - L180 Dość brutalne podejście

   let getRandomColor = () => {
       let color;
       do {
           color = COLORS[ Math.floor( Math.random()*COLORS.length ) ];
       } while(color == nextElemColor);
   
       return color;
   }

   Może wpierw przefiltrować kolory, a później losować

   let getRandomColor = () => {
       const availableColors = COLORS.filter(color => color !== nextElemColor);
       const color = availableColors[ Math.random() * availableColors.length >> 0]
   
       return color;
   }
   

3. etrisComputing.js #L204 - L206 Wystarczy

   currentIdInterval = setInterval(moveElemDown, 1000);
   

4. TetrisComputing.js #L234 - L242 

   let checkIfPlayerLost = () => {
       let y = 0,
           x = SIZE_X-1;
       while ( x >= 0 ) {
           if ( board[y][x].isLocked ) return true;
           x--;
       }
       return false;
   }

   includes?

   let checkIfPlayerLost = () => {
     const firstRow = board[0]
     return firstRow.includes(square => square.isLocked)
   }
   

5. Score.js #L57 - L72

   switch (rows) {
       case 1:
           addedScore+=100;
           break;
       case 2:
           addedScore+=200;
           break;
       case 3:
           addedScore+=400;
           break;
       case 4:
           addedScore+=800;
           break;
       default:
           return false;
   }

   Tutaj chyba można uprościć do

   addedScore += Math.pow(2, rows - 1) * 100
   

6. GameMenu.js #L66

   return ( (t-(t%60))/60 );

   Czasem się jakieś potwory zdarzają ; )

   return (t - t % 60) / 60;
   

7. GameMenu.js #L84 - L85 Czemu nie template string?

8. GameMenu.js #L99 - L101 Może forEach? + spójrz na entries

   for (const key of Object.keys(generalStats)) {
       createCookie(key.toLowerCase(), generalStats[key], COOKIE_EXPIRE_DAYS);
   }

Ogólnie bardzo imperatywnie piszesz. Higher order functions FTW!

Przydałby się jakiś linter do formatowania kodu, spory bałagan tam się zrobił. 

Używaj === zamiast ==  bo się można czasami zdziwić

Comments

W pierwszej kolejności dziękuje za poświęcony czas. A oto moje odpowiedzi na twoje sugestie:

1. TetrisComputing.js #L134 - L135 - default parameters - w całym projekcie ich brakuje

1. Do większości funkcji zrobiłem domyślne parametry, zupełnie wyleciało mi to z głowy. A do tych metod co wymagają więcej niż 3 parametry zrobię za pomocą obiektu "options"

TetrisComputing.js #L173 - L180 Dość brutalne podejście...

2. Nie wiem dlaczego ale zapominam używać tych cudownych metod, również nie wiedziałem o tym hack-u jak '>>', w przyszłości może bardzo się przydać

currentIdInterval = setInterval(moveElemDown, 1000);

3. Zapomniałem to przywrócić do skróconej postaci bo coś tam jeszcze kombinowałem. Już naprawione.

let checkIfPlayerLost = () => {
  const firstRow = board[0]
  return firstRow.includes(square => square.isLocked)
}

4. Znów zapomniałem użyć tutaj tych metod. Zrobiłem bardzo podobnie jak pokazałeś ale użyłem metody some 

const firstRow = board[0];
return firstRow.some( v => v.isLocked );

switch (rows) {
    case 1:
        addedScore+=100;
        break;
    case 2:
        addedScore+=200;
        break;
    case 3:
        addedScore+=400;
        break;
    case 4:
        addedScore+=800;
        break;
    default:
        return false;
}

addedScore += Math.pow(2, rows - 1) * 100

5. Punktacje zrobiłem wg. tego wzoru co napisałeś, ale specjalnie zastosowałem tutaj switch aby w przyszłości móc to łatwo edytować i np. zmienić punktacje.

return ( (t-(t%60))/60 );

6. Nawiasy ładnie ustalały kolejność wykonywania działań ale było ich tam zdecydowanie za dużo więc już zmieniłem.

GameMenu.js #L84 - L85 Czemu nie template string?

7. W sumie myślałem, że się to będzie zlewać i nie będzie widać co tam jest napisane bo słabo widać te . , wśród liter,  ale mimo wszystko zmieniłem bo tak powinno być.

GameMenu.js #L99 - L101 Może forEach? + spójrz na entries

for (const key of Object.keys(generalStats)) {
    createCookie(key.toLowerCase(), generalStats[key], COOKIE_EXPIRE_DAYS);
}

8. Masz racje, wyglądało to 2/10 zmieniłem na 

let key;
let val;
Object.entries(generalStats).forEach((arr) => {
	[key="",val=""] = arr;
	createCookie(key.toLowerCase(), val, COOKIE_EXPIRE_DAYS);
});

Ogólnie bardzo imperatywnie piszesz. Higher order functions FTW!

Uważasz, abym zmienił styl pisania (nawet nie wiedziałem, że takie coś istnieje) z imperatywnego na Higher-Order Functions. Masz może jakieś ciekawe materiały na ten temat? 

Przydałby się jakiś linter do formatowania kodu, spory bałagan tam się zrobił. 

Linter już się instaluje (kiedyś szukałem takiego narzędzia tylko nie wiedziałem jak się nazywa). 

Używaj === zamiast ==  bo się można czasami zdziwić

=== jest czasami problematyczne bo trzeba często typy danych konwertować. Ale mimo wszystko jest to bezpieczniejsze.

Pozdrawiam i jeszcze raz dziękuje !

---

Uważasz, abym zmienił styl pisania (nawet nie wiedziałem, że takie coś istnieje) z imperatywnego na Higher-Order Functions.

Kwestia preferencji, osobiście ciągnie mnie w stronę programowania funkcyjnego.

let key;
let val;
Object.entries(generalStats).forEach((arr) => {
    [key="",val=""] = arr;
    createCookie(key.toLowerCase(), val, COOKIE_EXPIRE_DAYS);
});

Destructuring można zastosować podczas wprowadzania argumentów do funkcji

Object.entries(generalStats).forEach(([key, val]) => {
    createCookie(key.toLowerCase(), val, COOKIE_EXPIRE_DAYS);
});

Answer 2

• W JS nie ma typu integer, więc nie kłam w JSDoc.
• Poza tym Object, nie object.
• Jaki jest sens w wystawianiu całej klasy przez var? Jeśli chcesz stosować ES6, to tu powinno być const. Idealnie byłoby to zrobić na modułach ES.
• Czemu część funkcji jest deklarowana normalnie a część jako arrow functions? Czym to się różni?
• Czemu funkcje są deklarowane przez let? Przecież się nie zmienią w trakcie trwania programu, więc const wydaje się sensowniejszym wyborem.
• Jeśli funkcja przyjmuje więcej niż 3 parametry, warto się zastanowić nad wzorcem z przekazywaniem obiektu options (jak to robi np. jQuery.ajax).
• Czemu cały "moduł" zwraca init(), podczas gdy init nic nie zwraca?
• Poza tym przy takim zapisie kodu this.metoda jest bardzo nieczytelna. Zwracałbym obiekt z publicznym API.
• Raczej istnieje konwencja, że jedno let = jedna zmienna.

Comments

1. Niedopatrzenie
2. Jak wyżej
3. O const nie wiedziałem. Edytuje i zrobię na modułach
4. Jeżeli chodzi o same klasy to miałem problem z "jakaśKlasa is not a constructor". A co do metod, jak je definiowałem(a to było zaraz po tym błędzie, to było na zasadzie kopiuj wklej
5. Dobrze wiedzieć
6. jak wyżej
7. powinno być samo init() ?

Masz może jakieś ciekawe artykuły, książki na temat pisania aplikacji w js ? Mogą być angielskojęzyczne lub po polsku.

---

1. powinno być samo init() ?

Mówiąc szczerze przy takim patternie widziałbym to inaczej:

const MyModule = ( function() {
	function jakasFunkcjaPrywatna() {}

	function jakasInnaFunkcjaPrywatna() {}

	return {
		jakasMetodaPubliczna() {},
		innaMetodaPubliczna() {}
	};
}() );

Ale w sumie trzaskałbym to na modułach.

 Masz może jakieś ciekawe artykuły, książki na temat pisania aplikacji w js ? Mogą być angielskojęzyczne lub po polsku.

http://helion.pl/ksiazki/javascript-programowanie-zaawansowane-tomasz-comandeer-jakut,jascpz.htm ( ͡° ͜ʖ ͡°) Albo klasyka: http://exploringjs.com 

---

• Raczej istnieje konwencja, że jedno let = jedna zmienna.

A dlaczego uważasz, że stosowanie jednego let do wielu zmiennych jest nie dobrą praktyką? (Sam tak najczęściej robię, dlatego ciekawi mnie dlaczego nie jest to zalecane).

---

https://github.com/airbnb/javascript#variables--one-const

---

W sumie przekonałeś mnie :)

Dzięki za link, szczerze mówiąc nie znałem tego więc pozycja jak najbardziej warta poczytania.

Answer 3

Po co to wstawiałeś, gram już 15 minut a mam dużo ważnych rzeczy do zrobienia! Fajnie wykonane i dobrze się gra :D