Takk for å dele koden din.

Det som irriterer meg litt med denne koden er at jeg ikke bruker grensesnitt, abstrakte klasser eller arv,

Å gjøre OOP betyr at du følger visse prinsipper som er (blant andre):

• informasjon skjuler / innkapsling
• enkeltansvar
• separasjon av bekymringer
• KISS (Hold det enkelt (og) dumt.)
• TØRR (ikke gjenta deg selv.)
• «Fortell! Ikke spør.»
• Demeterloven («Ikke snakk med fremmede!»)

Grensesnitt, abs traktatklasser eller prinsipper for arvestøtte og bør brukes etter behov. De «definerer» ikke OOP.

IMHO hovedårsaken til at tilnærmingen din mislykkes OOP er at «modellen» er en matrise av en primitiv type char. Dette fører til slutt til en prosessuell tilnærming for spilllogikken.

Jeg vil tenke på et grensesnitt som dette:

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

der Result ville være et nummer:

 enum Result{ NO_HIT, PARTIAL_HIT, DESTROYED } 

Og jeg ville ha forskjellige implementeringer av grensesnittet:

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 skal være nok, håper du få ideen …

Formelle problemer

Navngivning

Å finne gode navn er den vanskeligste delen i programmering. Så ta deg alltid tid til å tenke på identifikatornavnene dine.

På den lyse siden følger du Java-navngivningskonvensjonene.

Men du bør ha metodenavnene dine med et verb i presens.Eg: shipWasHit() skal hete hit().
Eller distanceBetweenPoints() skal være calculateDistanceBetween(). Her avslører parametrene at avstanden er mellom punkter, så det er ikke nødvendig å sette det i metodens navn.

Vær ordentlig i variabelnavnene dine. i stedet for

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

bør disse variablene heller være navngitt slik:

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

Ta navnene dine fra problemdomenet, ikke fra den tekniske løsningen. f.eks .: SHIP_ICON skal bare være SHIP med mindre du har en annen konstant i Ship -klassen .

Kommentarer

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

kommentarer skal bare brukes på grensesnitt eller abstrakte metoder der de inneholder kontrakten som implementøren må oppfylle.

Konstanterklasse

Sett ting sammen som hører sammen. Definer konstanter i klassen som bruker dem.

Kommentarer

• Alle gyldige punkter, og veldig nyttige. Takk skal du ha!

Det er allerede noen gode svar, men jeg trodde jeg ville legge til noen av tingene som sto når jeg så igjennom koden.

For øyeblikket er den eneste inngangskilden brukerinngang fra en skanner. Dette vil gjøre det ganske vanskelig hvis du ønsker å legge til en slags datamaskinmotstandere til spill mot.

Det ser ut til at det er noen kode i Board-klassen som kanskje passer bedre i Player-klassen.

Spesielt innholdet av metoden placeShipsOnBoard ().

Denne metoden tar input fra brukeren og oppretter en posisjon. La oss prøve å omstrukturere denne slik at en spiller (menneske eller datamaskin) kan opprette en posisjon.

La oss lage en grensesnitt

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 hva med en grunnleggende dataspiller

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

Så, i hovedkortklassen, rogrammerer vi til spillergrensesnittet

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 all koden refererer til en spiller (nå et grensesnitt) i stedet for en konkret implementering, kan vi enkelt legge til nye typer dataspillere . f.eks. ny CheatingComputer (), ny HardComputer (), ny MediumComputer (). Hver enkelt ville bare bestemme hvor de skulle skyte neste og hvor de skulle plassere neste skip på en annen måte.

Og hvis vi hadde dette, kunne vi lage et spill med to dataspillere og la det spille selv! Spennende rett: D

En annen relatert ting som vi kan endre er at du alltid vil ha to menneskelige spillere i konstruktøren din for Game. Vi kan få dette til å ta en liste over spillere, så du kan ha et hvilket som helst antall spillere i spillet ditt. Bare fordi det virkelige spillet Battleship er begrenset til to spillere, betyr ikke det at ditt må være.

Vi kan tillate justerbar rutenettstørrelse og et vilkårlig spillernummer. Plutselig har vi et 100×100 rutenett med en 8 spiller gratis for alle!

