Grazie per condividere il tuo codice.

Quello che mi infastidisce un po con questo codice è che non uso interfacce, classi astratte o ereditarietà,

Fare OOP significa seguire determinati principi che sono (tra gli altri):

• nascondere / incapsulare le informazioni
• singola responsabilità
• separazione delle preoccupazioni
• BACIO (Sii semplice (e) stupido.)
• ASCIUTTO (Non ripetere te stesso.)
• “Dillo! Non” chiedere. “
• Legge di Demetra (” Non parlare con estranei! “)

Interfacce, addominali classi di tratto, o ereditarietà, supportano i principi del cappello e dovrebbero essere usati secondo necessità. Non “definiscono” lOOP.

IMHO il motivo principale per cui il tuo approccio fallisce lOOP è che il tuo “Modello” è un array di un tipo primitivo char. Questo alla fine porta a un approccio procedurale per la logica del gioco.

Penserei a uninterfaccia come questa:

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

dove Result sarebbe unenumerazione:

 enum Result{ NO_HIT, PARTIAL_HIT, DESTROYED } 

E avrei diverse implementazioni dellinterfaccia:

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

Questo dovrebbe essere sufficiente, spero che tu fatti unidea …

Questioni formali

Denominazione

Trovare buoni nomi è la parte più difficile della programmazione. Quindi prenditi sempre il tuo tempo per pensare ai nomi degli identificatori.

Il lato positivo è che segui le convenzioni di denominazione Java.

Ma dovresti fare in modo che i nomi dei tuoi metodi inizino con un verbo nella sua tempo presente.Eg: shipWasHit() dovrebbe essere chiamato hit().
Oppure distanceBetweenPoints() dovrebbe essere calculateDistanceBetween(). Qui i parametri rivelano che la distanza è tra i punti, quindi non è necessario inserirla nel nome del metodo.

Sii prolisso nei nomi delle tue variabili. invece di

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

queste variabili dovrebbero essere denominato in questo modo:

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

Prendi i tuoi nomi dal dominio del problema, non dalla soluzione tecnica. ad esempio: SHIP_ICON dovrebbe essere solo SHIP a meno che tu non abbia unaltra costante allinterno della classe Ship .

Commenti

I commenti dovrebbero spiegare perché il codice è così comè . Rimuovi tutti gli altri commenti.

i commenti devono essere utilizzati solo su interfaccia o metodi astratti in cui contengono il contratto che limplementatore deve soddisfare.

Classe costanti

Metti insieme le cose che appartengono insieme. Definisci le costanti nella classe che le utilizza.

Commenti

• Tutti i punti validi e davvero utili. Grazie!

Ci sono già alcune buone risposte ma ho pensato di aggiungere alcune delle cose che erano quando ho esaminato il codice.

Al momento, lunica fonte di input è linput dellutente da uno Scanner. Ciò renderebbe piuttosto difficile se si volesse aggiungere una sorta di avversari controllati dal computer a giocare contro.

Sembra che ci sia del codice nella classe Board che potrebbe essere più adatto alla classe Player.

In particolare i contenuti del metodo placeShipsOnBoard ().

Questo metodo prende linput dellutente e crea una posizione. Proviamo a ristrutturarla in modo che un giocatore (umano o computer) possa creare una posizione.

Facciamo un interfaccia

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

Abbiamo già implementazioni da un umano,

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

E per quanto riguarda un lettore di computer di base

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

Quindi, nella classe Board principale, programmiamo sullinterfaccia Player

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

Se tutto il codice fa riferimento a un lettore (ora uninterfaccia) piuttosto che a unimplementazione concreta, possiamo facilmente aggiungere nuovi tipi di lettori di computer . per esempio. new CheatingComputer (), nuovo HardComputer (), nuovo MediumComputer (). Ognuno determinerebbe semplicemente dove sparare e dove posizionare la prossima nave in un modo diverso.

E, se avessimo questo, potremmo creare un gioco con 2 giocatori di computer e lasciarlo giocare da solo! Eccitante diritto: D

Unaltra cosa correlata che potremmo cambiare è che nel tuo costruttore per il gioco, avrai sempre 2 giocatori umani. Potremmo fare in modo che questo prenda un elenco di giocatori, quindi potresti avere un numero qualsiasi di giocatori nel tuo gioco. Solo perché il vero gioco Battleship è limitato a 2 giocatori, non significa che il tuo debba essere.

