Tak til deling af din kode.

Hvad irriterer mig lidt med denne kode er, at jeg ikke bruger grænseflader, abstrakte klasser eller arv,

At udføre OOP betyder, at du følger visse principper, som er (blandt andre):

• information skjuler / indkapsling
• enkelt ansvar
• adskillelse af bekymringer
• KISS (Hold det simpelt (og) dumt).
• TØR (gentag dig ikke.)
• “Sig det! Spørg ikke.”
• Demeterloven (“Tal ikke med fremmede!”)

Grænseflader, abs traktatklasser eller arvestøttehatteprincipper og bør bruges efter behov. De “definerer” ikke OOP.

IMHO hovedårsagen til, at din tilgang mislykkes OOP er, at din “Model” er en matrix af en primitiv type char. Dette fører i sidste ende til en proceduremæssig tilgang til spillogikken.

Jeg tænker på en grænseflade som denne:

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

hvor Result ville være et nummer:

 enum Result{ NO_HIT, PARTIAL_HIT, DESTROYED } 

Og jeg ville have forskellige implementeringer af grænsefladen:

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

Dette skulle være nok, håber du få ideen …

Formelle problemer

Navngivning

At finde gode navne er den sværeste del i programmeringen. Så tag dig altid tid til at tænke over dine identifikationsnavne.

På den lyse side følger du Java-navngivningskonventionerne.

Men du skal have dine metodenavne til at starte med et verb i nutid.Eg: shipWasHit() skal have navnet hit().
Eller distanceBetweenPoints() skal være calculateDistanceBetween(). Her afslører parametrene, at afstanden er mellem punkter, så det er ikke nødvendigt at sætte det i metodens navn.

Vær omhyggelig i dine variabelnavne. i stedet for

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

bør disse variabler snarere være navngivet som denne:

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

Tag dine navne fra problemdomænet, ikke fra den tekniske løsning. f.eks .: SHIP_ICON skal kun være SHIP, medmindre du har en anden konstant inden for Ship -klassen .

Kommentarer

Kommentarer skal forklare hvorfor koden er som den er . Fjern alle andre kommentarer.

kommentarer skal kun bruges på interface eller abstrakte metoder, hvor de indeholder den kontrakt som implementereren skal opfylde.

Konstanter klasse

Sæt ting sammen, der hører sammen. Definer konstanter i klassen, der bruger dem.

Kommentarer

• Alle gyldige punkter og virkelig nyttige. Tak skal du have!

Der er allerede nogle gode svar, men jeg troede, jeg ville tilføje nogle af de ting, der stod ud til mig, da jeg kiggede igennem koden.

I øjeblikket er den eneste inputkilde brugerinput fra en scanner. Dette ville gøre det ret vanskeligt, hvis du ville tilføje en slags computermodstandere til spil mod.

Det ser ud til, at der er en kode i Board-klassen, der måske er bedre egnet i Player-klassen.

Specifikt indholdet af metoden placeShipsOnBoard ().

Denne metode tager input fra brugeren og opretter en position. Lad os prøve at omstrukturere dette, så en spiller (menneske eller computer) kan oprette en position.

Lad os lave en interface

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

Vi har allerede implementeringer fra et menneske,

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

Og hvad med en grundlæggende computerafspiller

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

Derefter programmerer vi i hovedkortklassen til afspillergrænsefladen

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

Hvis hele koden henviser til en spiller (nu en grænseflade) snarere end en konkret implementering, kan vi nemt tilføje nye typer computerafspillere . for eksempel. ny CheatingComputer (), ny HardComputer (), ny MediumComputer (). Hver enkelt ville bare bestemme, hvor de skulle skyde næste, og hvor de skulle placere det næste skib på en anden måde.

Og hvis vi havde dette, kunne vi lave et spil med 2 computerafspillere og lade det spille selv! Spændende til højre: D

En anden relateret ting, som vi kunne ændre, er at i din konstruktør til spil vil du altid have 2 menneskelige spillere. Vi kan få dette til at tage en liste over spillere, så du kan have et hvilket som helst antal spillere i dit spil. Bare fordi det rigtige spil Slagskib er begrænset til 2 spillere, betyder det ikke, at dit skal være.

