Danke

Was mich an diesem Code ein wenig ärgert, ist, dass ich keine Schnittstellen, abstrakten Klassen oder Vererbungen verwende.

Wenn Sie OOP ausführen, befolgen Sie bestimmte Prinzipien, die (unter anderem) sind:

• Verstecken / Einkapseln von Informationen
• Einzelverantwortung
• Trennung von Bedenken
• KISS (Halten Sie es einfach (und) dumm.)
• TROCKEN (Wiederholen Sie sich nicht.)
• „Tell! Frag nicht.“
• Gesetz des Demeters („Sprich nicht mit Fremden!“)

Schnittstellen, abs Traktatklassen oder Vererbungsunterstützung haben Prinzipien und sollten nach Bedarf verwendet werden. Sie „definieren“ OOP nicht.

IMHO ist der Hauptgrund, warum Ihr Ansatz OOP fehlschlägt, dass Ihr „Modell“ ein Array eines primitiven Typs char ist. Dies führt letztendlich zu einem prozeduralen Ansatz für die Spielelogik.

Ich würde mir eine Schnittstelle wie diese vorstellen:

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

wobei Result wäre eine Aufzählung:

 enum Result{ NO_HIT, PARTIAL_HIT, DESTROYED } 

Und ich hätte verschiedene Implementierungen der Schnittstelle:

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

Dies sollte ausreichen, hoffe Sie Machen Sie sich ein Bild …

Formale Probleme

Benennen

Gute Namen zu finden ist der schwierigste Teil bei der Programmierung. Nehmen Sie sich also immer Zeit, um über Ihre Bezeichnernamen nachzudenken.

Auf der positiven Seite befolgen Sie die Java-Namenskonventionen.

Ihre Methodennamen sollten jedoch mit einem Verb beginnen Präsens.Eg: shipWasHit() sollte hit() heißen.
Oder distanceBetweenPoints() sollte sei calculateDistanceBetween(). Hier zeigen die Parameter, dass der Abstand zwischen Punkten liegt, sodass Sie ihn nicht in den Methodennamen einfügen müssen.

Geben Sie Ihre Variablennamen ausführlich ein. Anstelle von

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

sollten diese Variablen eher sein benannt wie folgt:

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

Nehmen Sie Ihre Namen aus der Problemdomäne, nicht aus der technischen Lösung. Beispiel: SHIP_ICON sollte nur SHIP sein, es sei denn, Sie haben eine andere Konstante innerhalb der Klasse Ship

Kommentare

Kommentare sollten erklären, warum der Code so ist, wie er ist . Entfernen Sie alle anderen Kommentare.

Kommentare sollten nur für Schnittstellen- oder abstrakte Methoden verwendet werden, bei denen sie den Vertrag enthalten, den der Implementierer erfüllen muss.

Konstantenklasse

Stellen Sie Dinge zusammen, die zusammen gehören. Definieren Sie Konstanten in der Klasse, die sie verwendet.

Kommentare

• Alle gültigen Punkte und sehr hilfreich. Danke!

Es gibt bereits einige gute Antworten, aber ich dachte, ich würde einige der Dinge hinzufügen, die standen für mich, als ich den Code durchgesehen habe.

Im Moment ist die einzige Eingabequelle die Benutzereingabe von einem Scanner. Dies würde es ziemlich schwierig machen, wenn Sie eine Art Computergegner hinzufügen möchten spielen gegen.

Es sieht so aus, als ob es in der Board-Klasse Code gibt, der möglicherweise besser für die Player-Klasse geeignet ist.

Speziell den Inhalt der placeShipsOnBoard () -Methode.

Diese Methode nimmt Eingaben vom Benutzer entgegen und erstellt eine Position. Versuchen wir, diese neu zu strukturieren, damit ein Spieler (Mensch oder Computer) eine Position erstellen kann.

Lassen Sie uns eine Position erstellen Schnittstelle

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

Wir haben bereits Implementierungen von einem Menschen,

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

Und was ist ein grundlegender Computer-Player

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

Dann programmieren wir in der Hauptplatinenklasse auf die Player-Oberfläche

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

Wenn sich der gesamte Code eher auf einen Player (jetzt eine Schnittstelle) als auf eine konkrete Implementierung bezieht, können wir problemlos neue Arten von Computer-Playern hinzufügen . z.B. neuer CheatingComputer (), neuer HardComputer (), neuer MediumComputer (). Jeder würde nur bestimmen, wo er als nächstes schießen und wo er das nächste Schiff auf andere Weise platzieren soll.

Und wenn wir das hätten, könnten wir ein Spiel mit 2 Computerspielern machen und es selbst spielen lassen! Aufregend richtig: D

Eine weitere verwandte Sache, die wir ändern könnten, ist, dass Sie in Ihrem Konstruktor für Game immer 2 menschliche Spieler haben. Wir könnten dafür sorgen, dass eine Liste von Spielern erstellt wird, sodass Sie eine beliebige Anzahl von Spielern in Ihrem Spiel haben können. Nur weil das echte Schlachtschiff auf 2 Spieler beschränkt ist, heißt das nicht, dass es dein sein muss.

