Gracias por compartir su código.

Lo que me molesta un poco con este código es que no uso interfaces, clases abstractas o herencia,

Hacer POO significa seguir ciertos principios que son (entre otros):

• ocultar / encapsular información
• responsabilidad única
• separación de preocupaciones
• BESO (Mantenlo simple (y) estúpido.)
• SECO (No te repitas.)
• «¡Dímelo! No preguntes».
• Ley de deméter («¡No hables con extraños!»)

Interfaces, abs clases de tracto o herencia apoyan los principios del sombrero y deben usarse según sea necesario. No «definen» OOP.

En mi humilde opinión, la razón principal por la que su enfoque falla OOP es que su «Modelo» es una matriz de un tipo primitivo char. Esto finalmente conduce a un enfoque de procedimiento para la lógica del juego.

Yo pensaría en una interfaz como esta:

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

donde Result sería una enumeración:

 enum Result{ NO_HIT, PARTIAL_HIT, DESTROYED } 

Y tendría diferentes implementaciones de la interfaz:

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

Esto debería ser suficiente, espero que capte la idea …

Problemas formales

Naming

Encontrar buenos nombres es la parte más difícil de la programación. Así que siempre tómate tu tiempo para pensar en los nombres de tus identificadores.

En el lado positivo, sigues las convenciones de nombres de Java.

Pero debes hacer que los nombres de tus métodos comiencen con un verbo tiempo presente.Por ejemplo: shipWasHit() debe llamarse hit().
O distanceBetweenPoints() debe ser calculateDistanceBetween(). Aquí los parámetros revelan que la distancia está entre puntos, por lo que no es necesario poner eso en el nombre del método.

Sea detallado en los nombres de sus variables. en lugar de

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

estas variables deberían ser llamado así:

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

Tome sus nombres del dominio del problema, no de la solución técnica. Por ejemplo: SHIP_ICON debe ser SHIP solo a menos que tenga otra constante dentro de la clase Ship .

Comentarios

Los comentarios deben explicar por qué el código es como es . Elimina todos los demás comentarios.

Los comentarios solo deben usarse en interfaces o métodos abstractos donde contengan el contrato que el implementador debe cumplir.

Clase de constantes

Junte las cosas que van juntas. Defina constantes en la clase que las usa.

Comentarios

• Todos los puntos válidos y realmente útiles. ¡Gracias!

Ya hay algunas buenas respuestas, pero pensé que agregaría algunas de las cosas que estaban cuando miré el código.

En este momento, la única fuente de entrada es la entrada del usuario desde un escáner. Esto haría que fuera bastante difícil si quisieras agregar algún tipo de oponentes informáticos a contra el que jugar.

Parece que hay algún código en la clase Board que podría ser más adecuado en la clase Player.

Específicamente el contenido del método placeShipsOnBoard ().

Este método toma la entrada del usuario y crea una Posición. Intentemos reestructurar esto para que un Jugador (humano o computadora) pueda crear una Posición.

Hagamos una interfaz

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

Ya tenemos implementaciones de un humano,

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

¿Y qué pasa con un reproductor de computadora básico

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

Luego, en la clase Tablero principal, programamos en la interfaz del reproductor

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 todo el código se refiere a un reproductor (ahora una interfaz) en lugar de una implementación concreta, podemos agregar fácilmente nuevos tipos de reproductores de computadora . p.ej. nuevo CheatingComputer (), nuevo HardComputer (), nuevo MediumComputer (). Cada uno simplemente determinaría dónde disparar a continuación y dónde colocar el próximo barco de una manera diferente.

Y, si tuviéramos esto, ¡podríamos hacer un juego con 2 jugadores de computadora y dejar que se juegue solo! Emocionante a la derecha: D

Otra cosa relacionada que podríamos cambiar es que en tu constructor de Juego, siempre tendrás 2 jugadores humanos. Podríamos hacer que esto tome una lista de jugadores, por lo que podría tener cualquier número de jugadores en su juego. El hecho de que el juego real Battleship esté limitado a 2 jugadores no significa que el tuyo deba serlo.

