Díky za sdílení kódu.

S tímto kódem mě trochu štve, že nepoužívám rozhraní, abstraktní třídy ani dědičnost,

Provádění OOP znamená, že dodržujete určité zásady, které jsou (mimo jiné):

• skrývání / zapouzdření informací
• jednotná odpovědnost
• oddělení obav
• KISS (udržujte to jednoduché (a) hloupé.)
• DRY (neopakujte se.)
• „Tell! Don“ t ask. “
• Law of demeter („ Don “t talk to strangers!“)

Rozhraní, abs třídy traktu nebo dědičnost podporují principy klobouku a měly by být použity podle potřeby. Nedefinují „OOP“.

IMHO hlavním důvodem, proč váš přístup selže, je to, že váš „Model“ je pole primitivního typu char. To v konečném důsledku vede k procedurálnímu přístupu k logice hry.

Myslel bych na takové rozhraní:

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

where Result by bylo enum:

 enum Result{ NO_HIT, PARTIAL_HIT, DESTROYED } 

A měl bych různé implementace rozhraní:

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

To by mělo stačit, doufám, že získejte nápad …

Formální problémy

Pojmenování

Hledání dobrých jmen je nejtěžší částí programování. Vždy si tedy udělejte čas a přemýšlejte o svých identifikátorových jménech.

Na druhé straně se budete řídit konvencemi pojmenování Java.

Měli byste ale mít názvy metod začínat slovesem v jeho present tense.Eg: shipWasHit() by měl být pojmenován hit().
Nebo distanceBetweenPoints() by měl být be calculateDistanceBetween(). Zde parametry odhalují, že vzdálenost je mezi body, takže to není nutné uvádět v názvu metody.

Názvy proměnných uveďte podrobně. namísto

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

by tyto proměnné měly být spíše pojmenovaný takto:

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

Vezměte svá jména z problémové domény, nikoli z technického řešení. např .: SHIP_ICON by měl být SHIP, pouze pokud ve třídě Ship máte jinou konstantu .

Komentáře

Komentáře by měly vysvětlovat proč je kód takový, jaký je . Odeberte všechny ostatní komentáře.

Komentáře by se měly používat pouze na rozhraní nebo abstraktních metodách, pokud obsahují smlouvu , kterou musí implementátor splnit.

Třída konstant

Spojte věci, které k sobě patří. Definujte konstanty ve třídě, která je používá.

Komentáře

• Všechny platné body a opravdu užitečné. Děkuji!

Existuje již několik dobrých odpovědí, ale myslel jsem si, že přidám některé věci, které stály když jsem si prohlédl kód.

V tuto chvíli je jediným zdrojem vstupu uživatelský vstup ze skeneru. To by docela ztěžovalo, kdybyste chtěli přidat do počítače nějaký druh počítačových oponentů. hrajte proti.

Vypadá to, že ve třídě Board je nějaký kód, který by se ve třídě Player mohl lépe hodit.

Konkrétně obsah metody placeShipsOnBoard ().

Tato metoda přebírá vstup od uživatele a vytváří pozici. Zkusme to restrukturalizovat tak, aby hráč (člověk nebo počítač) mohl vytvořit pozici.

Pojďme vytvořit rozhraní

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

Již máme implementace od člověka,

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

A co takhle základní počítačový hráč

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

Potom v hlavní třídě desky naprogramujeme na rozhraní přehrávače

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

Pokud se celý kód týká spíše přehrávače (nyní rozhraní) než konkrétní implementace, můžeme snadno přidat nové typy počítačových přehrávačů . např. nový CheatingComputer (), nový HardComputer (), nový MediumComputer (). Každý z nich by jen určil, kde bude pálit dál a kam umístit další loď jiným způsobem.

A pokud bychom to měli, mohli bychom vytvořit hru se 2 počítačovými hráči a nechat ji hrát sama! Vzrušující právo: D

Další související věc, kterou bychom mohli změnit, je, že ve vašem konstruktoru pro hru budete mít vždy 2 lidské hráče. Mohli bychom to přimět k převzetí seznamu hráčů, takže ve hře můžete mít libovolný počet hráčů. Jen proto, že skutečná bitevní loď je omezena na 2 hráče, neznamená to, že vaše musí být.

