tombpotochek / algochess Goto Github PK

Les dejo un par de comentarios sobre mejoras o correcciones:

• Eliminar código comentado, por ejemplo en la clase EstadoArma, Jinete, etc, además limpien el resto de código comentado.

• En las clases correspondientes a las diferentes Distancias, existen números mágicos, reemplazar por cosntantes y agregarlo al ProveedorDeConstantes.

• Sugerencia: En el método atacar(Unidad unidad, Tablero tablero) de Unidad en lugar de atrapar UnidadDestruidaException y luego quitar la pieza tal vez sea mejor dejar seguir la excepción y atraparlo en un nivel superior Juego o AlgoChess y sea éste el que se encargue de remover la pieza destruida delegando a tablero. Ya que como está ahora da a entender que el atacar hace dos cosas, ataca y si está destruido se encarga de quitarlo del tablero.

• La clase UnidadAtacante no se usa, es es correcto? Si no se usa removerlo.

Buenas aquí les dejo algunos comentarios y observaciones sobre la entrega 1:

• Agregar un README con la siguiente información: Nombre del grupo, integrantes del grupo, enlace al informe y diagramas actualizados. Además un enlace que muestre el estado de Travis.

• Servidor de integración continua en estado failed, revisen por que es ésto y traten de arreglarlo, la idea es tener siempre el servidor de CI en successful.

• Reordenar y agrupar clases, no tener todas las clases sueltas, tanto para el modelo como para test. Tratar de agruparlos en paquetes que tengan relación, manteniendo la cohesión.

• Números mágicos: Hay varias números sueltos por ahí (vida inicial, daño, costo, etc) reemplazarlos, existen dos posibilidades:

• Constantes dentro de la clase:

   private final int DANIO = 10;
   public ArmaSoldado() {
        super(DANIO, Rango.cercano());
    }

• Tener una clase estática, con todos las constantes juntas:

public static class ProveedorConstantes(){
   public static int DANIO_ARMA_SOLADO = 10;
   public static int DANIO_ARMA_JINETE = 10;
   public static int VIDA_SOLDADO = 100;
   ...
}

De la segunda manera es más sencillo configurar las constantes del juego y permite probar algunas funcionalidades del juego de manera más rápida durante las correcciones.

• Test que no hacen nada o están comentados por ejemplo: CasillaTest, CatapultaTest, etc. Revisarlos y borrarlos si no hacen falta.

• Faltan más test unitarios, Casilla por ejemplo, si bien está bien tener pruebas de integración , también debe haber pruebas unitarias.

• Hay codigo comentado en la clase Jinete, analicen si se tiene que quitar y busquen por otro casos parecidos.

• El método atacar(Unidad unaUnidad, Tablero tablero), se repite exactamente igual en todas las unidades, extraerlo en una entidad/abstracción. Se me ocurre, por como está su código ahora, crear la interfaz Movible que defina los métodos propios asociados al movimiento, luego crear una clase Abstracta para cada posible unidad (que se mueva, que ataque, que se mueva y ataque) y que cada unidad extienda la que necesita. Es aplicar uno de los Principios SOLID, la pregunta es: qué principio es?.

• Equipo debería ser una clase y no un String, de esta manera se le puede agregar comportamiento, dado que cada unidad, jugador, casillero tiene un equipo asociado.

• El mensaje 'moverUnidad(Posicion posicionOrigen, Posicion posicionDestino)en tablero no es del todo correcto, dado que si lo quieren hacer así debería recibir laUnidad` y la posición de destino, y dentro mandarle el mensaje a unidad de moverse y que se resuelva por delegación el vaciar y ocupar casilla.

    public void moverUnidad(Unidad unaUnidad, Posicion posicionDestino) {
         //validar que es una posición válida
    	Casilla casillaDestino = this.tablero.get(posicionDestino);
        unaUnidad.mover(casillaDestino); //internamente se resuelve el ocupado y vaciado de las casillas.
    }

Otra manera de hacerlo sería directamente mandar el mensaje a unidad:unaUnidad.mover(posicionDestino, tablero), sería una variante del anterior. Luego hay otra posibilidad unaUnidad.mover(tablero) en éste caso la unidad ya tiene una dirección fijada y sabe a donde se debe mover solo necesita al tablero para actualizar las casillas.

Por el momento es solo eso.

Cualquier duda me escriben.

Modelo:

• Daño extra al atacar en territorio enemigo

• Interfaz para atacar y mover unidades en Algochess

• Agregar los mensajes de error en las excepciones

• Hacer clase Observable e interfaz Observer para la notificacion a la vista de cambios en el modelo. Ejs:
  https://github.com/FedeJure/MetalSlugExample/blob/master/src/main/java/com/fiuba/algo3/modelo/Observable.java
  https://github.com/FedeJure/MetalSlugExample/blob/master/src/main/java/com/fiuba/algo3/modelo/Observer.java