Dzięki za udostępnienie kodu.

To, co mnie trochę denerwuje tym kodem, to to, że nie używam interfejsów, klas abstrakcyjnych ani dziedziczenia,

Wykonywanie OOP oznacza, że przestrzegasz pewnych zasad, którymi są (między innymi):

• ukrywanie / hermetyzacja informacji
• pojedyncza odpowiedzialność
• oddzielenie obaw
• KISS (zachowaj prostotę (i) głupotę.)
• SUCHY (nie powtarzaj się)
• „Powiedz! Nie pytaj”.
• Prawo demeter („Nie rozmawiaj z nieznajomymi!”)

Interfejsy, abs klasy traktatu lub dziedziczenie obsługują zasady kapelusza i powinny być używane w razie potrzeby. Nie „definiują” OOP.

IMHO Głównym powodem niepowodzenia twojego podejścia OOP jest to, że twój „Model” jest tablicą typu pierwotnego char. Ostatecznie prowadzi to do proceduralnego podejścia do logiki gry.

Pomyślałbym o takim interfejsie:

interface GameField{ char getIcon(); Result shootAt(); } 

gdzie Result byłoby wyliczeniem:

 enum Result{ NO_HIT, PARTIAL_HIT, DESTROYED } 

I miałbym różne implementacje interfejsu:

public class BorderField implements GameField{ private final char borderName; public BorderField(char borderName){ this.borderName = borderName; } @Override public char getIcon(){ return borderName; } @Override public Result shootAt(){ return Result.NO_HIT; } } 

public class WaterField implements GameField{ private boolean isThisFieldHit = false; @Override public char getIcon(){ return isThisFieldHit?"M": " "; } @Override public Result shootAt(){ return Result.NO_HIT; } } 

public class ShipField implements GameField{ private final Ship ship; private boolean isThisFieldHit = false; public ShipField(Ship ship){ this.ship = ship; } @Override public char getIcon(){ Result shipState = ship.getState(); switch(shipState){ case NO_HIT: return " "; case PARTIAL_HIT: return isThisFieldHit?"O":" "; case DESTROYED: return "#"; } @Override public Result shootAt(){ ship.hit(); return ship.getState(); } } 

To powinno wystarczyć, mam nadzieję, że wpadnij na pomysł …

Kwestie formalne

Nazewnictwo

Znalezienie dobrych nazw jest najtrudniejszą częścią programowania. Dlatego zawsze nie spiesz się, aby pomyśleć o nazwach identyfikatorów.

Z drugiej strony przestrzegasz konwencji nazewnictwa Java.

Jednak nazwy metod powinny zaczynać się od czasownika w jego czas teraźniejszy.Eg: shipWasHit() powinno nazywać się hit().
Lub distanceBetweenPoints() powinno być calculateDistanceBetween(). Tutaj parametry pokazują, że odległość jest między punktami, więc nie ma potrzeby umieszczania tego w nazwie metody.

Bądź rozwlekły w nazwach zmiennych. zamiast

 double x1 = from.getX(); double y1 = from.getY(); double x2 = to.getX(); double y2 = to.getY(); 

te zmienne powinny być raczej nazwane w ten sposób:

 double startPointX = from.getX(); double startPointY = from.getY(); double endPointX = to.getX(); double endPointY = to.getY(); 

Weź swoje nazwy z domeny powodującej problem, a nie z rozwiązania technicznego. np .: SHIP_ICON powinno być tylko SHIP, chyba że masz inną stałą w klasie Ship .

Komentarze

Komentarze powinny wyjaśniać, dlaczego kod wygląda tak, jak jest . Usuń wszystkie inne komentarze.

komentarze powinny być używane tylko w interfejsach lub metodach abstrakcyjnych, w których zawierają kontrakt , który musi spełnić implementator.

Klasa stałych

Połącz rzeczy, które do siebie pasują. Zdefiniuj stałe w klasie, która ich używa.

Komentarze

• Wszystkie ważne punkty i bardzo pomocne. Dziękuję Ci!

Jest już kilka dobrych odpowiedzi, ale pomyślałem, że dodam kilka z nich do mnie, kiedy przeglądałem kod.

W tej chwili jedynym źródłem danych wejściowych jest dane wejściowe użytkownika ze skanera. Byłoby to dość trudne, gdybyś chciał dodać jakichś przeciwników komputerowych do graj przeciwko.

Wygląda na to, że w klasie Board jest kod, który mógłby lepiej pasować do klasy Player.

W szczególności zawartość metody placeShipsOnBoard ().

Ta metoda pobiera dane wejściowe od użytkownika i tworzy Pozycję. Spróbujmy to zmienić tak, aby Gracz (człowiek lub komputer) mógł stworzyć Pozycję.

Zróbmy interfejs

public interface Player { Position placeNextShip(); void fireAt(Position target); } 

Mamy już implementacje od Człowieka,

public class HumanPlayer implements Player { // variables @Override public Position placeNextShip(){ // uses Scanner instance to create a Position } @Override public void fireAt(Position target){ // code from your attack method } } 

A co z podstawowy odtwarzacz komputerowy

public class DumbComputer implements Player { @Override public Position placeNextShip(){ // keep choosing random locations } @Override public void fireAt(Position target){ // keep firing at random positions } } 

Następnie w głównej klasie Board programujemy interfejs odtwarzacza

while(!isShipPlacementLegal){ for(Player player : players){ // some list of players in the game // either scanner input or randomly generated Position Position shipPlacement = player.placeNextShip(); boolean validPosition = validatePos(shipPlacement); if(validPostion){ // good to go! Place ship and continue to next player } else { // prompt again, whatever else you need to do here. } } } 

Jeśli cały kod odnosi się do odtwarzacza (obecnie interfejsu), a nie do konkretnej implementacji, możemy łatwo dodać nowe typy odtwarzaczy komputerowych . na przykład new CheatingComputer (), new HardComputer (), new MediumComputer (). Każdy z nich w inny sposób określałby, gdzie wystrzelić następny i gdzie umieścić następny statek.

A gdybyśmy to mieli, moglibyśmy stworzyć grę z 2 komputerowymi graczami i pozwolić jej grać samemu! Ekscytujące prawda: D

Inną powiązaną rzeczą, którą moglibyśmy zmienić, jest to, że w konstruktorze gry zawsze będziesz mieć 2 graczy ludzi. Moglibyśmy zrobić listę graczy, abyś mógł mieć dowolną liczbę graczy w swojej grze. To, że w prawdziwej grze Battleship jest ograniczone do 2 graczy, nie oznacza, że twój musi być.

Moglibyśmy pozwolić na regulację rozmiaru siatki i dowolną liczbę graczy. Nagle mamy siatkę 100×100 z 8 darmowy gracz dla wszystkich!

(dowolna ilość może być kontrolowana przez komputer).

Twoje statki są również inicjalizowane w statycznym bloku w twojej klasie pokładowej. Masz wszystkie prawdziwe statki z Battleship. Ale znowu, dlaczego nie pozwolić tutaj na większą elastyczność. Co by było, gdyby twój statek składał się z listy punktów? Możesz mieć statki w kształcie litery S, nie musiałyby ograniczać się do ustawienia poziomego lub pionowego. (To może być przesadne, ale myślę, że to fajna rzecz do przemyślenia!)

Skończę kilkoma drobnymi rzeczami, które dla mnie wyglądały śmiesznie.

throw new ArrayIndexOutOfBoundsException(); 

z klasy Position. Wydaje się, że tutaj wyjątek jest nieodpowiedni. W klasie Position nie ma tablicy, więc musi to odnosić się do tablicy w klasie Board. Myślę, że bardziej odpowiednim typem Exception byłby IllegalArgumentException, ArrayIndexOutofBoundsException jest szczegółem implementacji (innej klasy!). Powinieneś pamiętaj również o podaniu odpowiedniego komunikatu o błędzie wraz z wyrzuceniem wyjątku, np. „wartość musi mieścić się w zakresie xiy”

Linia

boolean isShipHit = (ship != null) ? true : false; 

można po prostu zastąpić przez

boolean isShipHit = ship != null; 

Nie ma tu potrzeby stosowania operatora trójskładnikowego.

Użycie targetHistory w klasie Player while (targetHistory.get (point)! = null)

Tutaj „używasz mapy wyłącznie w celu sprawdzenia, czy element w niej jest. To jest dokładnie to, czym jest Set za!

targetHistory = new HashSet<>(); while(targetHistory.contains(point)){ // re-prompt } 

Komentarze

• Dziękuję bardzo za wgląd! Wszystkie te odpowiedzi uzupełniają każdą inne naprawdę dobrze! ' zacznę pracować nad wersją 2.0 mając to wszystko na uwadze.
• Nie ma problemu, ' cieszę się, że okazało się to przydatne! Powodzenia w wersji 2.0!

Co denerwuje Trochę mi z tym kodem jest to, że nie używam interfejsów, klas abstrakcyjnych ani dziedziczenia, ale nie mogłem znaleźć tutaj dobrego przypadku użycia dla nich.

W Twojej grze nie ma w tym nic złego. Koncepcja gry jest tak prosta, że nie potrzebujesz żadnego z nich. Problem z małymi programami do zabawek polega na tym, że zwykle nie musisz stosować dużych rozwiązań projektowych. Musisz tylko upewnić się, że postępujesz zgodnie ze zwykłymi zasadami projektowania SOLID .

Skoro już to wyjaśniliśmy, pozwólmy spójrz na szczegóły swojego kodu, które powinny zostać ulepszone.

Pierwsza z nich jest dość oczywista. Nie pisz komentarzy tylko po to, by pisać komentarze. Niektórzy nauczyciele lubią zmuszać cię do pisania komentarzy javadoc do wszystkiego. W rzeczywistości jestem całkowicie temu przeciwny, z wyjątkiem sytuacji, gdy piszę jakiś pakiet narzędzi, z którego wszyscy inni muszą korzystać. Zwykle kod powinien być samodokumentujący. Twój kod robi w tym naprawdę dobrą robotę. Po prostu usuń komentarz, który w zasadzie powtarzając dobrze dobraną nazwę zmiennej / funkcji / …

Na przykład:

/** * Is point between boolean. * * @param point the point * @param position the position * @return the boolean */ public static boolean isPointBetween(Point point, Position position) { 

Jaką wartość dodaje ten komentarz do funkcji?Myślę, że jest to jedyna metoda, którą bym zmienił, aby była bardziej czytelna. Ponieważ „nie jest oczywiste, że pozycja składa się z from i to punktu, dla którego sprawdzamy, czy point leży między nimi.

Co by było, gdyby metoda miała taki podpis:

public static boolean containsPoint(Position position, Point point) { 

Czy nie ma trochę więcej sensu?

Powinienem tutaj dodać, że ogólnie nie jestem przeciwny komentarzom. Ale komentarz powinien wyjaśniać DLACZEGO coś się robi w ten sposób. Nie w jaki sposób jest to realizowane. Gdybym chciał wiem, jak to jest zaimplementowane. Właśnie spojrzałbym na kod.

Następną kwestią jest posiadanie tej klasy Util … W przeciwieństwie do niektórych purystów, nie mam nic przeciwko koncepcji klas Util .Klasy Util mogą być przydatne do łączenia podobnych funkcji. Na przykład java.lang.Math, które grupuje wszystkie zwykłe operacje arytmetyczne w jednej klasie.

Problem, który mnie niepokoi z twoją klasą Util jest to, że tak naprawdę nie ma dobrego powodu, aby ona istniała. Dwie funkcje, które tam masz, są używane tylko w klasie Board. Mogłyby więc „równie dobrze być private static funkcjami pomocniczymi wewnątrz tej klasy.

W rzeczywistości możemy nawet zrobić trochę lepiej po zmianie podpisu na to, co Zasugerowałem wcześniej. Co by było, gdybyśmy umieścili containsPoint(Position position, Point point) { wewnątrz klasy Position zamiast Position jako parametr metody? Wtedy możemy go użyć w ten sposób:

Position shipPosition = //some ship"s position if(shipPosition.contains(targetPoint)) { //handle ship hit } 

To naprawdę dobrze tam pasuje, prawda?

A propos klasy Positions. Miałem dziwne przeczucie, przeglądając twój kod. Na początku myślałem, że nie masz something[][] do reprezentowania tablicy. Myślałem, że przedstawiłeś wszystko jako punkty w całej bazie kodu. To może działać, ale sprawia, że drukowanie tablicy jest niewygodne. A potem zauważyłem, że w klasie Board znajduje się char[][]. Ale wtedy nie byłoby sensowniej umieszczać statków w środku tę siatkę bez pośredniej klasy Position?

Zauważyłem też inną niebezpieczną rzecz dotyczącą placeShipsOnBoard(). Czy naprawdę powinieneś po prostu zaufać użytkownikowi, że wprowadzi 2 prawidłowe współrzędne? A co, jeśli użytkownik próbuje być zabawny i wprowadza wartości od = (1,1) do = (2,2)? Czy powinniśmy na to pozwolić? A co, jeśli chcesz, aby wprowadził statek o długości 5 i wprowadził od = (1,1) do = (1,1) zasadniczo zmniejszając statek do jednego kwadratu (musisz trafić 5 razy! ma 5 żyć). Czy nie powinniśmy powstrzymać go przed takim oszukiwaniem?

Przyjrzyjmy się alternatywnemu sposobowi radzenia sobie z umieszczaniem statków. Najpierw pozwól użytkownikowi wybrać, czy chce umieścić statek poziomo czy pionowo. Następnie poproś go, aby wprowadził górną / lewą współrzędną statku. Pozostałe punkty obliczymy sami.

Oto, jak mogłaby wyglądać rzeczywista implementacja metody:

private Scanner scanner = new Scanner(System.in); private void placeShipsOnBoard() { System.out.printf("%nAlright - Time to place out your ships%n%n"); for(Ship ship : ships) { //awesome for-each loop is better here boolean horizontal = askValidShipDirection(); Point startingPoint = askValidStartingPoint(ship, horizontal); placeValidShip(ship, startingPoint, boolean horizontal); } } private boolean askValidShipDirection() { do { System.out.println("Do you want to place the ship horizontally (H) or vertically (V)?"); String direction = Scanner.nextLine().trim(); while ( !"H".equals(direction) && !"V".equals(direction); return "H".equals(direction); //note here: use "constant".equals(variable) so nullpointer is impossible. //probably not needed, but it"s best practice in general. } private Point askValidStartingPoint(Ship ship, boolean horizontal) { do { //note: do-while more useful here System.out.printf("%nEnter position of %s (length %d): ", ship.getName(), ship.getSize()); Point from = new Point(scanner.nextInt(), scanner.nextInt()); while(!isValidStartingPoint(from, ship.getLength(), horizontal)); return from; } private boolean isValidStartingPoint(Point from, int length, boolean horizontal) { int xDiff = 0; int yDiff = 0; if(horizontal) { xDiff = 1; } else { yDiff = 1; } for(int i = 0; i < lenth; i++) { if(!isInsideBoard(i,from.getY()) { return false; } if(!Constants.BOARD_ICON.equals(board[from.getY()+i*yDiff][from.getX()+i*xDiff])){ return false; } } return true; } private boolean isInsideBoard(int x, int y){ return x <= Constants.BOARD_SIZE && x >= 0 && y <= Constants.BOARD_SIZE && y >= 0 && x <= Constants.BOARD_SIZE && x >= 0 && y <= Constants.BOARD_SIZE && y >= 0; } private void placeValidShip(Ship ship, Point startingPoint, boolean horizontal) { int xDiff = 0; int yDiff = 0; if(horizontal) { xDiff = 1; } else { yDiff = 1; } for(int i = 0; i < ship.getLenth() ; i++) { board[ship.getY() + i*yDiff][ship.getX()+i*xDiff] = Constants.SHIP_ICON; } } 

Po prostu umieść statki bezpośrednio na planszy, możemy usunąć klasę Position i wszystkie jej odniesienia. Oznacza to, że nie wiemy już, czy statek zatonął, czy nie.

W trakcie pisania zauważyłem, że @Timothy Truckle również opublikował odpowiedź. Naprawdę podoba mi się jego rozwiązanie polegające na użyciu dedykowanych Field zamiast char „do reprezentowania tablicy.

nasza metoda lokalizacji statku zmienia się na:

private void placeValidShip(Ship ship, Point startingPoint, boolean horizontal) { int xDiff = 0; int yDiff = 0; if(horizontal) { xDiff = 1; } else { yDiff = 1; } for(int i = 0; i < ship.getLenth() ; i++) { board[ship.getY() + i*yDiff][ship.getX()+i*xDiff] = new ShipField(ship); } } 

W ten sposób możemy sprawdzić, czy statek został całkowicie zniszczony, czy tylko częściowo trafiony podczas ataku na Field.

Zamiast kontynuować tę odpowiedź, sugeruję, abyś przeczytał również @Timothy” i przyjrzał się zaletom obu naszych odpowiedzi. Na pierwszy rzut oka zgadzamy się lub po prostu uzupełniamy wzajemnie odpowiedzi. Powinieneś więc mieć kilka przyzwoitych wskazówek, jak ulepszyć swój kod 🙂

Komentarze

• Dziękuję bardzo za szczegółową recenzję! Wszyscy podają solidne uwagi. Ponieważ wszystkie odpowiedzi były tak dobre, ' zaznaczę zaznaczenie osobie, która odpowiedziała pierwszy.
• ahem ” W Twojej grze nie ma nic złego. Koncepcja gry jest tak prosta, że nie ' nie potrzebuję żadnego z nich. ” – Muszę się nie zgodzić: ' lepiej ćwiczyć wszystko najpierw w trybie łatwym. Jeśli nie możesz ' t zastosować pojęć i wzorców do czegoś prostego, na pewno możesz ' t zastosować je na czymś bardziej złożonym.
• Część dotycząca koneksów jest szczególnie ważna. Nadmiarowe komentarze są całkowitą stratą czasu i energii na pisanie ite, konserwacja i weryfikacja. Komentuj tylko wtedy, gdy nie możesz uzyskać kodu i funkcji do wyjaśnienia.

W innych odpowiedziach wskazano prawie wszystko, więc dodam tylko jedną rzecz. Wygląda na to, do mnie, że używasz wyjątków, aby w jakiś sposób sterować przepływem. Wyjątki nie są mechanizmami kontroli przepływu .

public Position(Point from, Point to) { if (from.getX() > Constants.BOARD_SIZE || from.getX() < 0 || from.getY() > Constants.BOARD_SIZE || from.getY() < 0 || to.getX() > Constants.BOARD_SIZE || to.getX() < 0 || to.getY() > Constants.BOARD_SIZE || to.getY() < 0) { throw new ArrayIndexOutOfBoundsException(); } this.from = from; this.to = to; } 

A potem

} catch (IndexOutOfBoundsException e) { System.out.println("Invalid coordinates - Outside board"); } 

Myślę, że przed utworzeniem elementu Position należy sprawdzić, czy podane współrzędne znajdują się wewnątrz tablicy. Jest to informacja wprowadzana przez użytkownika i jest to całkowicie uzasadnione. „Ponownie weryfikujesz już ten ship.getSize() != Utils.distanceBetweenPoints(from, to). Robiłbyś to nawet w samym obiekcie Board, zamiast sprawdzać pozycję dla Constants.BOARD_SIZE.