Mohli bychom umožnit nastavitelnou velikost mřížky a libovolné číslo hráče. Najednou máme mřížku 100×100 s 8 hráč zdarma pro všechny!

(libovolné množství lze ovládat počítačem).

Vaše lodě jsou také inicializovány ve statickém bloku ve vaší palubní třídě. Máte všechny skutečné lodě od bitevní lodi. Ale opět zde povolte větší flexibilitu. Co kdyby se vaše loď skládala ze seznamu bodů? Mohli byste mít lodě ve tvaru písmene S, nemuseli by se omezovat na vodorovnou nebo svislou orientaci. (Může to být nahoře, ale myslím, že je to skvělá věc na přemýšlení!)

Dokončím pár drobností, které mi připadaly vtipné

throw new ArrayIndexOutOfBoundsException(); 

ze třídy Pozice. Zdá se to jako nevhodná výjimka, kterou sem hodit. Ve třídě Position není žádné pole, takže to musí odkazovat na pole ve třídě Board. Myslím, že vhodnějším typem výjimky by byla IllegalArgumentException, ArrayIndexOutofBoundsException je implementační detail (jiné třídy!). Měli byste nezapomeňte také uvést příslušnou chybovou zprávu spolu s vyvoláním výjimky. např. „hodnota musí být v rozsahu x a y“

Řádek

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

lze jednoduše nahradit

boolean isShipHit = ship != null; 

Ternární operátor zde není potřeba.

Použití targetHistory ve třídě Player while (targetHistory.get (point)! = null)

Zde používáte mapu pouze za účelem zjištění, zda je v ní prvek. To je přesně to, čím je sada pro!

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

Komentáře

• Moc děkuji za pochopení! Všechny tyto odpovědi doplňují každý ostatní opravdu dobře! Začnu pracovat ‚ na verzi 2.0 s ohledem na toto vše.
• Žádný problém Jsem ‚ rád, že vám to připadá užitečné! Hodně štěstí v 2.0!

Co to štve trochu s tímto kódem nepoužívám rozhraní, abstraktní třídy ani dědičnost, ale zde pro ně nemohu najít dobrý případ použití.

Na vaší hře není nic špatného. Herní koncept je tak jednoduchý, že žádné z nich nepotřebujete. Problém s malými hračkovými programy spočívá v tom, že obvykle nemusíte používat velká designová řešení. Musíte se jen ujistit, že dodržujete obvyklé SOLID designové principy .

Teď, když jsme to vyjasnili, pojďme podívejte se na některé podrobnosti svého kódu, které by měly být vylepšeny.

První je docela zřejmý. Nepište komentáře kvůli psaní komentářů. Někteří učitelé vás rádi nutí, abyste ke všemu napsali komentář javadoc. Vlastně jsem úplně proti tomu, s výjimkou psaní nějakého balíčku obslužných programů, který musí používat všichni ostatní. Normálně mluvící kód by měl být samodokumentující. A váš kód v tom dělá opravdu dobrou práci. Takže stačí odstranit komentář, v podstatě opakování dobře zvoleného názvu proměnné / funkce / …

Například:

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

Jakou hodnotu přidává tento komentář do funkce?Myslím, že je to také jediná metoda, kterou bych změnil, aby byla čitelnější. Protože není zřejmé, že pozice je tvořena from a to bodem, u kterého zkontrolujeme, zda náš point leží mezi nimi.

Co když by metoda měla tento podpis:

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

To by nebylo dávají trochu větší smysl?

Měl bych sem dodat, že nejsem obecně proti komentářům. Ale komentář by měl vysvětlit, PROČ se něco dělá tímto způsobem. Ne jak je to implementováno. Kdybych chtěl vím, jak je implementováno, jen bych se podíval na kód.

Dalším bodem je použití této třídy Util … Na rozdíl od některých puristů nemám nic proti konceptu tříd Util . Třídy nástrojů mohou být užitečné pro sestavení podobných funkcí. Například java.lang.Math, který seskupuje všechny obvyklé aritmetické operace do jedné třídy.

