Bedankt voor het delen van je code.

Wat me een beetje irriteert aan deze code is dat ik geen interfaces, abstracte klassen of overerving gebruik,

OOP doen betekent dat je bepaalde principes volgt die (onder andere) zijn:

• informatie verbergen / inkapselen
• enkele verantwoordelijkheid
• scheiding van zorgen
• KUS (houd het simpel (en) dom.)
• DROOG (herhaal jezelf niet.)
• “Zeg het niet! Vraag het niet.”
• Law of demeter (“Dont talk with vreemden!”)

Interfaces, abs tract-klassen, of overervingsondersteunende hatprincipes en moeten indien nodig worden gebruikt. Ze “definiëren” OOP niet.

IMHO de belangrijkste reden waarom uw aanpak mislukt. OOP is dat uw “Model” een array van een primitief type is char. Dit leidt uiteindelijk tot een procedurele benadering voor de spellogica.

Ik zou een interface als deze kunnen bedenken:

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

waarbij Result zou een opsomming zijn:

 enum Result{ NO_HIT, PARTIAL_HIT, DESTROYED } 

En ik zou verschillende implementaties van de interface hebben:

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

Dit zou genoeg moeten zijn, ik hoop dat je begrijp het idee …

Formele kwesties

Naamgeving

Goede namen vinden is het moeilijkste deel bij het programmeren. Neem dus altijd de tijd om na te denken over uw identificatienamen.

Aan de andere kant volg je de Java-naamgevingsconventies.

Maar je moet de namen van je methoden laten beginnen met een werkwoord in zijn tegenwoordige tijd.Eg: shipWasHit() zou hit() moeten heten.
Of distanceBetweenPoints() zou moeten wees calculateDistanceBetween(). Hier laten de parameters zien dat de afstand tussen punten ligt, dus dat hoeft niet in de methode naam.

Wees uitgebreid in uw variabelenamen. in plaats van

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

deze variabelen zouden eerder moeten zijn met de volgende naam:

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

Haal je namen uit het probleemdomein, niet uit de technische oplossing. bijv .: SHIP_ICON zou SHIP moeten zijn, tenzij je een andere constante hebt binnen de Ship klasse .

Reacties

Reacties zouden moeten uitleggen waarom de code is zoals hij is . Verwijder alle andere opmerkingen.

opmerkingen mogen alleen worden gebruikt op interface- of abstracte methoden waar ze het contract bevatten waaraan de implementator moet voldoen.

Constants class

Zet dingen bij elkaar die bij elkaar horen. Definieer constanten in de klasse die ze gebruikt.

Opmerkingen

• Allemaal geldige punten, en erg nuttig. Dank je!

Er zijn al een paar goede antwoorden, maar ik dacht dat ik “enkele dingen zou toevoegen die stonden naar me toe toen ik door de code keek.

Op dit moment is de enige bron van invoer de gebruikersinvoer van een scanner. Dit zou het behoorlijk moeilijk maken als je een soort computertegenstanders zou willen toevoegen aan spelen tegen.

Het lijkt erop dat er een code in de Board-klasse zit die beter geschikt is voor de Player-klasse.

Specifiek de inhoud van de placeShipsOnBoard () – methode.

Deze methode vergt input van de gebruiker en creëert een positie. Laten we proberen dit te herstructureren zodat een speler (mens of computer) een positie kan creëren.

Laten we een interface

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

We hebben al implementaties van een mens,

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

En hoe zit het met een eenvoudige computerspeler

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

Vervolgens, in de hoofdbordklasse, programmeren we naar de spelerinterface

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

Als alle code verwijst naar een speler (nu een interface) in plaats van een concrete implementatie, kunnen we gemakkelijk nieuwe soorten computerspelers toevoegen . bijv. nieuwe CheatingComputer (), nieuwe HardComputer (), nieuwe MediumComputer (). Elk van hen zou gewoon bepalen waar ze vervolgens zouden vuren en waar ze het volgende schip op een andere manier zouden plaatsen.

En als we dit hadden, zouden we een spel kunnen maken met 2 computerspelers en het zelf laten spelen! Opwindend toch: D

Een ander gerelateerd ding dat we zouden kunnen veranderen, is dat je in je constructor voor Game altijd 2 menselijke spelers zult hebben. We kunnen hiervoor een lijst met spelers maken, zodat u een willekeurig aantal spelers in uw spel kunt hebben. Alleen omdat het echte spel Battleship beperkt is tot 2 spelers, wil dat nog niet zeggen dat die van jou dat ook moet zijn.