Vi kunne muligvis give mulighed for justerbar gitterstørrelse og et vilkårligt spillernummer. Pludselig har vi et 100×100 gitter med en 8 spiller gratis for alle!

(ethvert beløb, der kunne styres computer).

Dine skibe initialiseres også i en statisk blok i din bordklasse. Du har alle de rigtige skibe fra slagskib. Men igen, hvorfor ikke tillade mere fleksibilitet her. Hvad hvis dit skib bestod af en liste over punkter? Du kunne have S-formede skibe, de behøvede ikke at være begrænset til vandret lodret justering. (Dette kan være over toppen, men jeg synes, det er en sej ting at tænke på!)

Jeg slutter med et par små ting, der så sjovt ud for mig

throw new ArrayIndexOutOfBoundsException(); 

fra positionsklassen. Dette virker som en upassende undtagelse at kaste her. Der er ingen matrix i positionsklassen, så dette må referere til arrayet i Board-klassen. Jeg tror, at en mere passende undtagelsestype ville være en IllegalArgumentException, ArrayIndexOutofBoundsException er en implementeringsdetalje (af en anden klasse!). Sørg også for at give en passende fejlmeddelelse sammen med at kaste undtagelsen. F.eks. skal “værdien være inden for området x og y”

Linjen

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

kunne simpelthen erstattes af

boolean isShipHit = ship != null; 

Der er ikke behov for den ternære operatør her.

Brug af targetHistory i Player-klassen mens (targetHistory.get (point)! = null)

Her bruger du et kort med det ene formål at se om et element er i det. Dette er præcis hvad et sæt er for!

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

Kommentarer

• Mange tak for indsigten! Alle disse svar supplerer hver andre rigtig godt! Jeg ‘ Jeg begynder at arbejde på version 2.0 med alt dette i tankerne.
• Intet problem Jeg ‘ er glad for, at du fandt det nyttigt! Held og lykke med 2.0!

Hvad irriterer mig lidt med denne kode er, at jeg ikke bruger grænseflader, abstrakte klasser eller arv, men jeg kunne ikke finde en god brugssag for dem her.

Der er ikke noget galt med dette i dit spil. Spilkonceptet er så simpelt, at du ikke har brug for nogen af dem. Problemet med små legetøjsprogrammer er, at du normalt ikke behøver at anvende store designløsninger. Du skal bare sørge for at følge de sædvanlige SOLID-designprincipper .

Nu hvor vi fik det ryddet op lad os se på nogle detaljer i din kode, der skal forbedres.

Den første er ret åbenlyst. Skriv ikke kommentarer for at skrive kommentarer. Nogle lærere kan lide at tvinge dig til at skrive en javadoc-kommentar til alt. Jeg er faktisk fuldstændig imod dette, undtagen når jeg skriver en slags hjælpepakke, som alle andre skal bruge. Normalt skal koden være selvdokumenterende. Og din kode gør et rigtig godt stykke arbejde med det. Så fjern bare kommentaren, som “s gentager dybest set det velvalgte navn på variablen / funktionen / …

For eksempel:

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

Hvilken værdi tilføjer den kommentar til funktionen?Jeg tror, det er også den eneste metode, som jeg ville ændre for at gøre det mere læsbart. Fordi det ikke er indlysende, at positionen består af et from og to punkt, som vi kontrollerer, om vores point ligger mellem dem.

Hvad hvis metoden havde denne signatur:

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

Ville det ikke giver lidt mere mening?

Jeg skal tilføje her, at jeg dog ikke er imod kommentarer generelt. Men en kommentar skal forklare HVORFOR der gøres noget på den måde. Ikke hvordan det implementeres. Hvis jeg ville ved hvordan det blev implementeret ville jeg bare have kigget på koden.

Det næste punkt er at have den Util-klasse … I modsætning til nogle purister har jeg intet imod begrebet Util-klasser Util-klasser kan være nyttige til at sætte lignende funktioner sammen. For eksempel java.lang.Math, der grupperer alle de sædvanlige aritmetiske operationer i en klasse.

Den ting, der generer mig med din Util-klasse er, at der ikke virkelig er en god grund til, at den eksisterer. De 2 funktioner, du har derinde, bruges kun nogensinde inden for Board-klassen. Så de kunne “lige så godt have været private static hjælperfunktioner inden for den klasse.