Wir könnten eine einstellbare Gittergröße und eine beliebige Spielernummer zulassen. Plötzlich haben wir ein 100×100-Gitter mit einer 8 Spieler frei für alle!

(jede Menge davon könnte computergesteuert sein).

Ihre Schiffe werden auch in einem statischen Block in Ihrer Board-Klasse initialisiert. Sie haben alle echten Schiffe Aber was ist, wenn Ihr Schiff aus einer Liste von Punkten besteht? Sie könnten S-förmige Schiffe haben, die sich nicht auf die horizontale oder vertikale Ausrichtung beschränken müssten. (Das mag übertrieben sein, aber ich denke, es ist „eine coole Sache, darüber nachzudenken!)

Ich werde mit ein paar kleinen Dingen fertig werden, die für mich lustig aussahen

throw new ArrayIndexOutOfBoundsException(); 

aus der Position-Klasse. Dies scheint eine unangemessene Ausnahme zu sein, um hier zu werfen. Da die Position-Klasse kein Array enthält, muss sich dies auf das Array in der Board-Klasse beziehen. Ich denke, ein geeigneterer Ausnahmetyp wäre eine IllegalArgumentException. ArrayIndexOutofBoundsException ist ein Implementierungsdetail (einer anderen Klasse!) Stellen Sie außerdem sicher, dass Sie zusammen mit dem Auslösen der Ausnahme eine entsprechende Fehlermeldung angeben. Beispiel: „Wert muss innerhalb des Bereichs x und y liegen“

Die Zeile

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

kann einfach durch

boolean isShipHit = ship != null; 

ersetzt werden. Der ternäre Operator ist hier nicht erforderlich.

Die Verwendung von targetHistory in der Player-Klasse while (targetHistory.get (point)! = null)

Hier verwenden Sie eine Map, um zu sehen, ob sich ein Element darin befindet. Genau das ist ein Set für!

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

Kommentare

• Vielen Dank für den Einblick! Alle diese Antworten ergänzen sich andere wirklich gut! Ich ‚ werde anfangen, an Version 2.0 zu arbeiten Vor diesem Hintergrund.
• Kein Problem Ich ‚ bin froh, dass Sie es nützlich fanden! Viel Glück mit 2.0!

Was nervt Ein bisschen mit diesem Code ist, dass ich keine Schnittstellen, abstrakten Klassen oder Vererbung verwende, aber ich konnte hier keinen guten Anwendungsfall für sie finden.

Daran ist in Ihrem Spiel nichts auszusetzen. Das Spielkonzept ist so einfach, dass Sie keines davon benötigen. Das Problem bei kleinen Spielzeugprogrammen ist, dass Sie normalerweise keine großen Designlösungen anwenden müssen. Sie müssen nur sicherstellen, dass Sie die üblichen SOLID-Designprinzipien befolgen.

Nachdem wir das geklärt haben, lassen Sie uns Schauen Sie sich einige Details Ihres Codes an, die verbessert werden sollten.

Der erste ist ziemlich offensichtlich. Schreiben Sie keine Kommentare, um Kommentare zu schreiben. Einige Lehrer zwingen Sie gerne dazu, einen Javadoc-Kommentar zu allem zu schreiben. Ich bin eigentlich völlig dagegen, außer wenn ich eine Art Hilfspaket schreibe, das alle anderen verwenden müssen. Normalerweise sollte Code selbstdokumentierend sein. Und Ihr Code leistet dabei wirklich gute Arbeit. Entfernen Sie einfach den Kommentar, der „s“ ist Im Grunde genommen wird der gut gewählte Name der Variablen / function / …

wiederholt. Beispiel:

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

Welchen Wert fügt dieser Kommentar hinzu zur Funktion?Ich denke, dies ist auch die einzige Methode, die ich ändern würde, um sie besser lesbar zu machen. Weil es nicht offensichtlich ist, dass die Position aus einem from und to Punkt besteht, für den wir prüfen, ob unsere point liegt zwischen ihnen.

Was wäre, wenn die Methode diese Signatur hätte:

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

Würde das nicht ein bisschen mehr Sinn machen?

Ich sollte hier hinzufügen, dass ich nicht gegen Kommentare im Allgemeinen bin. Aber ein Kommentar sollte erklären, WARUM etwas so gemacht wird. Nicht wie es implementiert wird. Wenn ich wollte Ich weiß, wie es implementiert ist. Ich hätte mir nur den Code angesehen.

Der nächste Punkt ist diese Util-Klasse … Im Gegensatz zu einigen Puristen habe ich nichts gegen das Konzept der Util-Klassen Util-Klassen können nützlich sein, um ähnliche Funktionen zusammenzustellen. Zum Beispiel java.lang.Math, das alle üblichen arithmetischen Operationen in einer Klasse zusammenfasst.

Das, was mich stört Bei Ihrer Util-Klasse gibt es keinen guten Grund dafür. Die 2 Funktionen, die Sie dort haben, werden immer nur innerhalb der Board-Klasse verwendet. Sie könnten also genauso gut private static Hilfsfunktionen innerhalb dieser Klasse sein.

