Feedback & Code Review od Mentora Recenzenta (Szymon Pakuła) #94

MateuszNaKodach · 2021-02-16T17:44:17Z

No description provided.

Add a GiHub Actions workflow to the project. Every time someone wants to merge commit into develop it will try to build application, run tests and upload new files on GitHub Pages. Build and test checks must pass to be able to merge.

…2020.Project.TypeScript.Chess into develop

* more project settings * fixes to settings * add ESLint * additional checks for ts code

…2020.Project.TypeScript.Chess into develop

This reverts commit aacc1bb.

Co-authored-by: lamparina <anna.lamperska@gmail.com>

with some interfaces, types and constances

…2020.Project.TypeScript.Chess into develop

…rkflow

…4 and there is no pieces on Pawn's way"

…to_account_the_King's_check #43_Moves_a_piece_taking_into_account_the_King's_check

Build on windows

* Imports fix * Delete unused code line * Fix merge Co-authored-by: PiotrWR <pwrynio@gmail.com> Co-authored-by: Piotr Witold Rynio <41823689+PiotrWR@users.noreply.github.com> Co-authored-by: Mateusz Nowak <kontakt.mateusznowak@gmail.com>

…abled (#80) * Add test * Change turn to avoid error * Add pawnPromotionWasEnabled event * Fix tests * Block move until promotion is done * Restore tests for check | Merge changes and resolve conflicts * Update lock file * Fix bug * Add missing test case Co-authored-by: lamparina <anna.lamperska@gmail.com>

* Initiall commit * Initiall commit * Add types * Fix name of pieceMovesNotCausingAllyKingCheck function * Add first test * Add second test * Finishtest * Add CheckmatedKing variable and isCheckmatedKing function * Delete last added function: isCheckmatedKing * After code review | Update tests * Add empty method in ChessEngine. Add code in move() method * Fix last commit * Add new test * Delete doubled test * Fix bug in test * Finish checkmateHasOccurred function, In progress: is PossibleMoves * Finish all without cleaning code. * EsLint CoderReview | Update * Add tests for Stalemate Has Occurred * Add type for Stalemate Has Occurred * Fix | Fix test for Stalemate Has Occurred * Fix | Fix test from others tasks because this tests had bug with stalemate event * Finish stalemate event service * Eslint CodeReview | Update

* Add positive and negative test cases for castling * CastlingWasDone - positive cases | Castling not possible while king's exposed to attack * Disable castling if king or rook has already moved or there is any piece between them * Refactor ChessEngine * Return all possible moves from one method * Rename method * Fix method name * Refactor possibleMoves method Co-authored-by: lamparina <anna.lamperska@gmail.com>

@nowakprojects

Z podziękowaniem dla @nowakprojects @lamparina @tomdworniczak @Szambelan za ładne CodeReview poprzedniego taska * Update | Change Stalemate and fix a small bug in test * Update | Change type of Stalemate event * Update | Delete imports * Update | Delete kingPosition problem

* Create modal window for pawn promotion * Selected figure to be promoted * Proba skonczenia zadanka * Rename modal's css classes * Promotion modal and it's test * Changes in modal event * #76 modal window for pawn promotion - Model * #76 make promotingOnSquare private again * #76 remove dead code * #76 CR fixes Co-authored-by: Pawel Szambelan <p.szambelan1@gmail.com> Co-authored-by: lamparina <59768305+lamparina@users.noreply.github.com> Co-authored-by: lamparina <anna.lamperska@gmail.com>

* First part * Add bg * next part * next part * next part * next part

* #70 Tests and draft solution * #70 Refactor test case * #70 Apply pawn attack in passing logic * #70 Remove silly test case * #70 Add return type to method * #70 Add test case * #70 delete unneeded if statement * #70 refactor * #70 merge develop * Add en passant to possibleMoves * Last fix Co-authored-by: lamparina <anna.lamperska@gmail.com> Co-authored-by: lamparina <59768305+lamparina@users.noreply.github.com>

Co-authored-by: lamparina <anna.lamperska@gmail.com>

Nefariusek · 2021-02-19T11:12:09Z

index.html

+  <head>
+    <meta charset="UTF-8" />
+    <meta name="viewport" content="width=device-width, initial-scale=1.0" />
+    <title>....Our title</title>


bugs_bunny_our_meme.jpg 🤣

Nefariusek · 2021-02-19T11:20:50Z

src/app/presenter/ChessBoardPresenter.ts

+      // case 'PawnWasPromoted':
+      //   this.view.afterPromotionPiece(
+      //     this.translateSquareToAlgebraicNotation(event.onSquare),
+      //     event.chosenPiece.name.toLowerCase(),
+      //     event.chosenPiece.side,
+      //   );
+      //   break;


Warto usuwać zakomentowany kod. Najprawdopodobniej i tak osoba, która będzie rozwijała tę funkcjonalność napisze wszystko od nowa zamiast to odkomentowywać.
Dodatkowo jeśli ktoś będzie potrzebował tego, to z łatwością można do tego wrócić dzięki VCS.

Nefariusek · 2021-02-19T11:25:08Z

src/app/view/GameCommentsView.ts

@@ -0,0 +1 @@
+export interface GameCommentsView {}


Myślę że, tak jak w przypadku zakomentowanego kody, warto usuwać kod, który nie jest nigdzie używany.

Nefariusek · 2021-02-19T11:45:02Z

test/app/model/ChessEngine.spec.ts

+import { Bishop, Chessboard, ChessEngine, King, Knight, Pawn, Queen, Rook, Side, Square, SquareWithPiece } from '../../../src/app/model';
+import 'jest-extended';


sortowanie importowanych zależności w kolejności

"zewnętrzne" - tak jak 'jest-extended'

"wewnętrzne" - w tym przypadku Figury
pozwoli poprawić czytelność i będzie pomocne w przypadku znacznie większych projektów

Nefariusek · 2021-02-19T11:52:54Z

src/app/view/web/WebChessView.ts

+    const queenPromotion = document.createElement('div');
+    queenPromotion.classList.add('modal__pawn');
+    queenPromotion.textContent = 'Queen';
+    modalPawnWrap.appendChild(queenPromotion);
+
+    const rookPromotion = document.createElement('div');
+    rookPromotion.classList.add('modal__pawn');
+    rookPromotion.textContent = 'Rook';
+    modalPawnWrap.appendChild(rookPromotion);
+
+    const knightPromotion = document.createElement('div');
+    knightPromotion.classList.add('modal__pawn');
+    knightPromotion.textContent = 'Knight';
+    modalPawnWrap.appendChild(knightPromotion);
+
+    const bishopPromotion = document.createElement('div');
+    bishopPromotion.classList.add('modal__pawn');
+    bishopPromotion.textContent = 'Bishop';
+    modalPawnWrap.appendChild(bishopPromotion);


rozważyłbym utworzenie funkcji pomocniczej aby uniknąć duplikowania pewnych części kodu

Nefariusek · 2021-02-19T11:56:28Z

src/app/view/web/WebChessView.ts

+    const pieceImage = divFrom?.firstChild as HTMLImageElement;
+
+    if (pieceImage) {
+      pieceImage.src = `static/img/pieces/${side.toLowerCase()}-${piece.toLowerCase()}.svg`;


Dobrym pomysłem mogłoby być przechowywanie początka ścieżki (static/img/pieces/) w zmiennej globalnej, na wypadek zmian w strukturze katalogów.
W tym przypadku występuje to także w 83 linijce

Nefariusek · 2021-02-19T11:59:46Z

src/app/view/Button.ts

+      buttonDomElement.classList.remove('button--large');
+      buttonDomElement.classList.add('button--small');
+    }
+    if (this.size == Size.LARGE) {
+      buttonDomElement.classList.remove('button--small');
+      buttonDomElement.classList.add('button--large');


Tutaj także zastosować enuma, tak jak w przypadku Size.

Nefariusek

Organizacja kodu, zastosowanie programowania obiektowego i możliwości jakie daje TypeScript na bardzo wysokim poziomie. Implementacja jest bardzo przejrzysta.
Pochwalić należy także wysokie pokrycie kodu testami.

Podsumowując - super projekt.

annalamperska · 2021-02-19T13:08:46Z

Dzięki za review! Chciałam tylko dodać, że już wszystko działa :) Promocja, bicie w przelocie i takie tam ;)