We kunnen instelbare rastergrootte en een willekeurig spelersnummer toestaan. Plots hebben we een raster van 100×100 met een 8 speler gratis voor iedereen!

(elk aantal kan computergestuurd zijn).

Je schepen worden ook geïnitialiseerd in een statisch blok in je bordklasse. Je hebt alle echte schepen van Battleship. Maar nogmaals, waarom zou je hier niet meer flexibiliteit toestaan. Wat als je schip uit een lijst met punten zou bestaan? Je zou S-vormige schepen kunnen hebben, die zouden zich niet hoeven te beperken tot horizontaal of verticaal uitlijnen. (Dit is misschien overdreven, maar ik denk dat het “cool is om over na te denken!)

Ik zal eindigen met een paar kleine dingen die er grappig uitzagen

throw new ArrayIndexOutOfBoundsException(); 

uit de klasse Position. Dit lijkt een ongepaste uitzondering om hier te gooien. Er is geen array in de klasse Position, dus dit moet verwijzen naar de array in de klasse Board. Ik denk dat een geschikter Exception-type een IllegalArgumentException zou zijn, ArrayIndexOutofBoundsException is een implementatiedetail (van een andere klasse!). zorg er ook voor dat u een passend foutbericht opgeeft naast het gooien van de uitzondering. bijv. “waarde moet binnen bereik x en y zijn”

De regel

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

kan eenvoudig worden vervangen door

boolean isShipHit = ship != null; 

Er is hier geen ternaire operator nodig.

Het gebruik van targetHistory in de Player-klasse while (targetHistory.get (point)! = null)

Hier gebruik je een kaart met als enig doel om te zien of er een element in zit. Dit is precies wat een set is voor!

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

Reacties

• Heel erg bedankt voor het inzicht! Al deze antwoorden vullen elk andere echt goed! Ik ‘ ga aan de slag met versie 2.0 met dit alles in gedachten.
• Geen probleem. Ik ‘ ben blij dat je het nuttig vond! Veel succes met 2.0!

Wat irriteert ik een klein beetje met deze code is dat ik geen interfaces, abstracte klassen of overerving gebruik, maar ik kon hier geen goede use case voor hen vinden.

Er is niets mis mee in je spel. Het spelconcept is zo eenvoudig dat je die niet nodig hebt. Het probleem met kleine speelgoedprogrammas is dat je meestal geen grote ontwerpoplossingen hoeft toe te passen. U hoeft er alleen voor te zorgen dat u de gebruikelijke SOLID ontwerpprincipes volgt.

Nu we dat hebben opgehelderd, laten we kijk eens naar enkele details van je code die verbeterd zouden moeten worden.

De eerste is vrij duidelijk. Schrijf geen commentaren om commentaar te schrijven. Sommige docenten dwingen je graag om overal een javadoc-commentaar op te schrijven. Ik ben hier eigenlijk helemaal tegen, behalve wanneer ik een soort hulpprogramma-pakket schrijf dat iedereen moet gebruiken. Normaal gesproken zou code zelfdocumenterend moeten zijn. En je code doet dat heel goed. Dus verwijder gewoon de opmerking die in feite de goedgekozen naam van de variabele / functie / … herhalen.

Bijvoorbeeld:

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

Welke waarde voegt die opmerking toe naar de functie?Ik denk dat dit ook de enige methode is die ik zou veranderen om het leesbaarder te maken. Omdat het niet duidelijk is dat de positie bestaat uit een from en to punt waarvoor we controleren of ons point ligt tussen hen in.

Wat als de methode deze handtekening had:

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

Zou dat niet iets logischer?

Ik moet hier toevoegen dat ik “niet tegen opmerkingen in het algemeen ben. Maar een opmerking moet uitleggen WAAROM iets op die manier wordt gedaan. Niet hoe het wordt geïmplementeerd. Als ik dat zou willen weten hoe het geïmplementeerd is, zou ik net naar de code hebben gekeken.

Het volgende punt is dat je die Util-klasse hebt … In tegenstelling tot sommige puristen heb ik niets tegen het concept van Util-klassen . Util-klassen kunnen handig zijn om vergelijkbare functies samen te voegen. Bijvoorbeeld java.lang.Math die alle gebruikelijke rekenkundige bewerkingen in één klasse groepeert.

Wat mij zorgen baart met je Util-klasse is dat er niet echt een goede reden is dat het bestaat. De 2 functies die je daar hebt, worden alleen binnen de Board-klasse gebruikt. Dus ze hadden “net zo goed private static helperfuncties binnen die klasse kunnen zijn.