Věc, která mě trápí s vaší třídou Util je to, že pro její existenci opravdu není dobrý důvod. 2 funkce, které tam máte, se používají pouze ve třídě Board. Mohli tedy „být stejně dobře private static pomocnými funkcemi uvnitř této třídy.

Ve skutečnosti si můžeme po změně podpisu na to, co děláme, dokonce udělat trochu lépe Navrhl jsem dříve. Co kdybychom místo třídy Position div vložili containsPoint(Position position, Point point) { do třídy Position > jako parametr metody? Pak ji můžeme použít takto:

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

Hodí se tam opravdu dobře, že?

Když už mluvíme o třídě Positions. Při prohlížení vašeho kódu jsem měl z toho divný pocit. Zpočátku jsem si myslel, že nemáte something[][] představující tabuli. Myslel jsem, že jste reprezentovali vše jako Body v celé kódové základně. To by mohlo fungovat, ale tisk desky by byl nepříjemný. A pak jsem si všiml, že máte char[][] uvnitř vaší Board třídy. Ale pak by nedávalo smysl dávat lodě dovnitř dovnitř tu mřížku, aniž bychom měli střední třídu Pozice?

Také jsem si všiml další nebezpečné věci na placeShipsOnBoard(). Měli byste skutečně důvěřovat svému uživateli, že zadáte 2 platné souřadnice? Co když se uživatel pokusí být zábavný a zadá hodnoty od = (1,1) do = (2,2)? Měli bychom to dovolit? Nebo co když chcete, aby vložil loď o délce 5 a on zadá od = (1,1) do = (1,1) v podstatě zmenšení lodi na jeden čtverec (že musíte zasáhnout 5krát! Protože loď má 5 životů). Neměli bychom mu zabránit v tom, aby takto podváděl?

Pojďme se podívat na alternativní způsob řešení umístění lodí. Nejprve nechejte uživatele, aby si vybral, zda chce loď umístit vodorovně nebo svisle. Pak ho nechte zadat horní / levou souřadnici lodi. Zbývající body si vypočítáme sami.

Zde by mohla vypadat skutečná implementace metody:

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

Teď už jen umístěte lodě přímo na palubu, můžeme odstranit třídu Position a všechny její odkazy. To znamená, že už nevíme, zda se loď potopila nebo ne.

Během toho jsem si všiml, že @Timothy Truckle již také zveřejnil odpověď. Opravdu se mi líbí jeho řešení použití vyhrazených Field s namísto char „s k zastupování rady.

tak naše metoda lodi se změní na:

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

Tímto způsobem můžeme zkontrolovat, zda je loď zničena úplně nebo jen částečně zasáhnout při útoku na Field.

Nyní, místo pokračování této odpovědi, vám doporučuji přečíst si také @Timothy a podívat se na dobré stránky obou našich odpovědí. Na první pohled buď souhlasíme, nebo se jednoduše navzájem doplňujeme. Měli byste tedy mít nějaké slušné tipy, jak vylepšit svůj kód 🙂

Komentáře

• Moc děkuji za podrobnou kontrolu! Všichni dáváte solidní body. Jelikož všechny odpovědi byly tak dobré, dám ‚ l zaškrtnutí osobě, která odpověděla první.
• ahem “ Ve vaší hře není nic špatného. Herní koncept je tak jednoduchý, že nemusíte id = „b83c115eca“>

Další odpovědi poukázaly téměř na všechno, takže přidám jen jednu věc. Vypadá to pro mě, že používáte výjimky, abyste nějak prováděli řízení toku. Výjimky nejsou mechanismy řízení toku .

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

A pak

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

Před pokusem o vytvoření Position si myslím, že byste měli ověřit, zda jsou dané souřadnice uvnitř desky. Toto je vstup uživatele a je naprosto rozumné to udělat. Tento ship.getSize() != Utils.distanceBetweenPoints(from, to) již ověřujete. Dokonce byste to dělali i v samotném objektu Board, místo abyste měli kontrolu polohy Constants.BOARD_SIZE.