Merci pour partager votre code.

Ce qui me dérange un peu avec ce code, cest que je nutilise pas dinterfaces, de classes abstraites ou dhéritage,

Faire de la POO signifie que vous suivez certains principes qui sont (entre autres):

• masquage / encapsulation dinformations
• responsabilité unique
• séparation des préoccupations
• KISS (Restez simple (et) stupide.)
• DRY (Ne vous répétez pas.)
• « Dites! Ne demandez pas. »
• Loi de déméter (« Ne parlez pas à des inconnus! »)

Interfaces, abs les classes de tractus ou les principes du chapeau de support dhéritage et doivent être utilisés si nécessaire. Ils ne « définissent » pas la POO.

À mon humble avis, la principale raison pour laquelle votre approche échoue OOP est que votre « modèle » est un tableau de type primitif char. Cela conduit finalement à une approche procédurale pour la logique du jeu.

Je penserais à une interface comme celle-ci:

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

où Result serait une énumération:

 enum Result{ NO_HIT, PARTIAL_HIT, DESTROYED } 

Et jaurais différentes implémentations de linterface:

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

Cela devrait suffire, jespère que vous Obtenez lidée …

Problèmes formels

Dénomination

Trouver de bons noms est la partie la plus difficile de la programmation. Prenez donc toujours votre temps pour réfléchir à vos noms didentifiant.

Du bon côté, vous suivez les conventions de dénomination Java.

Mais vous devriez faire commencer les noms de méthodes par un verbe dans son le présent.Eg: shipWasHit() doit être nommé hit().
Ou distanceBetweenPoints() doit être calculateDistanceBetween(). Ici, les paramètres révèlent que la distance est entre les points, donc pas besoin de mettre cela dans le nom de la méthode.

Soyez verbeux dans vos noms de variables. au lieu de

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

ces variables devraient plutôt être nommé comme ceci:

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

Prenez vos noms du domaine du problème, pas de la solution technique. par exemple: SHIP_ICON doit être SHIP uniquement sauf si vous avez une autre constante dans la classe Ship .

Commentaires

Les commentaires doivent expliquer pourquoi le code est comme il est . Supprimez tous les autres commentaires.

Les commentaires ne doivent être utilisés que sur les méthodes dinterface ou abstraites où ils contiennent le contrat que limplémenteur doit remplir.

Classe de constantes

Rassemblez les choses qui vont ensemble. Définissez les constantes dans la classe qui les utilise.

Commentaires

• Tous les points valides, et vraiment utiles. Merci!

Il y a déjà de bonnes réponses mais je pensais « ajouter quelques-unes des choses qui se tenaient pour moi quand jai parcouru le code.

Pour le moment, la seule source dentrée est lentrée utilisateur dun scanner. Cela rendrait la tâche assez difficile si vous vouliez ajouter une sorte dadversaires informatiques à jouer contre.

Il semble quil y ait du code dans la classe Board qui pourrait être mieux adapté dans la classe Player.

Plus précisément le contenu de la méthode placeShipsOnBoard ().

Cette méthode prend les entrées de lutilisateur et crée une position. Essayons de la restructurer afin quun joueur (humain ou ordinateur) puisse créer une position.

Faisons un interface

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

Nous avons déjà des implémentations dun humain,

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

Et quen est-il un lecteur informatique de base

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

Ensuite, dans la classe principale Board, nous programmeons sur linterface Player

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

Si tout le code se réfère à un Player (maintenant une interface) plutôt quà une implémentation concrète, nous pouvons facilement ajouter de nouveaux types de lecteurs informatiques . par exemple. nouveau CheatingComputer (), nouveau HardComputer (), nouveau MediumComputer (). Chacun déterminerait simplement où tirer ensuite et où placer le prochain vaisseau dune manière différente.

Et, si nous avions cela, nous pourrions créer un jeu avec 2 joueurs sur ordinateur et le laisser jouer tout seul! Excitant droit: D

Une autre chose connexe que nous pourrions changer est que dans votre constructeur pour Game, vous aurez toujours 2 joueurs humains. Nous pourrions faire en sorte que cela prenne une liste de joueurs, afin que vous puissiez avoir nimporte quel nombre de joueurs dans votre partie. Ce nest pas parce que le vrai jeu Battleship est limité à 2 joueurs que le vôtre doit lêtre.

