Ocena Kółka i Krzyżyk - pierwszy program

0

Cześć, w końcu mój pierwszy "większy" program działa poprawnie. Chciałbym, żebyście powiedzieli co ewentualnie zmienić, poprawić, licze na konstruktywną krytykę.

Łapcie link do githuba: https://github.com/must1/TicTacToe

1

Klasa PlayerChanger.java:

  • Po co Ci getter skoro zmienna jest public? (Zapewne miało być private, bo teraz mogę ją ręcznie zmienić w kodzie)

Klasa BoardPrinter.java:

  • Jeśli masz tylko jedną metodę w klasie to zrób ją statyczną

Klasa ConditionsChecker.java:

        if (board[0][0] != '_' && board[0][0] == board[0][1] && board[0][0] == board[0][2])
            return false;
        if (board[1][0] != '_' && board[1][0] == board[1][1] && board[1][0] == board[1][2])
            return false;
        if (board[2][0] != '_' && board[2][0] == board[2][1] && board[2][0] == board[2][2])
            return false;
        if (board[0][0] != '_' && board[0][0] == board[1][0] && board[0][0] == board[2][0])
            return false;
        if (board[0][1] != '_' && board[0][1] == board[1][1] && board[0][1] == board[2][1])
            return false;
        if (board[0][2] != '_' && board[0][2] == board[1][2] && board[0][2] == board[2][2])
            return false;
        if (board[0][0] != '_' && board[0][0] == board[1][1] && board[0][0] == board[2][2])
            return false;
        if (board[0][2] != '_' && board[0][2] == board[1][1] && board[0][2] == board[2][0])
	    return false;

To aż się prosi o pętlę ;)

  • Swoją drogą nazwa tej metody checkIfWinner wprowadza w błąd - jeżeli ktoś wygrał zwraca false.
  • row > 3 || col > 3 -> co jeśli wprowadzę poprawny input mniejszy od zera?

Klasa Game.java:

  • Tu przestaję sprawdzać, poszukaj sobie jak zanegować wynik zwracający wartość logiczną
0
Burdzi0 napisał(a):

Klasa Game.java:

  • Tu przestaję sprawdzać, poszukaj sobie jak zanegować wynik zwracający wartość logiczną

Możesz napisać coś więcej? Niezbyt rozumiem.

Burdzi0 napisał(a):

Swoją drogą nazwa tej metody checkIfWinner wprowadza w błąd - jeżeli ktoś wygrał zwraca false.

Zaraz zrobię to wszystko na odwrót :P

Burdzi0 napisał(a):

Po co Ci getter skoro zmienna jest public? (Zapewne miało być private, bo teraz mogę ją ręcznie zmienić w kodzie)

Wkradł się błąd :D

Burdzi0 napisał(a):

Jeśli masz tylko jedną metodę w klasie to zrób ją statyczną

Możesz wytłumaczyć dlaczego tak się powinno robić?

Jeżeli chodzi o warunki wygranej, to postaram się nad tym pokombinować, wiem że aż oczy łzawią jak to widać :P

2

Co do pierwszego - koledze chyba chodzi o ten kawałek kodu:

if (conditionsChecker.checkIfOutOfBoard(row, col)) {

}
else {
    if (conditionsChecker.checkIfOccupied(row, col, board)) {

    }
    else {
        board[row - 1][col - 1] = playerChanger.getTurn();
        map.printBoard(board);
        playerChanger.changePlayer();
    }
}

Wychodzi na to że używasz if tylko dlatego że nie wiesz jak zanegować zwracaną wartość.

1

Spotkałem się z taką opinią, że kiedy klasa ma jedną metodę to owa metoda powinna być statyczna i w tym wypadku jak najbardziej to pasuje.
Zobacz, że tworzenie obiektu zupełnie do niczego nie jest Ci potrzebne, tj. nigdzie go nie przekazujesz, nie wykonujesz na nim żadnych działań.
Przykład takiej opinii jest tutaj (rzuć też okiem na odpowiedź niżej).
Jak poprawisz to, co znaleźliśmy to daj znać - rzucimy okiem ponownie i tym razem przyglądnę się bliżej.
I tak na koniec zadbaj o formatowanie - wcięcia, entery te sprawy, żeby kod czytało się lepiej (szybciej)

0
Burdzi0 napisał(a):

Klasa PlayerChanger.java:

  • Po co Ci getter skoro zmienna jest public? (Zapewne miało być private, bo teraz mogę ją ręcznie zmienić w kodzie)

Usunąłem całkowicie tę metodę.