* Update README.md Update README file * Update readme again * Add live version link and sreenshoots of a game Co-authored-by: lamparina <59768305+lamparina@users.noreply.github.com>

MateuszNaKodach and others added 30 commits January 19, 2021 09:40

Update README.md

e5f0867

initial commit

9ae96cd

more project settings

949b9c0

Add project settings (ESLint, tsconfig, ts-jest, prettier)

8c38735

Init Model-View-Presenter structure

df33b5d

Init Model-View-Presenter structure

b12f4d9

Create node.js.yml

8b5da92

Merge branch 'develop' of https://github.com/nowakprojects/CodersCamp…

e031aca

…2020.Project.TypeScript.Chess into develop

Additional checks settings (#15)

aacc1bb

* more project settings * fixes to settings * add ESLint * additional checks for ts code

Merge branch 'develop' of https://github.com/nowakprojects/CodersCamp…

1986243

…2020.Project.TypeScript.Chess into develop

Revert "Additional checks settings (#15)" (#19)

2943c04

This reverts commit aacc1bb.

#5 Stworzenie obrazków figur (#18)

e58d121

Co-authored-by: lamparina <anna.lamperska@gmail.com>

Fix | Add missing export / imports in view and tests

1983190

Feature/#17 possible moves for rock (#20)

d31d992

with some interfaces, types and constances

Add first test

e6b9950

Add more tests and added BoardFixture.ts

ca0b003

Merge branch 'develop' of https://github.com/nowakprojects/CodersCamp…

f552a09

…2020.Project.TypeScript.Chess into develop

Add chess Board fixtures and improve tests readability

18654d8

Add non-finished Pawn.ts

a228e4d

Add all tests

5f60ff3

Add "possibleMoves" functions

11dcdd8

ESLint configuration fix and add lint & prettier to GitHub Actions wo…

a9bb410

…rkflow

Fix movesToGo

96ee0da

Fix first test "Check possible squares to go, when White Pawn is on D…

77cf28a

…4 and there is no pieces on Pawn's way"

Add "goAhead" function for WHITE side

a9cbe9c

Add "goDoubleAhead" function for WHITE side

a455441

Add moving Pawn in diagonal ahead line for WHITE Pawns

2e2b0e2

CodeCov | Add test coverage reports and badge in README.md

7e7709b

Fix tests for BLACK side

30d9a33

PiotrRynio and others added 19 commits February 13, 2021 17:04

Merge pull request #68 from nowakprojects/#43_Moves_a_piece_taking_in…

785a17c

…to_account_the_King's_check #43_Moves_a_piece_taking_into_account_the_King's_check

Merge branch 'develop' into build_on_windows

e9e9f50

Merge pull request #75 from nowakprojects/build_on_windows

b49b5b6

Build on windows

Model/remove player (#78)

965bb07

Imports fix (#74) (#81)

7eede96

* Imports fix * Delete unused code line * Fix merge Co-authored-by: PiotrWR <pwrynio@gmail.com> Co-authored-by: Piotr Witold Rynio <41823689+PiotrWR@users.noreply.github.com> Co-authored-by: Mateusz Nowak <kontakt.mateusznowak@gmail.com>

Chess engine king was checked king was unchecked (#82)

4f5cf93

Fix describes in branch nr 42 (#86)

61056c1

View/#62 piece movement (#77)

4360d89

Update css | quick fix before deadline (#92)

8b01cd3

* First part * Add bg * next part * next part * next part * next part

Fix imgs

da8ae01

Add same row checking (#97)

ccad37d

Co-authored-by: lamparina <anna.lamperska@gmail.com>

Fix img problems after promotion (#96)

e002ab6

Disable castling when a pawn attacks square crossed by the king (#100)

1d981c3

Co-authored-by: lamparina <anna.lamperska@gmail.com>

Nefariusek reviewed Feb 19, 2021

View reviewed changes

DomiZet and others added 2 commits February 21, 2021 17:34

Update README.md (#99)

0d6ee0e

* Update README.md Update README file * Update readme again * Add live version link and sreenshoots of a game Co-authored-by: lamparina <59768305+lamparina@users.noreply.github.com>

Add codecov bagde to README

a6451c6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Feedback & Code Review od Mentora Recenzenta (Szymon Pakuła) #94

Feedback & Code Review od Mentora Recenzenta (Szymon Pakuła) #94

Uh oh!

MateuszNaKodach commented Feb 16, 2021

Uh oh!

Nefariusek Feb 19, 2021

Uh oh!

Nefariusek Feb 19, 2021

Uh oh!

Nefariusek Feb 19, 2021

Uh oh!

Nefariusek Feb 19, 2021

Uh oh!

Nefariusek Feb 19, 2021

Uh oh!

Nefariusek Feb 19, 2021

Uh oh!

Nefariusek Feb 19, 2021

Uh oh!

Nefariusek left a comment

Uh oh!

annalamperska commented Feb 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

		import { Bishop, Chessboard, ChessEngine, King, Knight, Pawn, Queen, Rook, Side, Square, SquareWithPiece } from '../../../src/app/model';
		import 'jest-extended';

Feedback & Code Review od Mentora Recenzenta (Szymon Pakuła) #94

Are you sure you want to change the base?

Feedback & Code Review od Mentora Recenzenta (Szymon Pakuła) #94

Uh oh!

Conversation

MateuszNaKodach commented Feb 16, 2021

Uh oh!

Nefariusek Feb 19, 2021

Choose a reason for hiding this comment

Uh oh!

Nefariusek Feb 19, 2021

Choose a reason for hiding this comment

Uh oh!

Nefariusek Feb 19, 2021

Choose a reason for hiding this comment

Uh oh!

Nefariusek Feb 19, 2021

Choose a reason for hiding this comment

Uh oh!

Nefariusek Feb 19, 2021

Choose a reason for hiding this comment

Uh oh!

Nefariusek Feb 19, 2021

Choose a reason for hiding this comment

Uh oh!

Nefariusek Feb 19, 2021

Choose a reason for hiding this comment

Uh oh!

Nefariusek left a comment

Choose a reason for hiding this comment

Uh oh!

annalamperska commented Feb 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants