Mulțumesc pentru partajarea codului dvs.

Ceea ce mă enervează puțin cu acest cod este că nu folosesc interfețe, clase abstracte sau moștenire,

A face OOP înseamnă că urmați anumite principii care sunt (printre altele):

• ascunderea / încapsularea informațiilor
• responsabilitate unică
• separarea preocupărilor
• SĂRUT (Păstrați-l simplu (și) prost.)
• SEC (nu vă repetați.)
• „Spune! Nu te întreba.”
• Legea demeterului („Nu vorbi cu străinii!”)

Interfețe, abs clasele de tract sau principiile de păstrare a moștenirii și ar trebui utilizate după cum este necesar. Acestea nu „definesc” OOP.

IMHO principalul motiv pentru care abordarea dvs. eșuează OOP este că „Modelul” dvs. este o matrice de tip primitiv char. Acest lucru duce în cele din urmă la o abordare procedurală pentru logica jocului.

M-aș gândi la o interfață ca aceasta:

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

unde Result ar fi o enumerare:

 enum Result{ NO_HIT, PARTIAL_HIT, DESTROYED } 

Și aș avea diferite implementări ale interfeței:

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(); } } 

Acest lucru ar trebui să fie suficient, sper că ia ideea …

Probleme formale

Denumirea

Găsirea unor nume bune este cea mai grea parte din programare. Deci, ia-ți întotdeauna timpul să te gândești la numele identificatorilor tăi.

Pe partea luminoasă, urmezi convențiile de denumire Java.

Dar ar trebui ca numele metodelor tale să înceapă cu un verb în timpul prezent.Eg: shipWasHit() ar trebui numit hit().
Sau distanceBetweenPoints() fi calculateDistanceBetween(). Aici parametrii dezvăluie că distanța este între puncte, deci nu este nevoie să o introduceți în numele metodei.

Fii detaliat în numele variabilelor. în loc de

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

aceste variabile ar trebui să fie mai degrabă denumit astfel:

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

Luați-vă numele din domeniul problemei, nu din soluția tehnică. de ex .: SHIP_ICON ar trebui să fie SHIP numai dacă nu aveți o altă constantă în clasa Ship .

Comentarii

Comentariile ar trebui să explice de ce codul este așa cum este . Eliminați toate celelalte comentarii.

comentariile ar trebui utilizate numai pe interfețe sau metode abstracte, în cazul în care conțin contractul pe care implementatorul trebuie să îl îndeplinească.

Clasa constantelor

Puneți împreună lucruri care aparțin împreună. Definiți constantele din clasa care le folosește.

Comentarii

• Toate punctele valide și foarte utile. Mulțumesc!

Există deja câteva răspunsuri bune, dar am crezut că aș adăuga câteva dintre lucrurile care au stat când m-am uitat prin cod.

În momentul de față, singura sursă de intrare este intrarea utilizatorului de pe un scaner. Acest lucru ar face destul de dificil dacă doriți să adăugați un fel de oponenți computerului la joacă împotriva.

Se pare că există un cod în clasa Board care ar putea fi mai potrivit pentru clasa Player.

Mai exact conținutul metodei placeShipsOnBoard ().

Această metodă primește date de la utilizator și creează o poziție. Să încercăm și să o restructurăm astfel încât un jucător (uman sau computer) să poată crea o poziție.

Să facem o interfață

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

Avem deja implementări de la un om,

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 } } 

Și ce zici de un computer player de bază

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

Apoi, în clasa principală de bord, programăm pe interfața playerului

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. } } } 

Dacă tot codul se referă mai degrabă la un Player (acum o interfață) decât la o implementare concretă, putem adăuga cu ușurință noi tipuri de playere pentru computer . de exemplu. nou CheatingComputer (), nou HardComputer (), nou MediumComputer (). Fiecare ar stabili doar unde să tragă în continuare și unde să plaseze următoarea navă într-un mod diferit.

Și, dacă am avea acest lucru, am putea face un joc cu 2 jucători pe computer și lăsăm să se joace singur! Un drept interesant: D

Un alt lucru asemănător pe care l-am putea schimba este că în constructorul tău pentru Game, vei avea întotdeauna 2 jucători umani. Am putea face acest lucru să ia o listă de jucători, astfel încât să puteți avea orice număr de jucători în jocul dvs. Doar pentru că jocul real Battleship este limitat la 2 jucători, nu înseamnă că al tău trebuie să fie.

Am putea permite dimensiunea grilei reglabilă și un număr arbitrar de jucător. Dintr-o dată, avem o grilă 100×100 cu 8 jucător gratuit pentru toți!

(orice cantitate ar putea fi controlată de computer).

Navele dvs. sunt, de asemenea, inițializate într-un bloc static din clasa dvs. de bord. Aveți toate navele reale de la Battleship. Dar, din nou, de ce nu permiteți mai multă flexibilitate aici. Ce se întâmplă dacă nava dvs. ar consta dintr-o listă de puncte? Ați putea avea nave în formă de S, nu ar trebui să se limiteze la orizontală și verticală. (Acest lucru poate fi deasupra, dar cred că „este un lucru interesant de gândit!)

Voi termina cu câteva lucruri mici care mi s-au părut amuzante

throw new ArrayIndexOutOfBoundsException(); 

din clasa Poziție. Aceasta pare a fi o excepție nepotrivită de aruncat aici. Nu există un tablou în clasa Poziție, deci acest lucru trebuie să se refere la tabloul din clasa Board. Cred că un tip de excepție mai potrivit ar fi o IllegalArgumentException, ArrayIndexOutofBoundsException este un detaliu de implementare (de altă clasă!). De asemenea, asigurați-vă că furnizați un mesaj de eroare adecvat împreună cu lansarea excepției. De exemplu, „valoarea trebuie să fie în intervalul x și y”

Linia

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

ar putea fi pur și simplu înlocuit cu

boolean isShipHit = ship != null; 

Nu este nevoie de operator ternar aici.

Utilizarea targetHistory în clasa Player în timp ce (targetHistory.get (point)! = null)

Aici folosiți o hartă cu singurul scop de a vedea dacă un element se află în el. Exact acest lucru este un set pentru!

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

Comentarii

• Vă mulțumesc foarte mult pentru înțelegere! Toate aceste răspunsuri completează fiecare altul foarte bine! Voi ‘ voi începe să lucrez la versiunea 2.0 ținând cont de toate acestea.
• Nicio problemă. ‘ mă bucur că ți s-a părut util! Noroc cu 2.0!

Ce enervează un pic cu acest cod este că nu folosesc interfețe, clase abstracte sau moștenire, dar nu le-am găsit aici un caz de utilizare bun.

Nu este nimic în neregulă cu acest lucru în jocul tău. Conceptul jocului este atât de simplu încât nu aveți nevoie de niciunul dintre acestea. Problema cu programele mici de jucărie este că de obicei nu trebuie să aplicați soluții mari de design. Trebuie doar să vă asigurați că urmăriți principiile obișnuite de proiectare .

Acum, după ce am clarificat, să uită-te la câteva detalii ale codului tău care ar trebui îmbunătățite.

Primul este destul de evident. Nu scrie comentarii pentru a scrie comentarii. Unora dintre profesori le place să te oblige să scrii un comentariu javadoc despre toate. Sunt de fapt complet împotriva acestui lucru, cu excepția cazului în care scriu un fel de pachet utilitar pe care toți ceilalți trebuie să-l folosească. În mod normal, codul ar trebui să se autodocumentează. Și codul dvs. face o treabă foarte bună în acest sens. Deci, eliminați comentariul repetând practic numele bine ales al variabilei / funcției / …

De exemplu:

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

Ce valoare adaugă acel comentariu la funcție?Cred că aceasta este și singura metodă pe care aș schimba-o pentru a o face mai ușor de citit. Deoarece nu este evident că poziția este alcătuită dintr-un punct from și to pentru care verificăm dacă se află între ele.

Ce se întâmplă dacă metoda ar avea această semnătură:

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

N-ar fi ai un pic mai mult sens?

Ar trebui să adaug aici că nu sunt împotriva comentariilor în general. Dar un comentariu ar trebui să explice DE CE se face ceva în acest fel. Nu cum este implementat. Dacă aș vrea știu cum s-a implementat aș vrea să mă uit la cod.

Următorul punct este acela de a avea clasa Util … Spre deosebire de unii puriști, nu am nimic împotriva conceptului de clase Util. . Clasele Util pot fi utile pentru a pune împreună funcții similare. De exemplu, java.lang.Math care grupează toate operațiile aritmetice obișnuite într-o singură clasă.

Lucrul care mă deranjează cu clasa dvs. Util este că nu există un motiv bun pentru a exista. Cele 2 funcții pe care le aveți acolo sunt folosite doar în clasa Board. Deci, ei ar putea „la fel de bine să fie private static funcții de asistență în cadrul acelei clase.

De fapt, putem chiar să facem ceva mai bine după ce am schimbat semnătura în ceea ce Am sugerat mai devreme. Ce se întâmplă dacă punem containsPoint(Position position, Point point) { în clasa Position în loc să avem Position ca parametru pentru metodă? Atunci îl putem folosi astfel:

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

Se potrivește foarte bine acolo, nu-i așa?

Vorbind despre clasa Positions. Am avut un sentiment ciudat în legătură cu acest lucru în timp ce mă uitam prin codul tău. La început, am crezut că nu aveți un something[][] pentru a reprezenta placa. Am crezut că ați reprezentat totul ca puncte în întreaga bază de cod. Acest lucru ar putea funcționa, dar face ca imprimarea plăcii să fie dificilă. Și apoi am observat că există un char[][] în clasa dvs. Board. Dar atunci nu ar avea mai mult sens să puneți imediat navele înăuntru acea grilă fără a avea o clasă de poziție intermediară?

De asemenea, am observat un alt lucru periculos despre placeShipsOnBoard(). Ar trebui să aveți încredere că utilizatorul dvs. va introduce 2 coordonate valide? Ce se întâmplă dacă utilizatorul încearcă să fie amuzant și introduce de la = (1,1) la = (2,2)? Ar trebui să permitem acest lucru? Sau ce se întâmplă dacă vrei să introducă o navă de lungime 5 și să introducă de la = (1,1) la = (1,1) micșorând în esență nava într-un singur pătrat (pe care trebuie să îl lovești de 5 ori! are 5 vieți). Nu ar trebui să-l împiedicăm să înșele așa?

Să vedem o modalitate alternativă de a face față plasării navelor. În primul rând, permiteți utilizatorului să aleagă dacă dorește să plaseze nava pe orizontală sau verticală. Apoi, puneți-l să introducă coordonata de sus / stânga a navei. Vom calcula noi înșine punctele rămase.

Iată cum ar putea arăta implementarea metodei efective:

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; } } 

Acum că tocmai plasați navele direct pe bord, putem elimina clasa Position și toate referințele acesteia. Acest lucru înseamnă că nu mai știm dacă o navă s-a scufundat sau nu.

În timp ce vorbeam, am observat că @Timothy Truckle a postat deja și un răspuns. Îmi place foarte mult soluția sa de a folosi Field s în loc de char „s pentru a reprezenta placa.

metoda navei noastre de loc se schimbă în:

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); } } 

În acest fel putem verifica dacă o navă este distrusă complet sau doar lovită parțial atunci când atacă un Field.

Acum, în loc să continuați cu acest răspuns, vă sugerez să citiți și @Timothy și să examinați punctele bune din ambele noastre răspunsuri. La prima vedere, fie că suntem de acord, fie că ne completăm pur și simplu reciproc. ”Deci, ar trebui să aveți câteva indicații decente despre cum să vă îmbunătățiți codul 🙂

Comentarii

• Vă mulțumesc foarte mult pentru o recenzie detaliată! Toți susțineți puncte solide. Deoarece toate răspunsurile au fost atât de bune, ‘ voi da bifa persoanei care a răspuns mai întâi.
• ahem ” Nu este nimic în neregulă cu acest lucru în jocul dvs. Conceptul jocului este atât de simplu încât nu faceți ‘ nu au nevoie de oricare dintre acestea. ” – Trebuie să nu fiu de acord: este ‘ mai bine să practici orice mai întâi în modul ușor. Dacă nu puteți ‘ să aplicați concepte și modele pe ceva simplu, cu siguranță puteți ‘ t aplicați-le pe ceva mai complex.
• Partea despre cimenturi este deosebit de importantă. Comentariile redundante reprezintă o pierdere completă de timp și energie de ite, întreține și verifica. Comentează numai dacă nu poți obține codul și funcțiile pentru a se explica.

Alte răspunsuri au arătat aproape totul, așa că voi adăuga un singur lucru. Se pare pentru mine că folosiți excepții pentru a efectua cumva controlul fluxului. Excepțiile nu sunt mecanisme de control al fluxului .

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; } 

Și apoi

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

Cred că ar trebui să validați că coordonatele date sunt în interiorul plăcii înainte de a încerca să creați Position. Aceasta este informația utilizatorului și este perfect rezonabilă să o faceți. „Validați deja acel ship.getSize() != Utils.distanceBetweenPoints(from, to). Ați face chiar acest lucru chiar în obiectul Board, în loc să aveți apoi verificarea poziției pentru Constants.BOARD_SIZE.