Nous pourrions permettre une taille de grille ajustable et un nombre de joueurs arbitraire. Soudain, nous avons une grille de 100×100 avec un 8 joueur gratuit pour tous!

(dont nimporte quelle quantité peut être contrôlée par ordinateur).

Vos vaisseaux sont également initialisés dans un bloc statique de votre classe de plateau. Vous avez tous les vrais vaisseaux Mais encore une fois, pourquoi ne pas permettre plus de flexibilité ici. Et si votre vaisseau se composait dune liste de points? Vous pourriez avoir des navires en forme de S, ils nauraient pas besoin dêtre limités à lalignement horizontal ou vertical. (Cest peut-être exagéré, mais je pense que cest « une chose sympa à penser!)

Je terminerai avec quelques petites choses qui me paraissaient amusantes

throw new ArrayIndexOutOfBoundsException(); 

de la classe Position. Cela semble être une exception inappropriée à lancer ici. Il ny a pas de tableau dans la classe Position, donc cela doit faire référence au tableau de la classe Board. Je pense quun type Exception plus approprié serait une IllegalArgumentException, ArrayIndexOutofBoundsException est un détail dimplémentation (dune classe différente!). Vous devriez assurez-vous également de fournir un message derreur approprié avec le lancement de lexception. Par exemple, « la valeur doit être comprise entre x et y »

La ligne

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

pourrait simplement être remplacé par

boolean isShipHit = ship != null; 

Il ny a pas besoin de lopérateur ternaire ici.

Lutilisation de targetHistory dans la classe Player while (targetHistory.get (point)! = null)

Ici, vous utilisez une carte dans le seul but de voir si un élément sy trouve. Cest exactement ce quest un Set pour!

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

Commentaires

• Merci beaucoup pour la perspicacité! Toutes ces réponses complètent chacune autre très bien! Je ‘ Je vais commencer à travailler sur la version 2.0 avec tout cela à lesprit.
• Pas de problème Je ‘ je suis content que vous layez trouvé utile! Bonne chance avec la version 2.0!

Quest-ce qui agace moi un peu avec ce code est que je nutilise pas dinterfaces, de classes abstraites ou dhéritage, mais je nai pas pu trouver un bon cas dutilisation pour eux ici.

Il ny a rien de mal à cela dans votre jeu. Le concept du jeu est si simple que vous nen avez pas besoin. Le problème avec les petits programmes de jouets est que vous navez généralement pas besoin dappliquer de grandes solutions de conception. Il vous suffit de vous assurer de suivre les principes de conception SOLID habituels.

Maintenant que nous avons clarifié cela, laissez « s regardez quelques détails de votre code qui devraient être améliorés.

Le premier est assez évident. Nécrivez pas de commentaires pour le plaisir décrire des commentaires. Certains professeurs aiment vous forcer à écrire un commentaire javadoc sur tout. Je suis en fait complètement contre cela, sauf lorsque jécris une sorte de package utilitaire que tout le monde doit utiliser. Normalement, le code devrait être auto-documenté. Et votre code fait un très bon travail à cela. Donc supprimez simplement le commentaire qui « s répétant essentiellement le nom bien choisi de la variable / fonction / …

Par exemple:

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

Quelle valeur ce commentaire ajoute à la fonction?Je pense que cest aussi la seule méthode que je changerais pour la rendre plus lisible. Car il « nest pas évident que la position soit constituée dun point from et to pour lequel nous vérifions si notre point se situe entre eux.

Et si la méthode avait cette signature:

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

Ne serait-ce pas Cela a un peu plus de sens?

Je devrais ajouter ici que je ne suis pas contre les commentaires en général. Mais un commentaire devrait expliquer POURQUOI quelque chose est fait de cette façon. Pas comment cela est implémenté. Si je le voulais sachez comment il est implémenté, je viens de regarder le code.

Le point suivant est davoir cette classe Util … Contrairement à certains puristes, je nai rien contre le concept des classes Util . Les classes Util peuvent être utiles pour rassembler des fonctions similaires. Par exemple java.lang.Math qui regroupe toutes les opérations arithmétiques habituelles dans une seule classe.

Ce qui me dérange avec votre classe Util, cest quil ny a pas vraiment une bonne raison pour quelle existe. Les 2 fonctions que vous avez là ne sont utilisées que dans la classe Board. Ils pourraient donc « avoir tout aussi bien été private static des fonctions daide à lintérieur de cette classe.