Faktisk kan vi endda gøre det lidt bedre efter at have ændret signaturen til hvad Jeg foreslog tidligere. Hvad hvis vi lægger containsPoint(Position position, Point point) { inde i Position klassen i stedet for at have Position som parameter for metoden? Så kan vi bruge den således:

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

Det passer rigtig godt der, gør det ikke?

Apropos klassen Positions. Jeg havde en underlig fornemmelse af dette, mens jeg kiggede igennem din kode. Først troede jeg, at du ikke havde en something[][] til at repræsentere tavlen. Jeg troede, du havde repræsenteret alt som point gennem hele kodebasen. Dette kunne fungere, men gør udskrivning af tavlen akavet. Og så bemærkede jeg, at der var en char[][] inde i din Board klasse. Men ville det ikke give mere mening at lægge skibene inde det gitter uden at have en mellemliggende positionsklasse?

Jeg bemærkede også en anden farlig ting ved placeShipsOnBoard(). Skal du virkelig bare stole på din bruger til at indtaste 2 gyldige koordinater? Hvad hvis brugeren prøver at være morsom og input fra = (1,1) til = (2,2)? Skal vi tillade dette? Eller hvad hvis du vil have ham til at indtaste et skib med længde 5, og han indtaster fra = (1,1) til = (1,1), der i det væsentlige krymper skibet til en enkelt firkant (at du skal ramme 5 gange! Siden skibet har 5 liv). Skal vi ikke forhindre ham i at snyde sådan?

Lad os se på en alternativ måde at håndtere placeringen af skibene på. Lad brugeren først vælge, om han vil placere skibet vandret eller lodret. Lad ham derefter indtaste skibets øverste / venstre koordinat. Vi beregner selv de resterende point.

Her ser den aktuelle metodeimplementering ud:

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 hvor vi bare placer skibene direkte på tavlen, vi kan fjerne Position klassen og alle dens referencer. Dette betyder, at vi ikke længere ved, om et skib er sunket eller ej.

Mens jeg yp dette, bemærkede jeg, at @Timothy Truckle allerede også sendte et svar. Jeg elsker virkelig hans løsning med at bruge dedikerede Field s i stedet for char “s til at repræsentere tavlen.

så vores stedskibsmetode ændres til:

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å den måde kan vi kontrollere, om et skib ødelægges helt eller bare rammer delvist, når vi angriber et Field.

Nu i stedet for at fortsætte med dette svar, foreslår jeg, at du også læser @Timothy, og ser på de gode punkter i begge vores svarere. Ved første øjekast er vi enten enige om eller blot supplerer hinandens svar. Så du skal have nogle anstændige tip til, hvordan du forbedrer din kode 🙂

Kommentarer

• Mange tak for en detaljeret gennemgang! Alle fremhæver solide punkter. Da alle svarene var så gode, giver jeg ‘ fluebenet til den person, der svarede først.
• ahem ” Der er ikke noget galt med dette i dit spil. Spilkonceptet er så simpelt, at du ikke ‘ behøver ikke nogen af disse. ” – Jeg må være uenig: Det ‘ er bedre at øve hvad som helst i let tilstand først. Hvis du kan ‘ ikke anvende begreber og mønstre på noget simpelt, kan du helt sikkert ‘ t anvende dem på noget mere komplekst.
• Det er specielt vigtigt med delen om conmenterne. Redundante kommentarer er et fuldstændigt spild af tid og energi til at skrive ite, vedligeholde og kontrollere. Kommenter kun, hvis du ikke kan få koden og funktionerne til at forklare sig selv.

Andre svar har påpeget næsten alt, så jeg tilføjer bare en ting. Det ser ud for mig at du bruger undtagelser til på en eller anden måde at udføre flowkontrol. Undtagelser er ikke kontrolflowmekanismer .

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

Og derefter

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

Jeg synes, du skal validere, at de givne koordinater er inde i tavlen, før du prøver at oprette Position. Dette er brugerinput og er helt rimeligt at gøre det. Du validerer allerede det ship.getSize() != Utils.distanceBetweenPoints(from, to). Du ville endda gøre det i selve Board i stedet for derefter at have Positionskontrol for Constants.BOARD_SIZE.