Wątek przeniesiony 2018-06-24 06:49 z Edukacja przez Patryk27.

CodeReview - Java - gra w szachy

2

Cześć, nie wiedziałem czy dobry dział wybieram, a zastanawiam się czy ktoś poświęciłby trochę czasu by zrobić dla mnie małe code review. Od niedawna pracuje jako junior a w domu coś tam pisze dodatkowo i staram się jednak zachowywać jakieś dobre praktyki. Cel na początek nie zbyt ambitny - szachy. Póki co silnik gry, obsługujący ruch figur po szachownicy, podłączony pod prosty UI JavyFX (grafiki 50% internet 50% paint tak wiec uwaga xd). W planach zrobienie z tego kompletnej gry, początkowo dalej na desktopie, a potem podział na client-serwer, z początku desktop na socketach, a potem web, czyli zapewne spring.

Dobra, co do kodu - https://gitlab.com/emdzej93/chess-game-core

Szybkie wprowadzenie, od strony GUI - **BoardView **agreguje obiekty Tile, które po kliknięciu wywołują ActionController.executeAction(row, column). Po każdym kliknięciu następuje przerenderowanie ustawienia figur, za wyświetlanie odpowiednich figur odpowiedzialna jest klasa PieceFactory, która na podstawie typu figury zwraca odpowiedni obraz. Wspomniany kontroler jest łącznikiem z warstwą logiki gry, przekazuje stan gry do widoku i decyduje czy dana akcja jest dozwolona z pomocą klasy MoveValidator, klasa ta sprawdza czy ruch danej figury jest dozwolony, czy po drodze do celu figura nie napotka innej figury, czy dany ruch to był atak czy zwykłe przemieszczenie figury. Sama klasa figur czyli Piece i rozszerzające ją klasy są tylko zbiorem reguł na temat ruchów. Klasa Board, agreguje podobnie jak po stronie GUI, obiekty Tile (inny namespace oczywiście). Tym razem te obiekty niosą informację o swojej pozycji, czy są wybrane, czy/jaka figura na nich stoi, itp.

Z grubsza to tyle, mam nadzieje że ktoś znajdzie trochę czasu by zajrzeć do repozytorium i podzielić się uwagami.

edit#
aha, repo zawiera wszystkie potrzebne grafiki, można zaciągnąć i odpalać u siebie bez problemu

4
  • Board jest singletonem? Serio? -,-
  • Po co jest metoda Board.prepareBoard() (Która w sumie powinna się nazywać Board.prepare())? Czemu Board nie może być od początku dobrze zainnicjalizowany bez wołania tej metody?
  • shouldCreateTiles() test który sprawdza losowy element? Testy powinny być deterministyczne.
  • shouldSelectAndUnselectTile() test robi dwie rzeczy. Powinieneś to zesplitować na dwa. Jeśli uznasz że jeden z nich pokrywa się z drugim, to usuń jeden z nich.
  • To samo tu onlyOnePieceShouldBeSelected().
  • W BoardView.java najlepiej jakbyś nie tworzył instancji ActionController w konstruktorze tylko przekazał przez parametr.
  • Tego fora z initBoardView() mógłbyś wynieść do funkcji (którą w sume potem pasowałoby wynieść do innej klasy).
  • Ten array TileView[][] wygląda jakby mógł być wyniesiony z BoardView do innej klasy. Każdy kod który operuje na tym arrayu powinien iść do fukcji w tej nowej klasie.
  • Jesteś pewien że TileView musi być non-static nested klasą? Po co mu referencja do rodzica?
  • W Piece to tworzenie Circle z konstruktora mógłbyś wynieść do metody.
  • Czemu ta metoda Bishop.initLegalMoves() to jest taki setter? Nie można by tego tak zrefactorować żeby tylko zwracała listę, bez modyfikowania stanu? To samo dla innych implementacji Piece.
  • W Piece masz Set<int[]> legalMoves. What? int[] jest legalMovem? Czemu nie zrobisz sobie klasy do tego, zamiast trzymać taki goły array? Ten kod Arrays.equals(legalMoves, new int[] {rowDifference, columnDifference}) mógłbyś wsadzić do tej klasy i porównać sobie row i column jak człowiek a nie takie cyrki.
  • W klasach nadpisujących Piece (te wszystkie King, Bishop, Rook etc.) ja tam widzę wzorzec Bridge Pattern. Dużo duplikacji tam masz. Dziwne to jest trochę że BlackCoś i WhiteCoś to są static nested klasy. Lepiej zrobić dwie hierarchie: osobno figury: Król, Królowa, Goniec, Koń, Wierza; a osobno kolory: Biały, Czarny.
  1. Możesz to zrobić łatwo, mając w Piece metody abstract createWhite() oraz abstract createBlack().
  2. Oczywiście każda implementacja powinna tam zwracać odpowiedni obrazek (zamiast mieć static nested klasy).
  3. Potem w enumie PieceColor zrobić metodę abstract get(Piece piece) i zależnie od wartości enuma zwracałyby odpowiedni piece.