En fait, nous pouvons même faire un peu mieux après avoir changé la signature en quoi Jai suggéré plus tôt. Et si nous mettions containsPoint(Position position, Point point) { dans la classe Position au lieu davoir la Position comme paramètre de la méthode? Ensuite, nous pouvons lutiliser comme ceci:

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

Il sintègre vraiment bien là, nest-ce pas?

En parlant de la classe Positions. Jai eu un sentiment étrange à ce sujet en regardant votre code. Au début, je pensais que vous naviez pas de something[][] pour représenter le tableau. Je pensais que vous aviez tout représenté sous forme de points dans toute la base de code. Cela pourrait fonctionner mais rend limpression du tableau difficile. Et puis jai remarqué quil y avait un char[][] dans votre classe Board. Mais alors cela naurait pas plus de sens de placer immédiatement les vaisseaux à lintérieur cette grille sans avoir de classe Position intermédiaire?

Jai également remarqué une autre chose dangereuse à propos du placeShipsOnBoard(). Devriez-vous vraiment faire confiance à votre utilisateur pour entrer 2 coordonnées valides? Que faire si lutilisateur essaie dêtre drôle et saisit de = (1,1) à = (2,2)? Devrions-nous permettre cela? Ou que se passe-t-il si vous voulez quil entre un navire de longueur 5 et quil entre = (1,1) à = (1,1) réduisant essentiellement le navire à une seule case (que vous devez frapper 5 fois! Depuis le navire a 5 vies). Ne devrions-nous pas lempêcher de tricher comme ça?

Regardons une autre façon de gérer le placement des vaisseaux. Tout dabord, laissez lutilisateur choisir sil souhaite placer le navire horizontalement ou verticalement. Puis demandez-lui de saisir les coordonnées haut / gauche du navire. Nous calculerons nous-mêmes les points restants.

Voici à quoi pourrait ressembler limplémentation réelle de la méthode:

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

Maintenant que nous venons de placez les vaisseaux directement sur le plateau, nous pouvons supprimer la classe Position et toutes ses références. Cela signifie que nous ne savons plus si un navire a coulé ou non.

En yping, jai remarqué que @Timothy Truckle avait déjà publié une réponse. Jadore sa solution consistant à utiliser des Field s au lieu de char « s pour représenter le tableau.

donc notre méthode de vaisseau de lieu change en:

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

De cette façon, nous pouvons vérifier si un navire est complètement détruit ou sil est simplement touché partiellement lors de lattaque dun Field.

Maintenant, au lieu de continuer sur cette réponse, je vous suggère de lire également les @Timothy » s, et de regarder les bons points dans nos deux réponses. À première vue, nous sommes soit daccord, soit nous complétons simplement la réponse de lautre. Vous devriez donc avoir des conseils décents sur la façon daméliorer votre code 🙂

Commentaires

• Merci beaucoup pour cet examen détaillé! Vous avez tous fait valoir des points solides. Comme toutes les réponses étaient très bonnes, je ‘ coche la personne qui a répondu dabord.
• ahem  » Il ny a rien de mal à cela dans votre jeu. Le concept du jeu est si simple que vous ne ‘ Jai besoin de lun de ces éléments.  » – Je ne suis pas daccord: il vaut mieux ‘ sentraîner tout en mode facile d’abord. Si vous pouvez ‘ t appliquer des concepts et des modèles à quelque chose de simple, vous pouvez certainement ‘ t les appliquer sur quelque chose de plus complexe.
• La partie sur les conments est particulièrement importante. Les commentaires redondants sont une perte totale de temps et dénergie à écrire ite, entretenez et vérifiez. Ne commentez que si vous ne pouvez pas obtenir le code et les fonctions pour sexpliquer.

Dautres réponses ont presque tout souligné, alors jajouterai juste une chose. pour moi que vous utilisez des exceptions pour effectuer en quelque sorte le contrôle de flux. Les exceptions ne sont pas des mécanismes de flux de contrôle .

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

Et puis

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

Je pense que vous devriez valider que les coordonnées données sont à lintérieur du tableau avant dessayer de créer le Position. Cest une entrée de lutilisateur et il est parfaitement raisonnable de le faire. Vous « êtes déjà en train de valider cet ship.getSize() != Utils.distanceBetweenPoints(from, to). Vous le feriez même dans lobjet Board lui-même, au lieu de vérifier la position pour Constants.BOARD_SIZE.