Kółko i Krzyżyk - JS ES5 - [Code Review]

Question

Hej. Chciałbym pochwalić się swoją małą aplikacją napisaną w JavaScript. Celowo kod jest napisany w samym ES5. Jednocześnie chciałbym prosić wszystkich bardziej doświadczonych o ocenę kodu i wypunktowanie mi moich błędów. Apka jest dla dwóch osób, nie ma możliwości grania z komputerem, a to dlatego że chciałem skupić się na samym języku, a nie poświęcać czasu logice.

Moje repozytorium znajdziecie tutaj: https://github.com/PanLydka/tic-tac-toe-ES5

Z kolei wersję live można przetestować tutaj: https://panlydka.github.io/tic-tac-toe-ES5/

 

Będę bardzo wdzięczny za każdą uwagę i na pewno wszystkie poprawki wniosę. Bardzo możliwe, że potem stworzę nowe repo z tym samym kodem, ale przepisanym na ES6. :)

Comments

'Teraz rusza się...' nie do końca dobrze działa, sprawdź sobie.

---

Chodzi o animację? No tak, jak ktoś szybko przeklika wszystkie pola, to w pewnym momencie może się nie wyświetlić, ale nie widzę tu problemu, bo jakby ktoś miał grać normalnie, to wszystko działałoby okej.

W dodatku nie mam pomysłu jak mógłbym to zrobić inaczej, żeby uniknąć takiego zawirowania. :)

---

Jeśli kliknę start to jest napisane, że ruch ma gracz O, wykonuję ruch i znowu jest napisane, że jest ruch dla O, a dodaje się X. Jeśli zagram po raz drugi, to informacje odnośnie tury są odwrócone, tj. przy ruchu X wcześniej jest informacja, że ruch ma O. :)

---

Wczoraj coś poprawiałem i musiałem popsuć i umknęło to mojej uwadze, za jakieś 3h to poprawię, serio nie zauważyłem. Dzięki

Answer 1

• Wg mnie zbyt często odwołujesz się do DOM, aby pobrać element(y). Pobierz sobie wszystkie dostępne na początku aplikacji elementy i w poszczególnych funkcjach operuj na zmiennych. Z tego, co zauważyłem, to w jednym miejscu nawet dwukrotnie pobierasz ten sam element:
  https://github.com/PanLydka/tic-tac-toe-ES5/blob/master/src/js/board/game.js#L90
  https://github.com/PanLydka/tic-tac-toe-ES5/blob/master/src/js/board/game.js#L92
• https://github.com/PanLydka/tic-tac-toe-ES5/blob/master/src/js/board/game.js#L44
  Możesz usunąć/dodać kilka klas jednocześnie: https://developer.mozilla.org/en/docs/Web/API/Element/classList

  remove( String [, String] )

  Remove specified class values

• https://github.com/PanLydka/tic-tac-toe-ES5/blob/master/src/js/board/game.js#L51
  Może Event Delegation? 

Comments

https://github.com/PanLydka/tic-tac-toe-ES5/blob/master/src/js/board/game.js#L3

Pobrałeś elementy, z których potem nie korzystasz za pośrednictwem pola this.fields. Ale na takiej zasadzie możesz właśnie pobrać na początku działania aplikacji wszystkie elementy, których będziesz potrzebować i w funkcjach odnosić się do nich przez this/self.

https://github.com/PanLydka/tic-tac-toe-ES5/blob/master/src/js/main.js#L4

Czemu tak? Robisz coś potem ze zmienną game? Nie wystarczy po prostu new Game()?

Właściwie, to nawet wystarczyło by zapisać tak:

// document.getElementById("start").addEventListener("click", new Game());

Nie przekazujesz tam żadnych parametrów, ani tym bardziej obiektu zdarzenia, więc możesz od razu na kliknięciu odpalić grę.

Answer 2

Zamiast tego spagetti

var lastAction = this.actions[this.currentPlayer][self.actions[self.currentPlayer].length-1];
this.actions[this.currentPlayer].splice((this.actions[this.currentPlayer].length-1), 1);

Proponuję użyć pop

---

Czemu winOptions są stringami?

this.winOptions = [
  ["1", "2", "3"], ["4", "5", "6"], ["7", "8", "9"],
  ["1", "4", "7"], ["2", "5", "8"], ["3", "6", "9"],
  ["1", "5", "9"], ["3", "5", "7"]
];

---

Po co w devDependencies

"devDependencies": {
    "babel-core": "^6.24.1",
    "babel-loader": "^7.0.0",
    "babel-preset-es2015": "^6.24.1",
    "coffeelint": "^1.16.0",
    "cssnano": "^3.10.0",
    "gulp": "^3.9.1",
    "gulp-cssnano": "^2.1.2",
    "gulp-livereload": "^3.8.1",
    "gulp-plumber": "^1.1.0",
    "gulp-sass": "^3.1.0",
    "gulp-webpack": "^1.5.0",
    "sass": "^1.0.0-beta.2",
    "webpack-watch-livereload-plugin": "0.0.1"
  },

• babel-core,
• babel-loader,
• babel-preset-es2015,
• coffelint,
• cssnano,
• sass,
• webpack-watch-livereload-plugin

?

Comments

winOptions są stringami, dlatego że this.actions.x oraz this.actions.o również przechowują stringi, a potem w funkcji status() porównuje te stringi. Nie muszę dodawać dodatkowej operacji polegającej na rzutowaniu typu.

---

A czemu this.actions są stringami?

---

Hmm. Szczerze nie wiem do końca. 

// funkcja fieldPick()

console.log(numberField);
// dostaję wartość typu int 

this.actions[this.currentPlayer].push(numberField);
console.log(this.actions[this.currentPlayer]);
// i dostaję już wartości w stringach :/