Podríamos permitir un tamaño de cuadrícula ajustable y un número de jugador arbitrario. De repente, tenemos una cuadrícula de 100×100 con un 8 jugador gratis para todos!

(cualquier cantidad de la cual podría ser controlada por computadora).

Tus naves también se inicializan en un bloque estático en tu clase de tablero. Tienes todas las naves reales de Battleship. Pero, de nuevo, ¿por qué no permitir más flexibilidad aquí? ¿Qué pasaría si su barco consistiera en una lista de puntos? Podría tener barcos en forma de S, no tendrían que limitarse a la alineación horizontal o vertical. (Esto puede ser exagerado, ¡pero creo que es genial pensar en ello!)

Terminaré con algunas pequeñas cosas que me parecieron divertidas

throw new ArrayIndexOutOfBoundsException(); 

de la clase Posición. Esto parece una excepción inapropiada para lanzar aquí. No hay una matriz en la clase Position, por lo que esto debe estar refiriéndose a la matriz en la clase Board. Creo que un tipo de excepción más adecuado sería una IllegalArgumentException, ArrayIndexOutofBoundsException es un detalle de implementación (¡de una clase diferente!). Debería también asegúrese de proporcionar un mensaje de error apropiado junto con lanzar la excepción. Por ejemplo, «el valor debe estar dentro del rango xey»

La línea

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

simplemente podría ser reemplazado por

boolean isShipHit = ship != null; 

No hay necesidad del operador ternario aquí.

El uso de targetHistory en la clase Player while (targetHistory.get (point)! = null)

Aquí estás usando un mapa con el único propósito de ver si un elemento está en él. Esto es exactamente lo que es un conjunto para!

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

Comentarios

• ¡Muchas gracias por la información! Todas estas respuestas complementan cada otros muy bien! Yo ‘ empezaré a trabajar en la versión 2.0 con todo esto en mente.
• No hay problema, ‘ ¡me alegra que lo haya encontrado útil! ¡Buena suerte con 2.0!

Lo que molesta un poco con este código es que no uso interfaces, clases abstractas o herencia, pero no pude encontrar un buen caso de uso para ellos aquí.

No hay nada de malo en esto en tu juego. El concepto del juego es tan simple que no necesitas ninguno de esos. El problema con los programas de juguetes pequeños es que normalmente no necesitas aplicar grandes soluciones de diseño. Solo necesita asegurarse de seguir los principios de diseño SOLID habituales.

Ahora que lo aclaramos, dejemos » mira algunos detalles de tu código que deberían mejorarse.

El primero es bastante obvio. No escribas comentarios por el simple hecho de escribir comentarios. A algunos profesores les gusta obligarte a escribir un comentario javadoc sobre todo. En realidad, estoy completamente en contra de esto, excepto cuando escribo algún tipo de paquete de utilidades que todos los demás deben usar. Normalmente, el código debería ser autodocumentado. Y su código hace un muy buen trabajo en eso. Así que simplemente elimine el comentario que «s básicamente repitiendo el nombre bien elegido de la variable / función / …

Por ejemplo:

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

¿Qué valor agrega ese comentario? a la función?Creo que este es también el único método que cambiaría para hacerlo más legible. Porque no es obvio que la posición esté formada por un punto from y to para el cual verificamos si nuestro point se encuentra entre ellos.

¿Qué pasaría si el método tuviera esta firma:

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

¿No sería eso? ¿Tiene un poco más de sentido?

Debo agregar aquí que «no estoy en contra de los comentarios en general. Pero un comentario debería explicar POR QUÉ algo se hace de esa manera. No cómo se implementa. Si quisiera Sé cómo se implementa. Acabo de ver el código.

El siguiente punto es tener esa clase Util … A diferencia de algunos puristas, no tengo nada en contra del concepto de clases Util Las clases de util pueden ser útiles para juntar funciones similares. Por ejemplo, java.lang.Math que agrupa todas las operaciones aritméticas habituales en una clase.

Lo que me preocupa con su clase Util es que realmente no hay una buena razón para que exista. Las 2 funciones que tiene allí solo se usan dentro de la clase Board. Así que podrían haber sido también private static funciones auxiliares dentro de esa clase.

