TP1-G7

Resultado: Aprobado con obs.

La entrega estuvo en tiempo y forma lo cual es muy positivo, sigan así!

A continuación les pasamos algunos puntos que estuvimos viendo. Abajo de todo está lo que queremos que hagan para seguir con el TP.

1. Falta DOCUMENTACION!!!

No hay ni siquiera un diagrama de clases para guiarnos o ver el comportamiento de las clases. Ni tampoco hay documentación viva (JavaDoc), lo cual es esencial para poder saber que hace el método sin tener que entrar a ver el código. Además, una documentación viva con JavaDoc hace que el código sea mucho más mantenible y permite tener una API mucho más fácil de usar. Para las próximas entregas agreguen un pequeño documento con los diagramas que consideren necesarios para explicar la solución y expliquen un poco que problemas se les presentaron, como y porque decidieron resolverlos de la forma que lo hicieron.

2. En el POM.XML falta agregar las siguientes líneas:

<build>

<finalName>eventos </finalName>

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

</configuration>

</plugin>

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

<artifactId>maven-eclipse-plugin</artifactId>

</plugin>

</plugins>

</build>

Esto hace que sea posible la compilación correcta al ejecutar “mvn compile” en la línea de comandos más allá del equipo donde se corra.

3. No hacen uso de Exceptions! Las excepciones son muy utiles para saber que partes del código estallaron. Deberian agregar unas cuantas en los métodos donde se supone que no tienen que pasar ciertas cosas, como por ejemplo si un Usuario se quiere registrar 2 veces a un Evento o el Usuario no existe, etc. Es importante chequear determinadas precondiciones y cosas que pueden pasar en el dominio, y que el sistema explote en caso de que haya algo inesperado.

o Por ejemplo no validan que cuando se registra la asistencia de un usuario a un evento, el usuario esté suscripto a ese evento.

o En el test testCancelarSuscripcionFallida están dandose cuenta de que algo no anduvo como corresponde, sin embargo dejan seguir el curso de las cosas como si estuviera todo bien. Esto tiene que explotar para que todos entiendan que algo anduvo mal o sino aclaren que en el valor de retorno se informa eso. Es MUY PELIGROSO que el que está llamando a un método piense que se llevó a cabo de forma correcta cuando ustedes saben que esto no fue así. Si eligen la primer opción que el test verifique que se lanzó una excepción, si eligen la segunda, que el test verifique que el método devolvió false.

o Piensen y agreguen validaciones que crean necesarias y si les hace falta las discutimos.

o Recuerden que los tests tienen que testear tanto los escenarios que funcionan “bien” como los que funcionan “mal” (por ejemplo lanzan excepciones)

4. El método “agregarSuscripcion(Evento)” de la clase Usuario devuelve un booleano indicando si el usuario fue correctamente agregado. Quizás es más práctico que no devuelva nada y lance una excepcion en caso de que no pueda agregarlo. De cualquiera de las 2 formas (pero más con la opcion del booleano) debería estar documentado para que nadie se olvide de chequear y piense que salió todo bien

5. El método “registrarAsistencia(Usuario)” de la clase Evento debería settear el valor Booleano de dicho Usuario en el HashMap en True. Sin embargo eso NO es lo que hace. Para cambiar el valor correspondiente a cierta Key en un Mapa, usen el método “mapa.put(usuario, valor)”.

Lo más grave es que este error no haya saltado en un test. Para corregirlo quiero que hagan un test que de rojo, y después modifiquen el código para hacer que el test de verde.

6. log.debug("TEST: testAgregarSuscripcion"); Estas lineas no hacen falta, no es necesario loguear los tests. El log es más que nada para entornos productivos o de test pero no unitario, sino de integración o de aceptación de usuario.

7. Tienen una variable estática (de clase) static private int numero;

o No esta bueno usarlo así, quizás si para obtener el ID, pero sería más feliz sacar esto fuera de la clase. Aun, si quisieran usarlo así, deberían ponerle un atributo de instancia a cada instancia de Evento, y al crearlo guardarse el valor de numero en ESE MOMENTO.

o log.info( "Consultado suscriptos para el evento: " + Evento.getNumero() );

Esta línea está mal, como decía arriba, toma el valor de la variable de clase que se comparte entre todas las instancias. (siempre va a mostrar el numero del último evento)

o Más allá de las cuestiones técnicas, se pidió que la entrega se desarrollara usando TDD justamente para no sobrediseñar ni generar ningún feature que no agregara valor directamente al cliente. El número del evento no era algo que se pidiera, en otras palabras, gastaron recursos del equipo desarrollando algo que al cliente no le interesa. No está mal si lo justifican por el lado técnico y que es necesario, pero es algo que tienen que justificar muy bien y pensar varias veces antes de mandarse a hacerlo.

8. En Evento.cancelarSuscripcion tienen un comentario del estilo Javadoc adentro del código, esto viola las convenciones. Usen /** cuando es un javadoc y /* para comentarios normales (van a ver que la idea les cambia el color de azul a verde)

9. if( this.chequearFecha() == true )

Esta línea se puede remplazar por: if( this.chequearFecha())

Al ser el valor de retorno boolean, queda más entendible así. Más allá de eso, hay algo con el nombre:

o chequeaFecha suena a un método que no devuelve nada (void) y realiza un chequeo sobre la fecha, lanzando una exception en caso de que falle.

o Si quieren que el método realmente devuelva un boolean ponganle un nombre del estilo isFechaValida

10. En la clase Usuario hacen muchas referencias al atributo eventos en vez de generar un getter (getEventos) y acceder a travez de él. Esto no está mal, pero cuando usen Hibernate es probable que le traiga algunos problemas. La recomendación es que lo cambien por un getter y que accedan siempre (aunque estén en la misma clase) a través de él.

Tienen un método consultarEventos, podrían refactorizarlo para que sea el getter y usar directamente ese. Un tip, en eclipse si se paran sobre la firma del método y seleccionan Refactor -> Rename o Alt + Shift + R pueden hacer esto muy simple.

11. Sobre los Test, generaron una suite. Lo cual está muy piola, porque ahora la funcionalidad correspondiente a la entrega 1 está agrupada y se puede correr toda junta. Si fue por ese motivo está perfecto. Si fue para poder correr todos los tests juntos, desde junit 4 se puede hacer directamente desde la ide sobre el package o carpeta que tiene los tests.

12. testAgregarComentario está vacío. Teniendo en cuenta que utilizaron TDD esto no debería pasar. Más allá de eso completenlo con un test real sobre este feature.

13. Cuando charlamos de la situación del proyecto con el cliente y le explicamos los avances, el mismo nos confirmó que hubo una malinterpretación y que se pasó agregar algo en la especificación acá va una transcripción de su respuesta:

Los comentarios deberían tener también información sobre quien y cuando los agregó, es algo que agregaría valor de negocio a la solución y me gustaría que se desarrolle en la próxima iteración.

Ok, ya teniendo la entrega 2 avanzada, no les vamos a pedir que hagan una reentrega, pero queremos que cuando entreguen por 2da vez (o si quieren hacer entregas parciales con algunas correcciones tambien pueden) corrijan los siguientes puntos.

2, 3, 5, 8, 12, 13.

Con respecto al 1 (documentación) Armen algun document contando algunas cosas que les parezcan relevantes. Si necesitan ayuda para saber que poner pregunten.

Aprovechando que van a usar Hibernate corrijan el punto 7, saquen la variable static y utilicen un ID de forma correcta. Corrijan el punto 10 también por que les puede traer problemas con este FWK.