2010C2‎ > ‎Correcciones‎ > ‎

Grupo3 Entrega1

Software Configuration Managment y coding conventions

 

Antes que nada, al ser esta la primera entrega vamos a hacer de cuenta que no pasa nada, pero el proyecto no está buildeando porque no tiene la versión de Java que hay que usar y Maven trata de usar la 1.4 que explota porque por ejemplo no tiene generics.

Así que fijense como decir explícitamente en el pom que quieren usar la version 1.5 o 1.6

 

Segundo tema de SCM, los archivos .classpath y .project son propios del ambiente de la persona que se bajó el proyecto. Como puede haber diferentes personas usando diferentes IDEs o diferentes configuraciones que trabajen sobre el mismo proyecto, estos archivos no suelen comitearse al repositorio de código, sino que se los marca como ignorados.

 

Tercero, veo que estuvieron usando caracteres especiáles (por ejemplo los acentos) y en algún punto se rompió. Esto es bastante común, y puede traer muchos dolores de cabeza. Mi consejo, no usen caracteres especiales en el código ni en los javadoc.

 

Paquetes y codding conventions. Les sugiero que separen sus clases en paquetes para que sea más fácil la lectura. Por ejemplo las excepciones en un paquete, o los mocks en un paquete aparte separado de los tests. Tambien pueden dividir por conceptos del negocio, por ejemplo:

  • edu.utn.tacs.grupo3.dominio.turno
  • edu.utn.tacs.grupo3.dominio.turno

Los tests deberían ir en el mismo paquete de la clase que testean, pero en carpetas separadas para los tests. El código de los tests es código que siempre va separado al del negocio, entonces tener un paquete test no tiene sentido.

 

Por último, pero no menos importante. Pongan javadoc, idealmente en todas las clases y métodos, o si creen que es demasiado, ponganlo en las clases para explicar que son, y en los métodos más importantes o complejos. El javadoc es documentación viva y es clave para poder comunicar ideas en nuestro código, aparte de facilitar muchísimo la corrección.

 

Una pavada, la clase EditingUneditableIndicador es una excepción, pero no termina en *Exception, esta es otra convención que se usa y es bastante práctica.

 

AsignarMaximoMinimoNormal no respeta las codding conventions, debería ser asignarMaximoMinimoNormal.

 

Paciente#getSePresento no respeta las convenciones, debería ser isSePresento()

 

Por último,  Investiguen JUnit 4 que tiene annotations e EasyMock para hacer mocks de forma dinámica, si tienen algún problema con estos 2 frameworks pídannos ayuda.

 

Ahora yendo a cuestiones de código y diseño

 

- Veo que usaron muchas excepciones chequeadas, tuvieron una discusión acerca de usar checked o unchecked? Si no es así piénsenlo, a mi personalmente y a la cátedra en general nos gusta usar excepciones no chequeadas en el 99.9% de los casos, porque creemos que forzar a un cliente de nuestro código a atrapar una excepción es una práctica defensiva que termina siendo molesta y generando un mal manejo de las mismas. Queda a criterio de ustedes usar excepciones chequeadas o no, pero es una decisión de diseño / arquitectura que tiene que estar bien justificada en la documentación.

Por ejemplo Indicador#setValor tira EditingUneditableIndicador, porque querría darle un tratamiento especial en cualquier caso a este escenario? Es una buena idea forzar a quien vaya a usar un indicador a manejar esta excepcion?

 

- Indicador#setValor quedó medio feo, hay que parsear Strings. Quizás yo lo pondría a un nivel más abajo, en las implementaciones concretas. Si voy a setearle un valor a un indicador, se supone que se de que tipo es, entonces puedo tener ese método en cada tipo de indicador, con el parámetro del tipo que corresponda.

 

- El método getValor de Indicador devuelve un String. De vuelta, necesitan tener ese nivel de abstracción? No por el momento. Si llegan a tener que mostrarlo como un String pueden agregarlo, no está bueno verse forzado a convertir todos los tipos de indicadores a un texto. Es un problema que pueden dejar para resolver más adelante dependiendo como se presenten los requerimientos.

 

TestMedico y TestAsignarValoresAIndicadoresDePaciente: El setup ejecuta cosas que se podrían hacer 1 vez. No se rompe porque pone objetos en un set, pero no está bueno.

Haganlo una vez por test, o creen todos los objetos de 0 en el setup.

 

TestMedico#testAsignarAUnAnalisisUnMedico y TestCrearTurno#testAsistioAlTurno están testeando un getter y un setter? Este tipo de tests no agregan valor, traten de evitarlos o si quieren testear lógica del negocio modifíquenlo.

 

Hay una interfaz AnalisisHelper. Cual es el sentido de la misma? Porque no usan directamente Analisis? El nombre no dice mucho, y la documentación tampoco. Si deciden que quieren una interfaz ahí, ponganle un nombre más claro y justifíquenlo en la documentación o en el javadoc.

 