Tatsächlich können wir es sogar ein wenig besser machen, nachdem wir die Signatur in was geändert haben Ich habe es früher vorgeschlagen. Was ist, wenn wir containsPoint(Position position, Point point) { in die Klasse Position einfügen, anstatt die Position als Parameter für die Methode? Dann können wir es folgendermaßen verwenden:

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

Es passt wirklich gut dorthin, nicht wahr?

Apropos Positions -Klasse. Ich hatte ein komisches Gefühl dabei, als ich Ihren Code durchgesehen habe. Zuerst dachte ich, Sie hätten keine something[][], um das Board darzustellen. Ich dachte, Sie hätten alles als Punkte in der gesamten Codebasis dargestellt. Dies könnte funktionieren, macht aber das Drucken des Boards umständlich. Und dann bemerkte ich, dass in Ihrer Board -Klasse ein char[][] vorhanden ist. Aber dann wäre es nicht sinnvoller, die Schiffe sofort hinein zu setzen dieses Gitter ohne Zwischenposition?

Ich habe auch eine andere gefährliche Sache an der placeShipsOnBoard() bemerkt. Sollten Sie Ihrem Benutzer wirklich nur vertrauen, dass er 2 gültige Koordinaten eingibt? Was ist, wenn der Benutzer versucht, lustig zu sein und Eingaben von = (1,1) bis = (2,2) macht? Sollen wir das zulassen? Oder was ist, wenn Sie möchten, dass er ein Schiff der Länge 5 eingibt und er von = (1,1) bis = (1,1) eingibt, wodurch das Schiff im Wesentlichen auf ein einzelnes Quadrat verkleinert wird (das Sie seit dem Schiff 5 Mal treffen müssen!) hat 5 Leben). Sollten wir ihn nicht daran hindern, so zu betrügen?

Schauen wir uns eine alternative Möglichkeit an, die Schiffe zu platzieren. Lassen Sie den Benutzer zunächst auswählen, ob er das Schiff horizontal oder vertikal platzieren möchte. Lassen Sie ihn dann die obere / linke Koordinate des Schiffes eingeben. Wir werden die verbleibenden Punkte selbst berechnen.

So könnte die tatsächliche Methodenimplementierung aussehen:

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

Nun, da wir gerade sind Platzieren Sie die Schiffe direkt auf dem Brett. Wir können die Klasse Position und alle Referenzen entfernen. Dies bedeutet, dass wir nicht mehr wissen, ob ein Schiff gesunken ist oder nicht.

Während ich dies tat, bemerkte ich, dass @Timothy Truckle bereits eine Antwort gepostet hat. Ich liebe seine Lösung, dedizierte Field s anstelle von char „s zu verwenden, um das Board darzustellen.

also Unsere Place-Ship-Methode ändert sich zu:

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

Auf diese Weise können wir überprüfen, ob ein Schiff vollständig zerstört oder nur teilweise getroffen wird, wenn wir eine .

Anstatt mit dieser Antwort fortzufahren, schlage ich vor, dass Sie auch @Timothys lesen und sich die guten Punkte in unseren beiden Antworten ansehen. Auf den ersten Blick sind wir uns entweder einig oder ergänzen uns einfach gegenseitig. Daher sollten Sie einige anständige Hinweise zur Verbesserung Ihres Codes haben 🙂

Kommentare

• Vielen Dank für eine ausführliche Bewertung! Sie alle machen solide Punkte. Da alle Antworten so gut waren, werde ich ‚ der Person, die geantwortet hat, das Häkchen geben zuerst.
• ahem “ Daran ist in Ihrem Spiel nichts auszusetzen. Das Spielkonzept ist so einfach, dass Sie ‚ brauche keine davon. “ – Ich muss nicht zustimmen: ‚ ist besser zu üben alles zuerst im einfachen Modus. Wenn Sie ‚ keine Konzepte und Muster auf etwas Einfaches anwenden können, können Sie sicher ‚ Wenden Sie sie nicht auf etwas Komplexeres an.
• Der Teil über die Kommentare ist besonders wichtig. Redundante Kommentare sind eine völlige Verschwendung von Zeit und Energie, um sie zu schreiben ite, pflegen und verifizieren. Kommentieren Sie nur, wenn Sie den Code und die Funktionen nicht erhalten, um sich selbst zu erklären.

Andere Antworten haben fast alles aufgezeigt, also füge ich nur eine Sache hinzu. Es sieht so aus für mich, dass Sie Ausnahmen verwenden, um irgendwie eine Flusskontrolle durchzuführen. Ausnahmen sind keine Kontrollflussmechanismen .

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

Und dann

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

Ich denke, Sie sollten überprüfen, ob sich die angegebenen Koordinaten innerhalb der Platine befinden, bevor Sie versuchen, die Position zu erstellen. Dies ist eine Benutzereingabe und durchaus sinnvoll. Sie validieren bereits das ship.getSize() != Utils.distanceBetweenPoints(from, to). Sie würden dies sogar im Board -Objekt selbst tun, anstatt dann die Positionsprüfung für Constants.BOARD_SIZE.