Tack för att dela din kod.

Det som irriterar mig lite med den här koden är att jag inte använder gränssnitt, abstrakta klasser eller arv,

Att göra OOP innebär att du följer vissa principer som är (bland andra):

• information döljer / inkapslar
• enskilt ansvar
• åtskillnad mellan oro
• KISS (Håll det enkelt (och) dumt.)
• TORR (upprepa inte dig själv.) li>
• ”Berätta! Fråga inte.”
• Demeterlag (”Prata inte med främlingar!”)

Gränssnitt, abs traktklasser eller principer för arvstöd och bör användas efter behov. De ”definierar” inte OOP.

IMHO den främsta anledningen till att ditt tillvägagångssätt misslyckas OOP är att din ”modell” är en matris av primitiv typ char. Detta leder i slutändan till en procedurell strategi för spellogiken.

Jag skulle tänka på ett sådant gränssnitt:

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

där Result skulle vara ett antal:

 enum Result{ NO_HIT, PARTIAL_HIT, DESTROYED } 

Och jag skulle ha olika implementeringar av gränssnittet:

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

Detta borde vara tillräckligt, hoppas du få idén …

Formella frågor

Namngivning

Att hitta bra namn är den svåraste delen i programmeringen. Så ta dig alltid tid att tänka på dina identifieringsnamn.

På den ljusa sidan följer du Java-namngivningskonventionerna.

Men du bör ha dina metodnamn att börja med ett verb i nutid.Eg: shipWasHit() ska heta hit().
Eller distanceBetweenPoints() borde vara calculateDistanceBetween(). Här avslöjar parametrarna att avståndet är mellan punkter, så inget behov av att sätta det i metodnamnet.

Var noggrann i dina variabelnamn. istället för

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

bör dessa variabler snarare vara heter så här:

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

Ta dina namn från problemdomänen, inte från den tekniska lösningen. t.ex.: SHIP_ICON bör endast vara SHIP om du inte har en annan konstant inom klassen Ship .

Kommentarer

Kommentarer ska förklara varför koden är som den är . Ta bort alla andra kommentarer.

kommentarer ska endast användas på gränssnitt eller abstrakta metoder där de innehåller det kontrakt som implementeraren måste uppfylla.

Konstanterklass

Sätt ihop saker som hör ihop. Definiera konstanter i klassen som använder dem.

Kommentarer

• Alla giltiga punkter och verkligen hjälpsamma. Tack!

Det finns redan några bra svar men jag trodde att jag skulle lägga till några av de saker som stod när jag tittade igenom koden.

För tillfället är den enda ingångskällan användarinmatning från en skanner. Det skulle göra det ganska svårt om du vill lägga till någon slags datormotståndare till spela mot.

Det ser ut som om det finns någon kod i Board-klassen som kanske passar bättre i Player-klassen.

Specifikt innehållet i metoden placeShipsOnBoard ().

Denna metod tar inmatning från användaren och skapar en position. Låt oss försöka omstrukturera den så att en spelare (mänsklig eller dator) kan skapa en position.

Låt oss göra en gränssnitt

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

Vi har redan implementeringar från en människa,

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

Och vad sägs om en grundläggande datorspelare

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

Sedan, i huvudkortklassen, programmerar vi till spelargränssnittet

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

Om all kod hänvisar till en spelare (nu ett gränssnitt) snarare än en konkret implementering kan vi enkelt lägga till nya typer av datorspelare . t.ex. ny CheatingComputer (), ny HardComputer (), ny MediumComputer (). Var och en skulle bara bestämma var de ska skjuta nästa och var de ska placera nästa skepp på ett annat sätt.

Och om vi hade det här kunde vi skapa ett spel med två datorspelare och låta det spela själv! Spännande rätt: D

En annan relaterad sak som vi kan ändra är att du i din konstruktör för spel alltid kommer att ha två mänskliga spelare. Vi kan få detta att ta en lista över spelare, så att du kan ha valfritt antal spelare i ditt spel. Bara för att det riktiga spelet Battleship är begränsat till två spelare betyder inte det att ditt måste vara.

Vi kan möjliggöra justerbar rutstorlek och ett godtyckligt spelarnummer. Plötsligt har vi ett 100×100 rutnät med ett 8 spelare gratis för alla!

(vilken mängd som helst som kan datorstyras).

Dina fartyg initialiseras också i ett statiskt block i din brädklass. Du har alla riktiga fartyg från Battleship. Men igen, varför inte tillåta mer flexibilitet här. Vad händer om ditt fartyg bestod av en lista med punkter? Du kan ha S-formade fartyg, de behöver inte begränsas till horisontell vertikal inriktning. (Det här kan vara överst, men jag tycker att det är en cool sak att tänka på!)

Jag slutar med några små saker som såg roliga ut för mig

throw new ArrayIndexOutOfBoundsException(); 

från positionsklassen. Detta verkar som ett olämpligt undantag att kasta här. Det finns ingen matris i positionsklassen, så det här måste syfta på matrisen i styrelseklassen. Jag tror att en mer lämplig undantagstyp skulle vara en IllegalArgumentException, ArrayIndexOutofBoundsException är en implementeringsdetalj (av en annan klass!). Se också till att ge ett lämpligt felmeddelande tillsammans med att kasta undantaget. t.ex. ”värde måste vara inom intervallet x och y”

Raden

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

kan helt enkelt ersättas med

boolean isShipHit = ship != null; 

Det finns inget behov av den ternära operatören här.

Användningen av targetHistory i Player-klassen medan (targetHistory.get (point)! = null)

Här använder du en karta för det enda syftet att se om ett element finns i det. Det är precis vad en uppsättning är för!

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

Kommentarer