Los métodos de Medico tienen parámetros con nombre analisisDeSangre. Supongo que esto fue un error causado por la IDE que creó el método en base a un test. Corríjanlo por un nombre más acorde.

 

Porque llamaría al método Medico#isAnalisNormal(analisis) si puedo decir analisis.isNormal()? Tiene sentido ese método en Medico? Cuidado con modelar la realidad a ese nivel.

 

- Porque Calendario tiene SingletonHolder y no INSTANCE directamente?

 

- Porque Estudio e Indicador implementa comparable? Necesitan un orden natural? O es un problema de la capa de presentación si hay que mostrarlo en algún momento con un orden en particular?

 

- Está bien como usaron el patrón prototype para clonar los estudios con sus indicadores, pero testeenlo más unitariamente, a nivel de cada objeto que se clona.

 

- El método Estudio#getIndicador(String unNombre) es un método helper que podría no estar. En caso de que en algún momento la aplicación necesite buscar un indicador por nombre, la clase que necesite eso, puede pedirlos todos y buscar ella misma. Están resolviendo problemas que todavía no se les presentaron y quizás no se presenten, ojo con esto.

 

-  Algunos métodos en su firma aclaran que tiran Exception. Esto no tiene sentido, quiere decir "Mirá que yo puedo tirar excepciones de cualquier tipo", para hacer algo así, tan genérico, es mejor usar excepciones no chequeadas.

 

- Si le pregunto a un indicador si es normal, y el indicador no está cargado, debería devolver false? Es la respuesta que estoy esperando o es un caso excepcional? Que representa ese false? Que no es normal o que no está cargado?

 

- No comparen strings con ==. Usen equals, por ejemplo en: IndicadorTexto#isNormal()

 

- La clase ListaAnalisisTipo es una solución a un problema que todavía no se les presenta, que es el de acceder a os análisis que tienen creados. Esto lo pueden resolver más adelante dependiendo de la arquitectura que tengan. Por ejemplo si están guardados en una base de datos pueden usar un DAO (Data Access Object) . Igual tener un punto en el dominio que sirva para eso  (Es un repositorio de datos) no está mal, solo que no es necesario por ahora.

 

- No hace falta sobreescribir clone si no se usa. Aunque sea un Singleton, es ponerse demasiado defensivo, y de última usando reflection y alguna otra hackeada se puede obtener otra copia de cualquier clase. Es medio filosófico, pero está bueno pensar que el que va a usar nuestro código es una persona responsable que no lo va a usar de forma maliciosa, de última si hace algo que sabe que está mal (como clonar un singleton) el sistema funcionará mal para él.

 

- Porque no hacen el constructor de IndicadorNumerico con el nombre, el mínimo y el máximo? Sería más práctico. Aparte, que pasa si me olvido de llamar al método AsignarMaximoMinimoNormal?

 

- El Paciente tiene una colección de análisis pero un solo turno? Hay algo raro en como diseñaron esto. Cuantas instancias del paciente "Juan Perez" existen en el sistema? Una o muchas? Depende como lo planteen ambas son válidas, pero no se termina de entender como se relacionan con el resto de los objetos.

 

- Para los días de la semana pueden usar una Enum, usar Strings siempre tiene que ser la ultima opcion. Imaginense que en vez de "Lunes" pongo "lunes" o "Lnues", ya es un problema.

 

- Que pasa si agrego una semana? Tengo que cargar todos los turnos de nuevo? El requerimiento dice: "quiero poder crear los turnos disponibles para un determinado día de la semana. A partir de ese día, todos las subsiguientes semanas tienen esos turnos disponibles en ese día" Entonces, como manejo las semanas? cuando creo una? Como es el ciclo de vida de un objeto semana?

 

- IndicadorTexto tiene un metodo AsignarMaximoMinimoNormal, porque se llama maximo minimo si es texto?

 

- La clase EstudioBuilder no me alcanza para armar un estudio. Como elijo indicadores de texto? como seteo los valores normales de esos indicadores? Tiene sentido EstudioBuilder? Justifíquenlo.

 

- TestResultadoAnalisis prueba Analisis y Estudio. No está mal, pero sería más prolijo un test para Análisis (que use estudios mock) y un test para estudios (que use observers  mock).

 

- Con respecto a los mocks, vi que estuvieron usandolos mucho. Cuidado con hacer abuso. Por ejemplo TestMedico#testNotificarAnalisisListo usa todos objetos mock. Que es lo que están probando? Si es un mock, como lo prueban? Es necesario que todo sea mock?

Por lo general se  mockean los objetos que actúan directamente con el objeto que se quiere testear. Por ejemplo si quiero testear médico puedo mockear análisis y testear la verdadera clase Medico. No deberían tener tantas estructuras de objetos mock en un mismo test.

 

Bueno, eso es lo que vimos, el resto está bien y al parecer cumple con los requerimientos planteados.

Necesitamos que corrijan los temas más importantes de esta lista, especialmente los de diseño. Hagan una reentrega antes de hacer la entrega 2.

Comments