Potremmo consentire dimensioni della griglia regolabili e un numero di giocatori arbitrario. Improvvisamente abbiamo una griglia 100×100 con un 8 giocatore gratis per tutti!

(qualsiasi importo può essere controllato dal computer).

Le tue navi sono anche inizializzate in un blocco statico nella tua classe di bordo. Hai tutte le navi reali dalla corazzata. Ma ancora una volta, perché non consentire una maggiore flessibilità qui. E se la tua nave fosse composta da un elenco di punti? Potresti avere navi a forma di S, non dovrebbero essere limitate allallineamento orizzontale o verticale. (Potrebbe essere esagerato, ma penso che sia “una cosa interessante a cui pensare!)

Finirò con alcune piccole cose che mi sono sembrate divertenti

throw new ArrayIndexOutOfBoundsException(); 

dalla classe Position. Sembra uneccezione inappropriata da lanciare qui. Non esiste un array nella classe Position, quindi deve riferirsi allarray nella classe Board. Penso che un tipo di eccezione più adatto sarebbe IllegalArgumentException, ArrayIndexOutofBoundsException è un dettaglio di implementazione (di una classe diversa!). Dovresti assicurati inoltre di fornire un messaggio di errore appropriato oltre a lanciare leccezione. Ad esempio “il valore deve essere compreso nellintervallo xey”

La riga

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

potrebbe essere semplicemente sostituito da

boolean isShipHit = ship != null; 

Non cè bisogno delloperatore ternario qui.

Luso di targetHistory nella classe Player while (targetHistory.get (point)! = null)

Qui stai usando una mappa al solo scopo di vedere se un elemento è in essa. Questo è esattamente ciò che è un Set per!

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

Commenti

• Grazie mille per lintuizione! Tutte queste risposte completano ciascuna altro molto bene! ‘ inizierò a lavorare sulla versione 2.0 con tutto questo in mente.
• Nessun problema, ‘ sono contento che tu labbia trovato utile! Buona fortuna con 2.0!

Cosa infastidisce un po con questo codice non uso interfacce, classi astratte o ereditarietà, ma non sono riuscito a trovare un buon caso duso per loro qui.

Non cè niente di sbagliato in questo nel tuo gioco. Il concetto di gioco è così semplice che non ne hai bisogno. Il problema con i piccoli programmi giocattolo è che di solito non hai bisogno di applicare grandi soluzioni di design. Devi solo assicurarti di seguire i soliti principi di progettazione SOLIDI .

Ora che abbiamo chiarito questo, vediamo “s guarda alcuni dettagli del tuo codice che dovrebbero essere migliorati.

Il primo è abbastanza ovvio. Non scrivere commenti per il gusto di scrivere commenti. Ad alcuni insegnanti piace costringerti a scrivere un commento javadoc su tutto. In realtà sono completamente contrario a questo, tranne quando si scrive una sorta di pacchetto di utilità che tutti gli altri devono usare. Normalmente il codice dovrebbe documentarsi da solo. E il tuo codice fa davvero un buon lavoro. Quindi rimuovi il commento che “s fondamentalmente ripetendo il nome ben scelto della variabile / funzione / …

Ad esempio:

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

Quale valore aggiunge quel commento alla funzione?Penso che questo sia anche lunico metodo che cambierei per renderlo più leggibile. Perché “non è ovvio che la posizione sia composta da un punto from e to per il quale controlliamo se il nostro point si trova tra di loro.

E se il metodo avesse questa firma:

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

Non lo farei ha un po più senso?

Dovrei aggiungere qui che non sono contrario ai commenti in generale. Ma un commento dovrebbe spiegare PERCHÉ qualcosa viene fatto in questo modo. Non come viene implementato. Se volessi so come è implementato, avrei solo guardato il codice.

Il punto successivo è avere quella classe Util … A differenza di alcuni puristi, non ho nulla contro il concetto di classi Util Le classi Util possono essere utili per mettere insieme funzioni simili. Ad esempio java.lang.Math che raggruppa tutte le solite operazioni aritmetiche in una classe.

La cosa che mi preoccupa con la tua classe Util è che non cè davvero una buona ragione per cui esista. Le 2 funzioni che hai lì sono sempre e solo utilizzate allinterno della classe Board. Quindi potrebbero “essere state altrettanto bene private static funzioni di supporto allinterno di quella classe.

