TP1-G3

Resultado: Aprobado con obs.

Estructura:

- Sev 1: Se han subido binarios y archivos de configuración. Ejemplos de cosas que no deben subirse: /target (y su contenido), .settings, .classpath, .project. Ojo con esto porque probablemente sea la causa por la que no se dieron cuenta del problema mencionado en "Instalación".

- Sev 3: No hay un archivo tipo readme.txt que indique los contenidos del repositorio, ni los documentos presentados.

Instalación:

- Sev 1: Descargando el código fuente y corriendo mvn compile no compila, porque al pom.xml le falta el siguiente fragmento:

<build>

<groupId>org.apache.maven.plugins</groupId>

<artifactId>maven-compiler-plugin</artifactId>

</configuration>

</plugin>

</plugins>

</build>

Ojo con los fix que hacen en eclipse (porque no corrigen el pom.xml sino los settings del project)

Tests:

- Sev 2: Existe código <significativo> que no tiene tests que lo invoque. "desuscribir" y "sePuedeDesuscribir" de la clase Evento no son llamados por los tests.

- Sev 2: Objetivo y contexto del test. Está claro que el test consultarSuscriptosAEvento tiene como objetivo verificar las suscripciones, pero mirando el test por dentro, el objetivo real es primero ver la cantidad de inscriptos y luego verificar si funciona el método estaSuscsripto, en total son como 2 o 3 tests. Les tiro nombres a modo de tip: suscribirDosPersonasAumentaLosInscriptos, estaSuscriptoDevuelveFalseParaLosNoSuscriptos, y estaSuscriptoDevuelveTrueParaLosSuscriptos. Además, los tests deberían ser claros en cuanto al contexto. O sea, si van a crear un evento con 2 asistentes para verificar la cantidad de asistentes, pueden obviar la creación de los asistentes (utilizando variables de instancia en el test) pero deberían hacer el "new" del Evento en el mismo test. El tema es que las exceptions también hacen que el test salga rojo (a menos que se especifique lo contrario), y si ustedes intentan desuscribir a alguien que no está suscripto, una buena forma de probarlo es pasando el mensaje desuscribir dentro del test (no en el setup) y verificar que devuelva una exception.

- Sev 2: Tests con significado. Cuando se comienza con TDD siempre surge la discusión si un test es significativo o no. Por ejemplo, el test crearUnEvento (clase EventoTest) valida un constructor, setters, y getters, cuando lo interesante sería: A) 1 test que cree el evento con el constructor por default y verifque que el evento no tiene inscriptos y algún atributo que resulte interesante, y B) 1 test que muestre el porqué de tener un constructor que reciba todos los parámetros (o sea, para qué lo necesito?)

Diseño:

- Sev 3: Si tienen que aclarar el propósito de un atributo o método, entonces es un potencial indicador que el nombre está mal. En el caso del atributo duracion de la clase Evento, en vez de agregar el comentario podrían llamar al atributo duracionEnMinutos, y con ello quien lo consuma ya sabe la unidad. El ejemplo en donde esto representa un error importante es en el método suscribir de la clase Evento. Permitan que la suscripción se pueda hacer de los 2 lados (agregando un IF en alguno de los 2 extremos), o cambien el nombre del método para indicar el propósito (por ejemplo registrarSuscripcionRealizada) y/o háganlo interno al paquete (con el impacto en testeabilidad)

- Sev 3: Si bien el TP todavía es chico, tengan en cuenta que log4j no reemplaza exceptions. Hay algunos puntos en los cuales un exception habría venido bien, por ejemplo al intentar desuscribir a un usuario de un evento al cual no está suscripto.

- Sev 3: Faltaría que loguearan un poco más, por ejemplo, loguearon la desuscripción pero no la suscripción. Si tuvieran que seguir el trace se darían cuenta que falta algo, o sea, uno viene leyendo "se creó usuario, se creó evento, se desuscribió usuario de evento" y da la sensación que el sistema está haciendo más cosas de las que dice que está haciendo, y estamos hablando de loglevel = debug...

- Sev 4: Clase Administrador, no está claro porque tomaron la decisión de subclasear algo que representa un "rol" (probablemente modificable durante la vida del usuario), pero estaría bueno que este tipo de decisiones las registraran en el txt de explicación o en JavaDoc, más aún viendo que en los test quedó registrado que se chocaron con esta decisión: "//TODO: Tiene sentido si cambiamos el modelo de permisos por un strategy".