• Tack så mycket för insikten! Alla dessa svar kompletterar var och en andra riktigt bra! Jag ’ Jag börjar arbeta med version 2.0 med allt detta i åtanke.
• Inget problem Jag ’ är glad att du tyckte att det var användbart! Lycka till med 2.0!

Vad irriterar mig lite med den här koden är att jag inte använder gränssnitt, abstrakta klasser eller arv, men jag kunde inte hitta ett bra användningsfall för dem här.

Det är inget fel med detta i ditt spel. Spelkonceptet är så enkelt att du inte behöver något av det. Problemet med små leksaksprogram är att du vanligtvis inte behöver använda stora designlösningar. Du behöver bara se till att du följer de vanliga SOLID-designprinciperna .

Nu när vi fick det rensat låt oss titta på några detaljer i din kod som bör förbättras.

Den första är ganska uppenbar. Skriv inte kommentarer för att skriva kommentarer. Vissa lärare gillar att tvinga dig att skriva en javadoc-kommentar till allt. Jag är faktiskt helt emot detta, förutom när jag skriver något slags verktygspaket som alla andra måste använda. Normalt sett borde koden vara självdokumenterande. Och din kod gör ett riktigt bra jobb med det. Så ta bara bort kommentaren som ”s i princip upprepa det väl valda namnet på variabeln / funktionen / …

Till exempel:

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

Vilket värde lägger den kommentaren till till funktionen?Jag tror att detta också är den enda metoden som jag skulle ändra för att göra den mer läsbar. Eftersom det inte är uppenbart att positionen består av en from och to punkt för vilken vi kontrollerar om vår point ligger mellan dem.

Vad händer om metoden hade den här signaturen:

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

Skulle inte det göra lite mer meningsfullt?

Jag borde lägga till här att jag inte är emot kommentarer i allmänhet men. Men en kommentar ska förklara varför något görs på det sättet. Inte hur det implementeras. Om jag ville vet hur den har implementerats skulle jag bara ha tittat på koden.

Nästa punkt är att ha den Util-klassen … Till skillnad från vissa purister har jag inget emot begreppet Util-klasser Util-klasser kan vara användbara för att sätta ihop liknande funktioner. Till exempel java.lang.Math som grupperar alla vanliga aritmetiska operationer i en klass.

Det som stör mig med din Util-klass är att det inte finns någon bra anledning för att den ska existera. De två funktionerna som du har där inne används bara någonsin inom styrelseklassen. Så de kunde ”lika gärna ha varit private static hjälparfunktioner i den klassen.

Vi kan till och med göra lite bättre efter att ha ändrat signaturen till vad Jag föreslog tidigare. Vad händer om vi lägger containsPoint(Position position, Point point) { inuti Position -klassen istället för att ha Position som parameter för metoden? Då kan vi använda den så här:

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

Det passar riktigt bra där, eller hur?

På tal om klassen Positions. Jag hade en konstig känsla av detta när jag tittade igenom din kod. Först trodde jag att du inte hade en something[][] för att representera tavlan. Jag trodde att du hade representerat allt som poäng under hela kodbasen. Och sedan märkte jag att jag hade en char[][] inuti din Board -klass. Men skulle det inte vara vettigare att omedelbart sätta fartygen inuti det rutnät utan att ha en mellanliggande positionsklass?

Jag märkte också en annan farlig sak med placeShipsOnBoard(). Ska du verkligen lita på att din användare anger 2 giltiga koordinater? Vad händer om användaren försöker vara rolig och matar in från = (1,1) till = (2,2)? Ska vi tillåta detta? Eller tänk om du vill att han ska mata in ett fartyg med längd 5 och han matar in från = (1,1) till = (1,1), vilket i huvudsak krymper fartyget till en enda kvadrat (som du måste slå 5 gånger! har 5 liv). Ska vi inte hindra honom från att fuska så här?

Låt oss titta på ett alternativt sätt att hantera placeringen av fartygen. Låt först användaren välja om han vill placera fartyget horisontellt eller vertikalt. Låt honom sedan mata in fartygets övre / vänstra koordinat. Vi kommer att beräkna de återstående poängen själva.

Här är hur den faktiska metodimplementeringen kan se ut:

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

Nu när vi bara placera fartygen direkt på tavlan så kan vi ta bort Position -klassen och alla dess referenser. Det betyder att vi inte längre vet om ett fartyg har sjunkit eller inte.

Medan jag ypade detta märkte jag att @Timothy Truckle redan publicerade ett svar också. Jag älskar verkligen hans lösning att använda dedikerade Field s istället för char ”s för att representera styrelsen.

så vår platsfartygsmetod ändras till:

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

På det sättet kan vi kontrollera om ett fartyg förstörs helt eller bara träffas delvis när vi attackerar en Field.

Nu istället för att fortsätta med det här svaret föreslår jag att du läser @Timothy också, och tittar på de bra poängen i båda våra svarare. Vid första anblicken är vi antingen överens om eller helt enkelt komplettera varandras svar. Så du borde ha några anständiga tips om hur du kan förbättra din kod 🙂

Kommentarer

• Tack så mycket för en detaljerad granskning! Alla gör solida poäng. Eftersom alla svaren var så bra,

Andra svar har påpekat nästan allt, så jag lägger bara till en sak. Det ser ut för mig att du använder undantag för att på något sätt utföra flödeskontroll. Undantag är inte kontrollflödesmekanismer .

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

Och sedan

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

Jag tycker att du bör verifiera att de givna koordinaterna finns i styrelsen innan du försöker skapa Position. Detta är användarinmatning och det är helt rimligt att göra det. Du validerar redan det ship.getSize() != Utils.distanceBetweenPoints(from, to). Du skulle till och med göra det i själva Board istället för att sedan ha positionskontroll för Constants.BOARD_SIZE.