De hecho, incluso podemos hacerlo un poco mejor después de cambiar la firma a lo que Sugerí anteriormente. ¿Qué pasa si ponemos containsPoint(Position position, Point point) { dentro de la Position clase en lugar de tener la Position como parámetro para el método? Entonces podemos usarlo así:

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

Encaja muy bien allí, ¿no es así?

Hablando de la clase Positions. Tuve una sensación extraña sobre esto mientras revisaba su código. Al principio pensé que no tenías un something[][] para representar el tablero. Pensé que habías representado todo como Puntos en todo el código base. Esto podría funcionar pero hace que la impresión del tablero sea incómoda. Y luego noté que tenía un char[][] dentro de su Board clase. Pero entonces, ¿no tendría más sentido poner inmediatamente los barcos dentro esa cuadrícula sin tener una clase de posición intermedia?

También noté otra cosa peligrosa sobre el placeShipsOnBoard(). ¿Debería realmente confiar en que su usuario ingrese 2 coordenadas válidas? ¿Qué pasa si el usuario intenta ser gracioso e ingresa de = (1,1) a = (2,2)? ¿Deberíamos permitir esto? O ¿qué pasa si quieres que ingrese un barco de longitud 5 y él ingresa de = (1,1) a = (1,1) esencialmente reduciendo el barco a un solo cuadrado (que tienes que golpear 5 veces! Desde que el barco tiene 5 vidas). ¿No deberíamos evitar que haga trampa de esta manera?

Veamos una forma alternativa de manejar la colocación de los barcos. Primero, deje que el usuario elija si quiere colocar el barco horizontal o verticalmente. Luego, pídale que ingrese la coordenada superior / izquierda del barco. Calcularemos los puntos restantes nosotros mismos.

Así es como podría verse la implementación real del método:

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

Ahora que acabamos de Coloque los barcos en el tablero directamente, podemos eliminar la clase Position y todas sus referencias. Esto significa que ya no sabemos si un barco se ha hundido o no.

Mientras escribía esto, noté que @Timothy Truckle ya publicó una respuesta también. Realmente me encanta su solución de usar Field s dedicados en lugar de char «s para representar el tablero.

nuestro método de envío de lugar cambia a:

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 esa manera podemos verificar si un barco se destruye por completo o simplemente se golpea parcialmente al atacar un Field.

Ahora, en lugar de continuar con esta respuesta, te sugiero que leas también a @Timothy, y veas los puntos positivos de ambas respuestas. A primera vista, estamos de acuerdo o simplemente complementamos la respuesta del otro. Por lo tanto, debería tener algunos consejos decentes sobre cómo mejorar su código 🙂

Comentarios

• ¡Muchas gracias por una revisión detallada! Todos ustedes hacen puntos sólidos. Dado que todas las respuestas fueron tan buenas, ‘ le daré la marca de verificación a la persona que respondió primero.
• ejem » No hay nada de malo en esto en tu juego. El concepto del juego es tan simple que no ‘ No necesito ninguno de esos. » – No estoy de acuerdo: Es ‘ mejor practicar cualquier cosa en modo fácil primero. Si no puedes ‘ t aplicar conceptos y patrones en algo simple, seguro que puedes ‘ t aplicarlos en algo más complejo.
• La parte sobre los comentarios es especialmente importante. Los comentarios redundantes son una completa pérdida de tiempo y energía para escribir iterar, mantener y verificar. Solo comente si no puede hacer que el código y las funciones se expliquen.

Otras respuestas han señalado casi todo, así que solo agregaré una cosa. Parece para mí que está utilizando excepciones para realizar de alguna manera el control de flujo. Las excepciones no son mecanismos de control de flujo .

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

Y luego

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

Creo que debería validar que las coordenadas dadas están dentro del tablero antes de intentar crear el Position. Esta es la entrada del usuario y es perfectamente razonable hacerlo. Ya estás validando ese ship.getSize() != Utils.distanceBetweenPoints(from, to). Incluso estarías haciendo eso en el objeto Board en sí, en lugar de tener la comprobación de posición para Constants.BOARD_SIZE.