(hvilket som helst beløp som kan være datamaskinstyrt).

Skipene dine initialiseres også i en statisk blokk i brettklassen din. Du har alle de virkelige skipene fra Battleship. Men igjen, hvorfor ikke tillate mer fleksibilitet her. Hva om skipet ditt besto av en liste over punkter? Du kan ha S-formede skip, de trenger ikke å være begrenset til horisontal vertikal justering. (Dette kan være over toppen, men jeg synes det er en kul ting å tenke på!)

Jeg vil fullføre med noen få små ting som så morsomme ut for meg

throw new ArrayIndexOutOfBoundsException(); 

fra posisjonsklassen. Dette virker som et upassende unntak å kaste her. Det er ingen matrise i Position-klassen, så dette må referere til matrisen i Board-klassen. Jeg tror en mer passende unntakstype ville være en IllegalArgumentException, ArrayIndexOutofBoundsException er en implementeringsdetalj (av en annen klasse!). Sørg også for å gi en passende feilmelding sammen med å kaste unntaket. F.eks. «verdien må være innenfor området x og y»

Linjen

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

kan ganske enkelt erstattes av

boolean isShipHit = ship != null; 

Det er ikke noe behov for den ternære operatøren her.

Bruk av targetHistory i Player-klassen mens (targetHistory.get (point)! = null)

Her bruker du et kart med det ene formål å se om et element er i det. Dette er akkurat hva et sett er for!

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

Kommentarer

• Tusen takk for innsikten! Alle disse svarene utfyller hvert andre virkelig bra! Jeg ‘ Jeg begynner å jobbe med versjon 2.0 med tanke på alt dette.
• Ikke noe problem jeg ‘ er glad for at du syntes det var nyttig! Lykke til med 2.0!

Hva irriterer meg litt med denne koden er at jeg ikke bruker grensesnitt, abstrakte klasser eller arv, men jeg kunne ikke finne et godt brukstilfelle for dem her.

Det er ikke noe galt med dette i spillet ditt. Spillkonseptet er så enkelt at du ikke trenger noe av det. Problemet med små lekeprogrammer er at du vanligvis ikke trenger å bruke store designløsninger. Du trenger bare å sørge for at du følger de vanlige SOLID-designprinsippene .

Nå som vi fikk det ryddet la la oss se på noen detaljer i koden din som bør forbedres.

Den første er ganske åpenbar. Ikke skriv kommentarer for å skrive kommentarer. Noen lærere liker å tvinge deg til å skrive en javadoc-kommentar til alt. Jeg er faktisk helt imot dette, bortsett fra når jeg skriver en slags verktøypakke som alle andre må bruke. Normalt sett skal koden være selvdokumenterende. Og koden din gjør det veldig bra. Det er bare å fjerne kommentaren som «s i utgangspunktet å gjenta det velvalgte navnet på variabelen / funksjonen / …

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 verdi legger kommentaren til til funksjonen?Jeg tror dette også er den eneste metoden jeg vil endre for å gjøre den mer lesbar. Fordi det ikke er åpenbart at posisjonen består av et from og to punkt som vi sjekker om point ligger mellom dem.

Hva om metoden hadde denne signaturen:

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

Ville ikke det gir litt mer mening?

Jeg bør legge til her at jeg ikke er imot kommentarer generelt. Men en kommentar skal forklare HVORFOR noe blir gjort på den måten. Ikke hvordan det blir implementert. Hvis jeg ville vet hvordan den ble implementert ville jeg bare sett på koden.

Det neste poenget er å ha den Util-klassen … I motsetning til noen purister har jeg ingenting imot begrepet Util-klasser Util-klasser kan være nyttige for å sette sammen lignende funksjoner. For eksempel java.lang.Math som grupperer alle vanlige regneoperasjoner sammen i en klasse.

Det som plager meg med Util-klassen din er at det egentlig ikke er en god grunn til at den eksisterer. De to funksjonene du har der, blir bare brukt i styret. Så de kunne «like godt vært private static hjelperfunksjoner i den klassen.