WHITE { get(Piece piece) { return piece.createWhite(); } }
BLACK { get(Piece piece) { return piece.createBlack(); } }
abstract get(Piece piece);

Wtedy cały Twój kod 65-linikowy z PieceFactory wyglądałby tak return piece.getColor().get(piece).

  • Przekazywanie tego filename'a obrazka już lepiej jakby było metodą abstrakcyjną implementowaną przez każdą figurę, a nie parametr konstruktora.
  • MoveValidator to katastrofa.
  1. Po pierwsze bierze instancję singletona, zamiast ją dostać w parametrze konstruktora.
  2. Po drugie wszystkie metody są dużo za długie i mają duuuużo zbyt wiele odpowiedzialności.
  3. W tych dwóch ifach w getMoveType() zawsze sprawdzasz checkCollision() więc mógłbyś to wynieść przed te ify i od razu zwrócić ILLEGAL. Jedna mniej zależnośc w ifach.
  4. Dalej, raz zrobisz if (!isTileOccupied a zaraz potem na dole if (isTileOccupied. To lepiej zrobić tylko w jednym, a niżej zrobić else'a. Kod byłby troszkę czytelniejszy.
  5. MoveValidator ma tylko jedną metodę publiczną getMoveType() która zwraca enuma MoveType. Więc tak na prawdę powinna się nazywać MoveTypeFactory, a metoda getMoveType() powinna być create(). (Factory pattern).
  6. checkCollision() jest do refactora. Tam jest kilka różnych warunków sprawdzanych. Mógłbyś stamtąd wydzielić interfejs ColisionCase, i dodać implementacje CollisionUp, CollisionDown, CollisionUpRight, CollisionKnight itd. Mogłyby zwracać true jak wykryły brak kolizji (koń), false jak wykryły kolizję, albo null jak nic nie wykryły. Miałbyś listę takich case'ów List<CollisionCase> iterować po nich, sprawdzać isCollision() i dopóki jest null to iterować aż nie trafisz na bool'a ,wtedy go zwracasz.
  7. Dużo masz chainowanego kodu board.getSelectedTile().getPiece().getLegalMoves(). Może figury zamiast zwracać legalne ruchy, mogłyby dostawać ruch i go walidować? Pomyśl, przeanalizuj. Może to co jest teraz jest ok, może warto zmienić? Wtedy każda Piece miałaby metodę abstrakcyjną isMoveValid() i każda osobno by sobie to inaczej implementowała. Albo też możesz uznać że już nie warto zmieniać. (Pro tip, zanim zaczniesz zmieniać najpierw napisz testy pod te ruchy).
  8. Metoda MoveValidator.isTileOccupied() jest w ogóle nie potrzebna. Nie ma swoich odpowiedzialności, jedyne co robi to deleguje wywołanie. Moim zdaniem do wywalenia.
  • Zrób żeby Board nie był singletonem do c****.
  • Tile[][], to samo co z TileView[][], wygląda jakby trzeba by to wynieść do innej klasy, np Tiles. I znowu, każdy kod z Board który na nich operuje powinien być wsadzony do tej klasy.
  • Pro Tip 2. Jak będziesz miał tą klasę która ma w sobie Tile[][] to mógłbyś dodać do niej jakiś iterator, albo metodę forEach() tak żebyś mógł robić tiles.forEach(tile -> { (albo tiles.forEach((x,y) -> {) zamiast pisać wszędzie 2 fory.
  • Trzymaj konstruktory Board na samej górze klasy.
  • Board.createPieces() trza by wynieść do innej klasy. Jeśli uważasz że musisz oddzielić wywołania metod setPiece() komentarzami // Knights, // Rooks etc. to to dobry sygnał żeby je wydzielić do metod setKnights(), setRooks() etc.
  • Tile.isOccupied() Serio? Wystarczy return piece != null, a nie if.
  • ActionController.executeAction() jest switch po różnych wartościach enuma. Wtedy się w tym enumie robi metodę abstrakcyjną (np execute()) i wtedy dla odpowiednich wartości implementuje się je inaczej.
  • Te stringi "illegal move", "neutral move", etc. też mógłbyś wsadzić do metody abstrakcyjnej MoveType.getName() i implementować osobno dla każdej wartości enuma.
  • Ogólnie zapomnij że jest coś takiego jak switch. Zawsze to można zamienić na enuma albo polimorfizm.
  • Zazwyczaj jak masz coś takiego board.getSelectedTile().getPiece() instanceof Pawn to znaczy że masz źle zaprojektowany interfejs. Chcesz wykonać na interfejsie Piece jakąś metodę, tylko wtedy jeśli jej implementacją jest Pawn więc to sprawdzasz. Zamiast tego, lepiej, żeby reinitLegalMoves() było metodą abstrakcyjną, która jest zaimplementowana w Pawn, a wszystkie inne implementacje (Knight, King, etc) mają pustą tą metodę. Wtedy nie musisz robić instanceofa. Jeśli bardzo chcesz to możesz tą pustą metodę wsadzić do Piece i nie robić jej abstrakcyjnej, tylko w Pawn ją override'ować.
  • W BoardView.renderPieces() masz iterowanie po tiles. Moim zdaniem wszystko co jest w tych forach, w tym kodzie powinno być wydzielone do metody, do której przekazyawny byłby tileView (ten z tiles[i][j]) oraz odpowiadający tile (ten z controller.getTiles()[i][j]).
  • W TileView.equals() nie masz sprawdzania czy object nie jest nullem oraz czy nie są tą samą instancją - operator ==. (To drugie opcjonalnie).
  • Wszystko co jest w PieceFactory.getPiece() powinno być wyniesione. Masz tam miliard instanceofów, a to znaczy że wszystkie te wywołania można by zastąpić jednym wywołaniem do metody abstrakcyjnej w com.company.chess.core.pieces.Piece.getGuiPiece(PieceColor c) (nazwę sam wymyśl). Jak już to zrobisz to PieceFactory tak na prawdę jest do wywalenia.
  • TileView - serio?
if(coś) {
    return true;
}
return false;

serio?

2

Bardziej zaawansowane

 private void movePiece(int newRow, int newColumn){
        int oldRow = board.getSelectedTile().getRow();
        int oldColumn = board.getSelectedTile().getColumn();
        if(board.getSelectedTile().getPiece() instanceof Pawn){
            ((Pawn) board.getSelectedTile().getPiece()).reinitLegalMoves();
        }
        board.getTile(newRow, newColumn).setPiece(board.getSelectedTile().getPiece());
        board.getTile(oldRow, oldColumn).setPiece(null);
    }
  • Tutaj robisz board.getSelectedTile() (czyli masz Tilea), bierzesz jego row i column, a potem 3 linijki niżej, zamiast użyć tego Tile którego wziąłeś z getSelectedTile() jak człowiek, to jeszcze raz robisz board.getTile(oldRow, oldColumn) po tych row i column które przed chwilą z niego wyciągnąłeś.

  • Poza tym, te dwie ostatnie linijki wyglądają jakby je można było przenieść do metody Board.move() która przenosiłaby aktualnie wybraną figurę z getSelectedTile() na tą tile z newRow i newColumn.

Ja bym ją napisał tak

 private void movePiece(int newRow, int newColumn){
        Tile selected = board.getSelectedTile();
        Piece piece = selected.getPiece();

        piece.reinitLegalMoves();
        board.getTile(newRow, newColumn).setPiece(piece);  // BTW to trochę słabe że najpierw stawiasz pionek w nowym miejscu,
        selected.setPiece(null);                           // a potem zabierasz ze starego. To znaczy że jest moment w grze w której jeden pionek jest w dwóch miejscach. Powinieneś najpierw zabrać, a potem postawić.
    }

a potem tak:

 private void movePiece(int newRow, int newColumn){
        board.getSelectedTile().getPiece().reinitLegalMoves();
        board.moveSelectedPieceTo(newRow, newColumn);
    }
2

Zaawansowane part dwa:

board.getTile(oldRow, oldColumn).getPiece().getColor() == board.getTile(newRow, newColumn).getPiece().getColor()

Chodzi o to że czemu ActionController ma znasz szczegóły sprawdzania czy ruch jest attacking czy nie. To powinno być wydzielone gdzieś indziej. Ja bym do Tile dodał metodę isAttackingMove(Tile), i zamieniłbym powyższy kod na taki:

board.getTile(oldRow, oldColumn).isAttacking(board.getTile(newRow, newColumn))

Dodatkowa rada, zrób sobie klasę Move czy coś która miałaby w sobie row oraz column.. Wychodzi że one są ze sobą koncepcyjnie związane, bo wszędzie jest przekazujesz razem, więc czemu by jej nie wsadzić do klasy Move? Wtedy ten kod mógłby wyglądać tak

board.getTile(oldMove).isAttacking(board.getTile(newMove))

i to isAttacking() jeszcze ewentualnie możnaby schować do Boarda (kwestia gustu):

board.isAttacking(oldMove, newMove)

1 użytkowników online, w tym zalogowanych: 0, gości: 1