In effetti, possiamo anche fare un po meglio dopo aver cambiato la firma in cosa Ho suggerito prima. E se mettessimo containsPoint(Position position, Point point) { nella classe Position invece di avere la Position come parametro per il metodo? Quindi possiamo usarlo in questo modo:

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

Si adatta molto bene lì, non è vero?

A proposito della classe Positions. Ho avuto una strana sensazione al riguardo mentre guardavo il tuo codice. Allinizio pensavo che non avessi un something[][] per rappresentare la scheda. Pensavo che avessi rappresentato tutto come punti nellintera base di codice. Questo potrebbe funzionare ma rende scomoda la stampa della scheda. E poi ho notato che cè un char[][] nella tua classe Board. Ma allora non avrebbe più senso mettere immediatamente le navi allinterno quella griglia senza avere una classe Posizione intermedia?

Ho notato anche unaltra cosa pericolosa sul placeShipsOnBoard(). Dovresti davvero fidarti solo del tuo utente per inserire 2 coordinate valide? E se lutente cerca di essere divertente e inserisce da = (1,1) a = (2,2)? Dovremmo permetterlo? O se vuoi che inserisca una nave di lunghezza 5 e lui inserisce da = (1,1) a = (1,1) essenzialmente riducendo la nave a un singolo quadrato (che devi colpire 5 volte! Poiché la nave ha 5 vite). Non dovremmo impedirgli di barare in questo modo?

Vediamo un modo alternativo per gestire il posizionamento delle navi. Innanzitutto, lascia che lutente scelga se vuole posizionare la nave orizzontalmente o verticalmente. Quindi chiedigli di inserire la coordinata superiore / sinistra della nave. Calcoleremo noi stessi i punti rimanenti.

Ecco come potrebbe apparire leffettiva implementazione del metodo:

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

Ora che ci limitiamo Posizionare le navi direttamente sul tabellone possiamo rimuovere la classe Position e tutti i suoi riferimenti. Ciò significa che non sappiamo più se una nave è affondata o meno.

Mentre lo scrivevo, ho notato che anche @Timothy Truckle aveva già pubblicato una risposta. Adoro la sua soluzione di utilizzare Field invece di char “per rappresentare il tabellone.

quindi il nostro metodo di destinazione della nave cambia in:

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

In questo modo possiamo verificare se una nave viene distrutta completamente o solo parzialmente colpita quando attacca un Field.

Ora invece di continuare con questa risposta, ti suggerisco di leggere anche @Timothy” e di guardare i punti positivi in entrambe le nostre risposte. A prima vista, siamo daccordo o semplicemente ci completiamo a vicenda. Quindi dovresti avere alcuni suggerimenti decenti su come migliorare il tuo codice 🙂

Commenti

• Grazie mille per la recensione dettagliata! Tutti voi fate dei punti concreti. Poiché tutte le risposte sono state così buone, ‘ darò il segno di spunta alla persona che ha risposto prima.
• ahem ” Non cè niente di sbagliato nel tuo gioco. Il concetto di gioco è così semplice che non ‘ non ne ho bisogno. ” – Devo non essere daccordo: ‘ è meglio esercitarsi qualsiasi cosa prima in modalità facile. Se puoi ‘ t applicare concetti e schemi a qualcosa di semplice, puoi sicuramente ‘ t applicarli a qualcosa di più complesso.
• La parte sui conmenti è particolarmente importante. I commenti ridondanti sono una completa perdita di tempo ed energia per scrivere ite, mantieni e verifica. Commenta solo se non riesci a ottenere il codice e le funzioni per spiegare se stessi.

Altre risposte hanno evidenziato quasi tutto, quindi aggiungerò solo una cosa. Sembra per me che stai usando le eccezioni per eseguire in qualche modo il controllo del flusso. Le eccezioni non sono meccanismi di controllo del flusso .

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

E poi

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

Penso che dovresti verificare che le coordinate fornite siano allinterno della scheda prima di provare a creare Position. Questo è linput dellutente ed è perfettamente ragionevole farlo. Stai già convalidando quelloggetto ship.getSize() != Utils.distanceBetweenPoints(from, to). Lo faresti anche nelloggetto Board stesso, invece di controllare la posizione per Constants.BOARD_SIZE.