Vi kan faktisk til og med gjøre det litt bedre etter å ha endret signaturen til det Jeg foreslo tidligere. Hva om vi legger containsPoint(Position position, Point point) { inne i Position -klassen i stedet for å ha Position som parameter for metoden? Så kan vi bruke den slik:

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

Det passer veldig bra der, ikke sant?

Apropos klassen Positions. Jeg hadde en merkelig følelse av dette mens jeg så på koden din. Først trodde jeg at du ikke hadde en something[][] til å representere tavlen. Jeg trodde du hadde representert alt som poeng gjennom hele kodebasen. Dette kan fungere, men gjør det vanskelig å skrive ut tavlen. Og så la jeg merke til at jeg hadde en char[][] inne i Board -klassen. Men ville det ikke være mer fornuftig å sette skipene inn i det det rutenettet uten å ha en mellomliggende posisjonsklasse?

Jeg la også merke til en annen farlig ting ved placeShipsOnBoard(). Skal du egentlig bare stole på at brukeren din oppgir 2 gyldige koordinater? Hva om brukeren prøver å være morsom og legger inn fra = (1,1) til = (2,2)? Bør vi tillate dette? Eller hva om du vil at han skal legge inn et skip med lengde 5 og han legger inn fra = (1,1) til = (1,1), i hovedsak krymper skipet til en enkelt firkant (at du må treffe 5 ganger! Siden skipet har 5 liv). Skal vi ikke hindre ham i å jukse slik?

La oss se på en alternativ måte å håndtere plassering av skipene på. La brukeren først velge om han vil plassere skipet horisontalt eller vertikalt. La ham deretter legge inn topp / venstre koordinat på skipet. Vi beregner de resterende punktene selv.

Her er hvordan den faktiske implementeringen av metoden 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; } } 

Nå som vi bare plasser skipene direkte på tavlen, vi kan fjerne Position klassen og alle dens referanser. Dette betyr at vi ikke lenger vet om et skip har sunket eller ikke.

Mens jeg ypte dette, la jeg merke til at @Timothy Truckle allerede la ut et svar også. Jeg elsker virkelig løsningen hans på å bruke dedikerte Field s i stedet for char «s til å representere styret.

så vår stedskipsmetode endres 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åten kan vi sjekke om et skip blir ødelagt helt eller bare treffer delvis når vi angriper et Field.

Nå i stedet for å fortsette med dette svaret, foreslår jeg at du leser @Timothy også, og ser på de gode punktene i begge våre svarere. Ved første øyekast er vi enten enige om eller bare utfyller hverandres svar. Så du bør ha noen anstendige tips om hvordan du kan forbedre koden din 🙂

Kommentarer

• Tusen takk for en detaljert gjennomgang! Alle kommer med solide poeng. Siden alle svarene var så gode, vil jeg ‘ gi merket til personen som svarte først.
• ahem » Det er ikke noe galt i dette i spillet ditt. Spillkonseptet er så enkelt at du ikke ‘ t trenger noen av disse. » – Jeg må være uenig: Det ‘ er bedre å øve hva som helst i enkel modus først. Hvis du kan ‘ t bruke konsepter og mønstre på noe enkelt, kan du sikkert ‘ t bruke dem på noe mer komplekst.
• Det er spesielt viktig med delen om kondomene. Redundante kommentarer er et fullstendig bortkastet tid og energi til å det, vedlikeholde og verifisere. Kommenter bare hvis du ikke kan få koden og funksjonene til å forklare seg.

Andre svar har påpekt nesten alt, så jeg legger bare til en ting. Det ser ut for meg at du bruker unntak for å utføre strømningskontroll på en eller annen måte. Unntak er ikke kontrollflytmekanismer .

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 deretter

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

Jeg tror du bør validere at de oppgitte koordinatene er inne i brettet før du prøver å opprette Position. Dette er brukerinngang og er helt rimelig å gjøre det. Du validerer allerede det ship.getSize() != Utils.distanceBetweenPoints(from, to). Du ville til og med gjøre det i Board selve objektet, i stedet for da å ha posisjonssjekk for Constants.BOARD_SIZE.