rodrigueznazareno56/fiuba-algo3-tp2

Trabajo práctico Nro 2°

fiuba-algo3-tp2's Issues

Falla la creación automática del Release a partir de pushear un Tag

Como se ve en el build del tag

Run softprops/action-gh-release@v1
👩‍🏭 Creating new GitHub release for tag v0.0.2...
⚠️ GitHub release failed with status: 403
undefined
retrying... (2 retries remaining)
👩‍🏭 Creating new GitHub release for tag v0.0.2...
⚠️ GitHub release failed with status: 403
undefined
retrying... (1 retries remaining)
👩‍🏭 Creating new GitHub release for tag v0.0.2...
⚠️ GitHub release failed with status: 403
undefined
retrying... (0 retries remaining)
❌ Too many retries. Aborting...
Error: Too many retries.

Igualmente se lo puede crear manualmente desde Github a partir del tag

Implementar Señority Senior

Comportamiento de acuerdo a enunciado

Seniority	Turnos	Plus de energía por turno
Novato	1-8	0
Semi-Senior	8-12	5
Senior	12 en adelante	10

Si pueden hacer el unitTest para hacerlo por TDD mejor

Sobre inventarioDeEQuiapmiento en caso de Refactor para quitar el getEquipamiento() / obtenerEquipamiento()

Abro este issue cómo discusión para que todos opinen, tengo dudas sobre lo siguiente:

Lo primero es que no termino de estar seguro de esta lógica de assertion en casos de uso como el 5

    @Test
    public void verificarQueSiRecibeUnPremioPorPrimeraVezObtieneUnCasco() {
        ....
       
        //Assert
        assertEquals(gladiador.obtenerEquipamiento(), inventarioDeEquipamiento.buscarPor(1));

No está abstraída de la implementación. Osea en verdad no estoy chequeando un comportamiento que está establecido por contrato, enunciado o caso de uso. Sino que estoy chequeando algo que se que va a ser así porque conozco la lógica interna de la clase.
Chequear que al decirle gladiador.obtenerEquipamiento(), tengo que obtener lo mismo que inventarioDeEquipamiento.buscarPor(1), requiere saber cuánto mínimo saber que gladiador hace un this.inventario.agregarSiNoExiste(equipamiento);. También requiere conocer la lógica interna de buscarPor(1) cuando mínimo para saber qué buscarPor(1) retorna el Casco porque está implementado con un ArrayList<>() donde ya el lugar 0 está ocupado por SinEquipamiento.

Finalmente vale añadir que si deseamos deshacernos de getter innecesarios (aquellos utilizados sólo para testear) entonces seguiríamos en un problema al intentar quitar gladiador.obtenerEquipamiento().

Una forma posible de testear estos casos, testeando consecuencias en el comportamientos y aprovechando que no se trata de un test unitario seria, ver que sucede si es atacado por un fiera ahora que posee un equipamiento superior

    @Test
    public void verificarQueSiRecibeUnPremioPorPrimeraVezObtieneUnCasco() {
        ....
       
        //Assert
        // un gladiador con casco pierde 15 puntos de energía frente al ataque de una fiera
        fiera.afectarGladiador(gladiador)
        assertEquals(gladiador.getEnergia(), 5);

Pero bueno, tampoco estoy tan seguro por eso lo abro como discusión

obtenerEquipamiento se tan solo es utilizada para testear, con algo de ganas se puede quitar el metodo

El equals de la clase equipamiento esta raro.

Solo queremos que devuelva true en el caso en el que sea igual, entonces la segunda comprobación de if false y el ultimo return se puede resumir en un if false final.

Pasar todos los paquetes a modelo

Hay numeros que podrian cambiar por constantes, varios en FieraSalveje, uno en comida

Los test de la entrega 2 probablemente precisan un refactor

El comentario deberia decir fieraSalvaje.atacarContraEquipamiento(EscudoYEspada escudoYEspada)

FIUBA-Algo3-TP2/src/main/test/edu/fiuba/algo3/Equipamientos/EscudoYEspadaTest.java

Line 32 in b4f5b31

// El quipo incrementado al disipar un ataque invoca fieraSalvaje.atacarContraEquipado(Llave llave);

Implementar obstaculo Asiste a un Bacanal

Entiendo que es sencillamente seria una clase AsisteAUnBacanal que implmenta consecuencia. Y su comportamiento.

De acuerdo a enunciado

Obstáculo	Qué sucede	Impacto
Asiste a un Bacanal	El jugador tira un dado para determinar la cantidad de copas de vino a tomar (1-6)	Saca 4 puntos de energía por cada trago tomado

Si pueden testearlo mejor, sino habran una nueva issue pidiendo que alguien lo implemente

Definir un forma de realizar los getters

En algunas clases estamos haciendo

    public int getEnergia() {
        return energia.getValor();
    }

y en otras:

    public Equipamiento obtenerEquipamiento() {
        return equipamiento;
    }

Me da igual pero llevemos todo a una misma manera. get u obtener

Asiste a un Bacanal no debe ser instanciado con la cantidad de tragos

Es una consecuencia que se encuentra presente en una celda. Cuando se instancia la celda, también se instancias todas las consecuencias presentes en ellas. Cuando un jugador cae en la celda, este es afectado por todas las consecuencias presentes en ella.
Nuestra actual implementación de bacanal instancia el bacanal ya con un número de tragos predefinido, este numero de tragos no puede estar en la instanciación ya que la misma se realiza al momento de crear el mapa, el camino, las celdas y sus consecuencias.

    public AsisteAUnBacanal(int cantiadadDeTragos){
        this.cantiadadDeTragos = cantiadadDeTragos;
    }

El bacanal debería ser instanciado sencillamente como un Bacanal y en el momento que un jugador es afectado con esta consecuencia, recien ahi el bacanal solicita al jugador arrojar un dado para obtener cuantos tragos debe tomar.

SinConcecuencia es una clase anemica

Es utilizada por casillero cuando no tiene ninguna consecuencia, sin embargo es anémica. A diferencia de SinEquipamiento que si posee comportamiento. SinConcecuencia no parece estar bien

Comida comportamiento. Inconsistencia entre enunciado y caso de uso 4.

El caso de uso 4 de la entrega 1 es
● Verificar que si recibe comida incrementa energía en 10

Por enunciado

Premio	Qué sucede	Impacto
Comida	El jugador encuentra una comida (usen su creatividad)	La energía se ve incrementada en 15 puntos

En la catedra respondieron:

Joaquin Gomez [hace 5 días]
tomen las que esten al principio, no en las pruebas. Si aparece otro caso, avisen por favor

Hay que:

• Cambiar el test casoDeUso4, tanto nombre como comportamiento esperado
• Cambiar el comportamiento de Comida, hoy devuelve un objeto Energia con valor 10, debe tener valor 15

callJustOnce en vez de Mockito.times(1)

Esta presente en varios test

Refactorizar seniorities test

Algunos nombres estan mal

Unificar constantes

Tenemos que unificar constantes porque hay algunas repetidas en distintos archivos, pudiendo llegar a tener valores distintos

Implementar en Casillero las funciones avanzar y retroceder

Se implmentan ambas funciones en Casillero para que se lanzaen excepciones cuando Gladiador intente retroceder estando en el Casillero inicial o quiera avanzar posicionado en el Casillero final.

Refactor casos de uso entega 2

En los casosDeUso de la entrega 2 en vez de recorrer con un gladiador y medir sus consecuencias, podemos tener un metodo comparar para el mapa, camino y algoRoma

Inconsistencia en la forma de tratar a las concecuencias(?

En los casos de uso que involucran a la clase "EquipamientoIncrementado" que extiende de Concecuencia (CasoDeUso 5, 6, 11) hacemos:

this.gladiador.recibirConsecuencia(equipamientoIncrementado);

En los casos de uso que involucran a la clase "FieraSalvaje" y "Comida" que extiende de Concecuencia (CasoDeUso 4, 7, 10) hacemos:

comida.afectarGladiador(gladiador);
// o
fieraSalvaje.afectarGladiador(gladiador);

Hay que definir una forma consistente de tratar a las Concecuencias. Creo que en gran medida esta va a salir de como sea la logica al momento de mover.

CasillerosFactory no implementa el patron Factory

En principio no es un problema, ni tampoco un requerimiento de cambio sin embargo es bueno tenerlo en consideración. Tanto sea para implementarlo o simplemente cambiarle el nombre.

Refactor AssertEquals de energia

Diego nos dijo que no hagamos

assertEquals(35, gladiador.getEnergia());

Sino que comparemos objetos energia, algo asi como:

assertEquals(new Energia(35), gladiador.getEnergia());

para esto probablemente hay que sobreescribir el método equals de Eneria para que dos objetos Energia sea iguales cuando poseer el mismo valor.

Energia deberia ser como senority

FinDelJuego Excepcion debe ser renombrado a FinDeJuegoException

Es necesario actualizar el badge de codecov

Este es el badge para el README.md

[![codecov](https://codecov.io/gh/RodriguezNazareno56/FIUBA-Algo3-TP2/graph/badge.svg?token=A6M4WZMIMN)](https://codecov.io/gh/RodriguezNazareno56/FIUBA-Algo3-TP2)

Hasta donde entiendo tambien debemos modificar el build.yml de:

jobs:
  build:
    runs-on: ubuntu-20.04
    steps:
      ...
      - name: Upload coverage report
        uses: codecov/codecov-action@v2

a

name: build
on: [ push, pull_request ]
jobs:
  build:
    runs-on: ubuntu-20.04
    steps:
      ...
      - name: Upload coverage report
        uses: codecov/codecov-action@v2
        env:
          CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}

Refactor CasoDeUso5, CasoDeUso6, CasoDeUso11. No se debe testear que un objeto sea de un tipo

Mi corrector en el TP1 fue Joaquin Rivero, una de las correcciones que me hizo fue:

Pruebas: : REGULAR
[Medio] No se debe testear que un objeto sea de un tipo. Se debe testear comportamiento. Tampoco que un objeto sea nil

En los caso de uso 5, 6 y 11, hacemos:
assertEquals(gladiador.getEquipamiento().getClass(), Llave.class);

Si bien idealmente debería haber un refactor para quitar el .getEquipamiento. Es importante cambiar este modo de validar, comparando tipos.

Implementar obstaculo Lesion

Entiendo que es sencillamente seria una clase AsisteAUnBacanal que implmenta consecuencia. Y su comportamiento.

De acuerdo a enunciado

Obstáculo	Qué sucede	Impacto
Lesión	El gladiador se enoja con la vida, patea una piedra y debe esperar un turno sin avanzar	El turno siguiente no avanza

Si pueden testearlo mejor, sino habran una nueva issue pidiendo que alguien lo implemente

Casos borde Test Eneriga

1. Deberiamos evaluar si la energia tiene como minimo el valor cero y no valores negativos ?
2. Se puede inicializar un valor de energia negativo ?

FieraSalvajeTest pruebas sin mock podria pasar a otro paquete

Estan bien las pruebas realizadas al utilizar el gladiador pero creo que deberían pasar a otro paquete y no en el de unitTesting

Triunfar no debe ser una excepcion

Implementar tests unitarios para IncrementoDeEquipamiento

implementaciones para Caso de uso 9

Para el caso de uso 9:

"Verificar que si llega a la meta sin llave en el equipamiento, retrocede a la mitad de las casillas"

Podriamos pensar que la ultima casilla tiene como consecuencia "Ganar". En caso que tenga la llave se arroja la exeption para despues tratarla y en caso que no tenga la llave regresa el jugador a la mitad del juego.

Como las consecuencias las asignan con el JSON deberiamos ver si es posible asignarla nosotros.

CodeQL actions Falla

Hay que comprender por qué falla esta acción. Imagino que en slack nos podrán dar una respuesta, debe ser un fallo presentes en todo o algún problema de configuración

https://github.com/RodriguezNazareno56/FIUBA-Algo3-TP2/actions/workflows/codeql-analysis.yml

El atributo booleano de gladiador EstaHabilitadoParaMover podria ser una entidad, de esa forma pueden sacar el if del metodo avanzar.

Quitar el getEnergia() en casos de uso

Mirando el enunciado del tp, en la entrega 2 dice

Pruebas (sin interfaz gráfica). Refactor de todas aquellas pruebas de la entrega 1 que hayan sido puros
getters para verificar valores y no hayan verificado comportamiento.

En el casoDeUso1 hacemos:

    @Test
    public void verificarQueElJugadorEmpiezaConLaEnergiaYEquipamientoCorrespondiente() {
        // TODO: refactorizar para quitar los getEnergia() y getEquipamiento()
        // Arrange
        ....
        
        // Assert
        assertEquals(gladiador.obtenerEquipamiento(), equipamiento);
        assertEquals(gladiador.getEnergia(), 20);
     }

Una forma de refactorizar esto seria:

    @Test
    public void verificarQueElJugadorEmpiezaConLaEnergiaYEquipamientoCorrespondiente() {
        // TODO: refactorizar para quitar los getEnergia() y getEquipamiento()
        // Arrange
        ....
        
        // Assert
        // Un gladiador debe iniciar sin equipamiento y con 20 puntos de energia.
        // Un gladiador sin equipamiento al ser atacado por una fiera salvaje recibe 20 puntos de energia
        // Si la inicializacion fue correcta, entonces tras el ataque debe quedar sin energia
        // y por ende incapaz de moverse
        gladiador.recibirConsecuencia(new FieraSalvaje());
        Throwable exception= Assertions.assertThrows(MovimientoException.class, gladiador::avanzar);
        assertEquals("El gladiador no se puede mover sin energia", exception.getMessage());
     }

Lo veo algo menos legible pero cumple con el requerimiento de la entrega 2 citado anteriormente. Sin embargo llevado a otras pruebas donde hacemos uso del getter de energía, no lo veo tan sencillo de replicar. Con lo cual hay dos opciones o alguien le encuentra la vuelta para chequear comportamiento y sacar ese getter o dejamos el getter en todas (diría está incluida).

Entrega Final

Entrega Final

Aprobada: ✅
Nota: 8.

Asistencia

Alumno	Presente
* Integrante 1 - Rodriguez Nazareno Jose Luis	✅
* Integrante 2 - Servin Velazquez Marcos	✅
* Integrante 3 - Maldonado Henry	✅
* Integrante 4 - Condori Favio	✅
* Integrante 5 - Sosa Juan Manuel	✅

Comentaris por aspecto

Modelo y paradigma

Herencia

✅

Delegación

✅

Encapsulamiento

✅

Ocultamiento de la información

✅

Principios de diseño

✅

S.O.L.I.D

✅

Tell don’t ask

Podemos reemplazar este método por otro que ejecute una acción cuando
el valor sea menor a 0.

package edu.fiuba.algo3.modelo.gladiador;

import edu.fiuba.algo3.controladores.observers.EnergiaObservable;

import java.util.Objects;

public class Energia extends EnergiaObservable {
    // ...
    
    public boolean isAgotada() {
        return valor <= 0;
    }
}

Ej.:

    public void hacerCuandoAgotada(Callable abc) {
        if ( isAgotada() ) {
            try {
                abc.call();
            } catch (Exception e) {
                throw new RuntimeException(e);
            }
        };
    }

Patrones de diseño

✅

Adaptabilidad

✅

Diagramas UML

Notación

✅

Consistencia

✅

Complejidad

✅

Informe

Herencia vs. Delegación

❌ Falta explicar porqué favoreció una por sobre la otra.

Principios de diseño

❌ Falta explicar donde aplicó "cual" principio.

Patrones de diseño

❌ Falta explicar donde aplicó "cual" patrón.

UI/UX

Estado del sistema: En todo momento el usuario puede conocer el estado del sistema y recibir retroalimentación (feedback) ante los cambios

a. mostrar el estado de los objetos actualizados

b. indicar en lenguaje del usuario cuando hay una falla que impide la continuidad

Legibilidad: Todo usuario puede leer el contenido y los objetos de la UI sin esfuerzo, independientemente de su condición visual.

a. Usar colores accesibles con buen contraste, no más de 4 o 5

b. Usar tipografías y tamaños que no alteren la legibilidad

Mínimo diseño: la UI sólo debe contener la información necesaria para resolver la tarea o problema que se pide en el momento con eficiencia

a. mostrar estados y valores necesarios para el contexto

Consistencia y affordance: Todos los elementos visuales e interactivos (botones, entrada de texto, etiquetas, etc) deberían tener el mismo estilo y comportamiento esperado.

a. usar estilos propios en elementos interactivos estándar** como botones o listas

b. usar siempre el mismo estilo y tamano para el mismo comportamiento

Energia condensar metodo aumentarEnergia, disminuirEnergia en un combinar o como quieran llamarlo, si lo consideran necesario

Refactor avanzar

Si un jugador tiene una energía inferior a 0, ese turno se pasa y se le acreditan 5 puntos extra

Creo que esto no esta implementado

Código repetido en clase Camino.java

Los métodos celdas() y getCeldas() cumplen la misma función.

No se debe hacer assert de los mensajes de las exepciones

Test unitarios para los Senorities

Test unitarios que chequeen el comportamiento de los senorities de acuerdo a enunciado

Seniority	Turnos	Plus de energía por turno
Novato	1-8	0
Semi-Senior	8-12	5
Senior	12 en adelante	10

Hay algunas excepciones sueltas fuera de los paquetes que la utilizan o cualquiera sea la forma en que vayamos a organizarlas

Eliminar el metodo inicializarJuego, dejar la logica dentro del jugarTurno

AssertEquals muchos estan invertidos

¿Posible Refactor al nombre de la clase Nada de equipamiento?

No se que tan a gusto se siente con una clase que se llama Nada, si quieren ponerle un nombre mejor propongan sino bueno queda asi.

Implementar Test Unitario para Comida

SemiSenior debe retornar un Senior cuando alcanza una experiencia de 12

FIUBA-Algo3-TP2/src/main/java/edu/fiuba/algo3/Gladiador/senority/SemiSenior.java

Line 12 in 8040bd5

public Senority aumentarExperiencia() {

bajo un lógica parecida a como lo hace novato:

    @Override
    public Senority aumentarExperiencia() {
        experiencia += 1;
        if (experiencia == 8) {
            return new SemiSenior();
        }
        return this;
    }

Test unitarios para Energia

Implementar test unitarios para energia. La clase puede cambiar sus metodo, de acuerdo a lo que salga del issue #12