Klasa BoardPrinter.java:

  • Jeśli masz tylko jedną metodę w klasie to zrób ją statyczną

Tylko tego nie zrobiłem., Musze jeszcze o tym poczytać o modyfikatorach dostępu, bo na ten moment to wiem, że powinno się dawać private i tyle :D Tak to rzucam sobie publicami na prawo i lewo :P To jest i tak akurat najmniejszy problem ze wszystkich chyba :P.

Klasa ConditionsChecker.java:

        if (board[0][0] != '_' && board[0][0] == board[0][1] && board[0][0] == board[0][2])
            return false;
        if (board[1][0] != '_' && board[1][0] == board[1][1] && board[1][0] == board[1][2])
            return false;
        if (board[2][0] != '_' && board[2][0] == board[2][1] && board[2][0] == board[2][2])
            return false;
        if (board[0][0] != '_' && board[0][0] == board[1][0] && board[0][0] == board[2][0])
            return false;
        if (board[0][1] != '_' && board[0][1] == board[1][1] && board[0][1] == board[2][1])
            return false;
        if (board[0][2] != '_' && board[0][2] == board[1][2] && board[0][2] == board[2][2])
            return false;
        if (board[0][0] != '_' && board[0][0] == board[1][1] && board[0][0] == board[2][2])
            return false;
        if (board[0][2] != '_' && board[0][2] == board[1][1] && board[0][2] == board[2][0])
	    return false;

To aż się prosi o pętlę ;)

  • Swoją drogą nazwa tej metody checkIfWinner wprowadza w błąd - jeżeli ktoś wygrał zwraca false.
  • row > 3 || col > 3 -> co jeśli wprowadzę poprawny input mniejszy od zera?

Dodany warunek poniżej 0.
Zmieniłem to co zwraca metoda.
Dodałem pętlę.

Klasa Game.java:

  • Tu przestaję sprawdzać, poszukaj sobie jak zanegować wynik zwracający wartość logiczną.

Zanegowałem tak, żeby klasa **checkIfWinner **się zgadzała.

Jeżeli chodzi o formatowanie to dodałem kilka enterów.

Wiem, że powinienem dodać jeszcze warunek gdyby pojawił się remis. Ale to przy następnej kolejności zrobię.

1

Klasa PlayerChanger:

Usunąłem całkowicie tę metodę.

Jak wolisz, ja na Twoim miejscu zmieniłbym na private skoro i tak już wyodrębniasz jako zmienną do innej klasy. Popatrz, że mogę zrobić w klasie Game zrobić

playerChanger.turn = 'F';

i karuzela kręci się dalej ;) Jak już to wyodrębniłeś to rób to z głową :P
Co ciekawe to jak stosujesz otwarcie i zamknięcie klamer jest niespójne (czasami po enterze, czasami po spacji).

Klasa BoardPrinter:

Musze jeszcze o tym poczytać o modyfikatorach dostępu, bo na ten moment to wiem, że powinno się dawać private i tyle

To nie ma nic do rzeczy. Modyfikatory dostępu a static/non-static to dwa odrębne tematy.

Klasa Game:

row = input.nextInt();
col = input.nextInt();

a wpisz sobie tutaj "Ala ma kota" ;)
I kawałek kodu, którego bardzo nie lubię

 if (conditionsChecker.checkIfOutOfBoard(row, col)) {

            } else {

                if (conditionsChecker.checkIfOccupied(row, col, board)) {

                }
                else {

                    board[row - 1][col - 1] = playerChanger.turn;
                    map.printBoard(board);
                    playerChanger.changePlayer();

                    }
}

Jeżeli nie jest spełniony pierwszy warunek i nie jest spełniony drugi warunek wykonaj ten blok kodu:

board[row - 1][col - 1] = playerChanger.turn;
map.printBoard(board);
playerChanger.changePlayer();

Odsyłam do pierwszego prawa de Morgana - zobacz jak to można uprościć ;)
A tak na serio to nie musisz z niego korzystać - podaję jako ciekawostkę - tylko spróbuj to uprościć bo przez te puste bloki kodu mam przeciąg w mieszkaniu :P
Na razie tyle, nie mam siły na myślenie :P

0

W końcu mam wolne na uczelni, tak ze mogłem ponownie do tego przysiąść :)

  • pozbyłem się tych pustych bloków
  • w PlayerChanger zmienna turn jest prywatna
  • dodałem try/catch, tak że teraz już tylko można wpisywać co się chce, a działać będzie dobrze (przynajmniej tak mi się wydaje :P)

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