In feite kunnen we het zelfs een beetje beter doen nadat we de handtekening hebben veranderd in wat Stelde ik eerder voor. Wat als we containsPoint(Position position, Point point) { in de Position -klasse plaatsen in plaats van de Position als parameter voor de methode? Dan kunnen we het als volgt gebruiken:

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

Het past daar echt goed, nietwaar?

Over de klasse Positions gesproken. Ik had hier een raar gevoel over toen ik je code doorzocht. Eerst dacht ik dat je geen something[][] had om het bord te vertegenwoordigen. Ik dacht dat je alles in de hele codebase als punten had voorgesteld. Dit zou kunnen werken, maar het maakt het afdrukken van het bord lastig. En toen merkte ik dat er een char[][] in uw Board -klasse zit. Maar dan zou het niet logischer zijn om de schepen onmiddellijk in dat raster zonder een tussenliggende Position-klasse te hebben?

Ik heb ook een ander gevaarlijk ding opgemerkt over de placeShipsOnBoard(). Moet u uw gebruiker echt vertrouwen om 2 geldige coördinaten in te voeren? Wat als de gebruiker grappig probeert te zijn en invoert van = (1,1) tot = (2,2)? Moeten we dit toestaan? Of wat als je wilt dat hij een schip met een lengte van 5 invoert en hij invoert van = (1,1) tot = (1,1), waardoor het schip in wezen wordt verkleind tot een enkel vierkant (dat je 5 keer moet raken! Sinds het schip heeft 5 levens). Zouden we niet moeten voorkomen dat hij op deze manier vals speelt?

Laten we eens kijken naar een alternatieve manier om het plaatsen van de schepen te behandelen. Laat de gebruiker eerst kiezen of hij het schip horizontaal of verticaal wil plaatsen. Laat hem dan de coördinaten boven / links van het schip invoeren. We zullen de resterende punten zelf berekenen.

Hier is hoe de daadwerkelijke implementatie van de methode eruit zou kunnen zien:

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 we zojuist plaats de schepen direct op het bord kunnen we de Position class en al zijn referenties verwijderen. Dit betekent wel dat we niet meer weten of een schip is gezonken of niet.

Terwijl ik dit deed, merkte ik dat @Timothy Truckle ook al een antwoord had gepost. Ik hou echt van zijn oplossing om toegewijde Field s te gebruiken in plaats van char “s om het bord te vertegenwoordigen.

dus onze plaats-scheepsmethode verandert 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); } } 

Op die manier kunnen we controleren of een schip volledig is vernietigd of slechts gedeeltelijk is geraakt bij het aanvallen van een Field.

In plaats van verder te gaan met dit antwoord, raad ik je aan om ook @Timothy” s te lezen en naar de goede punten in beide antwoorden te kijken. Op het eerste gezicht zijn we het eens over of vullen we elkaars antwoord simpelweg aan. Dus je zou een paar goede tips moeten hebben om je code te verbeteren 🙂

Opmerkingen

• Hartelijk dank voor een gedetailleerde beoordeling! Jullie hebben allemaal solide punten. Omdat alle antwoorden zo goed waren, geef ik ‘ het vinkje aan de persoon die heeft geantwoord eerst.
• ahem ” Hier is niets mis mee in je spel. Het spelconcept is zo eenvoudig dat je ‘ die heb je niet nodig. ” – Ik ben het er niet mee eens: ‘ is beter om te oefenen alles eerst in de eenvoudige modus. Als je ‘ geen concepten en patronen kunt toepassen op iets eenvoudigs, kun je ‘ pas ze toe op iets complexers.
• Vooral het gedeelte over de conments is belangrijk. Overbodige opmerkingen zijn een totale verspilling van tijd en energie ite, onderhouden en verifiëren. Geef alleen commentaar als u de code en functies niet kunt krijgen om zichzelf uit te leggen.

Andere antwoorden hebben op bijna alles gewezen, dus ik zal slechts één ding toevoegen. voor mij dat je uitzonderingen gebruikt om op de een of andere manier flow control uit te voeren. Uitzonderingen zijn geen controlestroommechanismen .

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

En dan

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

Ik denk dat je moet controleren of de opgegeven coördinaten in het bord staan voordat je probeert de Position te maken. Dit is gebruikersinvoer en het is volkomen redelijk om het te doen. Je “valideert dat ship.getSize() != Utils.distanceBetweenPoints(from, to) al. Je zou dat zelfs doen in het Board -object zelf, in plaats van dat Positie wordt gecontroleerd op Constants.BOARD_SIZE.