Correcciones y sugerencias

flbulgarelli commented 3 years ago

¡Buenas!

Te dejo @EMazzaglia te dejo nuestros comentarios:

¡Bien por animarse a escribir tests para QMP! Aprovecho para hacerte algunos comentarios al respecto:
1. Aunque uses @DisplayName, poné nombres de métodos significativos en lugar de test1 y así
2. Un test debe tener siempre algún tipo de aserción. Por ejemplo acá ... https://github.com/mnigliazzo/qmp-grupo10/blob/5f89f971639ef0ad6b41401bd37c64e0a5ef5bf3/src/test/java/tests/service/ClimaServiceTest.java#L37-L40 ... debería haber al menos un assert
3. Si bien acá hay una aserción ... https://github.com/mnigliazzo/qmp-grupo10/blob/5f89f971639ef0ad6b41401bd37c64e0a5ef5bf3/src/test/java/tests/service/ClimaServiceTest.java#L33 ... no es muy fuerte, dado que lo único que está validando es que no lance excepción (que es básicamente lo mismo que nada, porque por defecto si una excepción es lanzada, el test falla).
  1. Si bien en clase ya mencionamos que no recomendamos implementar abstracciones mediante clases anidadas, si lo hacés sugiero no repetir el nombre de la abstracción raiz porque se vuelve redundante. Por ejemplo acá ... https://github.com/mnigliazzo/qmp-grupo10/blob/5f89f971639ef0ad6b41401bd37c64e0a5ef5bf3/src/test/java/tests/service/ClimaServiceTest.java#L23-L26 ... esperaría ver Prenda.Builder en lugar de Prenda.PrendaBuilder. PrendaBuilder sería un mejor nombre para una clase sin anidar. Más allá de eso, en cualquier caso creo que será mejor contar con una abstracción con un nombre de dominio, como Borrador.
  2. No tiene mucha utilidad tener una interfaz PrendaRepository y una implementación concreta si luego vas a tipar las cosas según la implementación como acá .... https://github.com/mnigliazzo/qmp-grupo10/blob/5f89f971639ef0ad6b41401bd37c64e0a5ef5bf3/src/test/java/tests/service/ClimaServiceTest.java#L21. Recomiendo que no hagas esa separación (en este punto no aporta porque no hay mas de una implementación posible del repositorio) o que si avanzás con ese (pequeño) sobrediseño, tipes los atributos de forma consistente.
  3. Viendo el código de esta clase .... https://github.com/mnigliazzo/qmp-grupo10/blob/5f89f971639ef0ad6b41401bd37c64e0a5ef5bf3/src/main/java/service/impl/PrendaRepositoryImpl.java#L17-L35 ... no me queda claro como deberían implementarse esos métodos.
4. :up: Ahora que veo el README entiendo que allí debe obtener las prendas que cumplan la restricción de temperatura. Pero allí veo un problema: ¿por qué lo hace el repositorio de prendas? ¿Es un objeto global y bien concido (por definición los repositorios lo son)? En etapas anteriores hablamos de guardarropas, que son justamente los objetos que agrupan las prendas de une usuarie, por lo que las sugerencias no deberían ser hechas globalmente sino a partir de los guardarropas de cada persona. En otras palabras, yo debería poder pedirle a cada guardarropas que me de sugerencias, y cada uno debería usar sólo las prendas que conoce.
5. Más allá de las cuestiones particulares de este dominio, en general los repositorios tendrán lógicamente sólamente asociada al registro general de los objetos de un cierto tipo, y sus operaciones se limitarán a búsquedas, eliminaciones y agragados, y no implementarán lógica de dominio.
6. Debería ver un poco más de código como para poder terminar de darte una devolución sobre este aspecto. En particular el método obtenerPrendaRandom me suena a que terminará siendo un missplaced method, pero me falta información.
Un detalle de código....

 @Override
    public Atuendo sugerirAtuendo() {
        Temperatura temperatura = climaService.getTemperature("Buenos Aires, Argentina");
        Atuendo atuendo = obtenerAtuendoSegunTemperatura(temperatura);
        return atuendo;
    }

... ojo con generar variables que no se reutilizan y generan por tanto confusión. Eso mismo deberías escribirlo de forma más directa: `return obtenerAtuendoSegunTemperatura(climaService.getTemperature("Buenos Aires, Argentina"))

Entiendo que https://github.com/mnigliazzo/qmp-grupo10/blob/emazzaglia-qmp4/src/main/java/service/impl/ClimaServiceImpl.java no pretende ser "real" sino un objeto que simula ser la verdadera API de AccuWeather. En ese caso, estamos en presencia de un stub, que no debería ser parte del código productivo (porque lisa y llanamente no lo es) sino del código de test, y ponerlo en consecuencia dentro de src/test/java. Opcionalmente podrías implementarlo usando Mockito (ver el TPI2, que tiene varios links, entre ellos este con un resumen de sus operaciones principales)
Es cierto que para esta entrega podríamos asumir que el problema de QMP3 (las sugerencias) estaba resuelto en un componente, pero me llama la atención la firma del método que usaste: https://github.com/mnigliazzo/qmp-grupo10/blob/5f89f971639ef0ad6b41401bd37c64e0a5ef5bf3/src/main/java/service/AtuendoService.java#L5-L7. Difícilmente pueda sugerir algo sin nada de contexto, ¿no? Debería tener en cuenta el clima, le usuario y/o su guardarropa. De lo contrario, éste objeto debería ser quien interactué con todos los demás componentes y pasaría a ser un objeto que resuelva mucho más que sólo generar combinatorias posibles de prendas.
Entiendo que tu ClimaService es tu interfaz de alto nivel para interactuar con AccuWeather o proveedores similares. Sin embargo mirá estas firmas: https://github.com/mnigliazzo/qmp-grupo10/blob/5f89f971639ef0ad6b41401bd37c64e0a5ef5bf3/src/main/java/service/ClimaService.java#L10-L12. Si bien getTemperatura es un mensaje de alto nivel, getWeather sigue representando al clima como una lista de diccionarios, probablemente con la misma estructura del sistema AccuWeather adaptado, acoplándose implícitamente a sus interfaces.
Esto viene de entregas pasadas: :eyes: ojo que tu clase Atuendo expone getters que no parecen ser necesarios y hacen mutable a tu objeto innecesariamente. Lo mismo vale para tu clase Prenda
Detalle de implementación: acá .... https://github.com/mnigliazzo/qmp-grupo10/blob/5f89f971639ef0ad6b41401bd37c64e0a5ef5bf3/src/main/java/modelo/prenda/Prenda.java#L127-L136 .... no es necesario utilizar else, dado que la excepción corta el flujo de ejecución y podes obviarlo.
Sobre ésto .... https://github.com/mnigliazzo/qmp-grupo10/blob/5f89f971639ef0ad6b41401bd37c64e0a5ef5bf3/src/main/java/modelo/clima/Temperatura.java#L6-L8 .... coincído en que deberías enumerarlo. Más allá de eso, el nombre no me deja en claro que representa. Y nuevamente quitaría los setters innecesarios y los reemplazaría por un constructor que tome los atributos por parámetro.
1. :up: Ahora entiendo que representa la unidad de medida. Eso no es el "tipo de temperatura", sino, justamente, la unidad.
Por último, sobre tu justificación ....

Este punto, ya lo explique un poco mas arriba pero al ser una interfaz, si yo cambio la API del clima, solo tengo que preocuparme por cambiar la implementacion del ClimaServiceImpl y que ese cambio siga devolviendo algo de tipo Temperatura. Esta muy desacoplada la solucion con este enfoque. Por lo que creo que realizar un cambio a otra API facilmente es posible.

... si bien esto es mayormente cierto (con la salvedad del punto 8) no terminás de responder cómo harás para probar el código. Por mas que sea una interfaz, si no explicás qué mockearás (y cómo lo harás) no terminará de quedar respondida esta cuestión.

¡Saludos!

EMazzaglia commented 3 years ago

Hola @flbulgarelli gracias por el review! No había tenido tiempo de sentarme a digerir toda la info. Paso a comentarte algunas cosas sobre la revisión.

Respecto a los tests, no los iba a escribir y la realidad es que iba a borrarlos porque me acorde que no hace falta escribir código que compile en los diseños. De todas maneras gracias por el consejo del nombre de los métodos eso si lo tomo, al igual que al menos un assert en el test 👯
Respecto de las clases anidadas, lo deje porque no entraba en esta iteración pero nunca mas hago clases anidadas lo juro. Aparte no le da tanta claridad al diagrama como si los tuvieras separados. Tomo la sugerencia de los nombres del builder tambien.
Acá viene algo muy interesante que aprendí en la clase del viernes pasado. NO HAGAMOS INTERFACES CUANDO TODAVIA NO TIENEN SENTIDO. Y me pareció excelente, venia con una malacostumbre del laburo de crear interface para cualquier clase y esta mal a nivel diseño es un sobre diseño. Respecto al tipado si, hay que tipar según la interfaz 📄
El PrendaRepository esta mal, fue una mala interpretación mia del enunciado. El viernes cuando vimos lo de que la app es para usuario dije "que tonto claro" no tenes un PrendaRepository global acá, no tiene sentido, tenes un guardarropa por usuario. Gracias por todas las explicaciones y con la clase del viernes pasado me quedo muchisimo mas claro.
Respecto a 5
```
@Override
public Atuendo sugerirAtuendo() {
    Temperatura temperatura = climaService.getTemperature("Buenos Aires, Argentina");
    Atuendo atuendo = obtenerAtuendoSegunTemperatura(temperatura);
    return atuendo;
}
```
Esto lo escribo así porque me es mas fácil debuggear y para mi le da mas claridad al código (construyo la respuesta paso a paso). Pero si a la catedra le gusta mas de la otra forma no hay problema, me adapto.
El 6 esta bien, tenia que aplicar un Adapter ahi, pero no habia leido el apunte antes de codear el TP.
Otra vez lo que me paso en el PrendaRepository, el enunciado lo interprete como que existia un solo usuario entonces tenia objetos globales que resolvian todo para uno solo. Cualquier cosa hice.
Respecto del 8, si vuelvo a que me falto entender bien lo del adapter. Podria haber hecho getTemperature() solo y luego que la implementación de ese servicio sea el AccuWeather con los cambios para que devuelva una temperatura.

9 y 10 Si, genial por las recomendaciones, lo de los if-else no lo habia implementado yo pero quedo de iteraciones pasadas.

Viendo lo que me sugeris en el punto 11, medio que responde la duda de arriba, la idea es usar constructores que me permiten instanciar una prenda nueva y ya, asi como un TipoTemperatura o con un nombre mas lindo unidadTemperatura.
En la justificacion me falto decir que mockeando el servicio getWeather puedo no incurrir en gastos dado que no voy a estar yendo a la api sino a un mock.

CONSULTAS:

Respecto de las prendas mutables tenia una duda que vengo arrastrando hace un par de clases. Como seteo el contexto de mi test si no puedo modificar el estado de una prenda? Creo prendas nuevas y ya?
El diagrama pudiste verlo? Tendras algun feedback, porque por mas que este mal, quiero saber si al menos esta comunicado bien.

Muchas gracias por la corrección.

Saludos Emi

flbulgarelli commented 3 years ago

Esto lo escribo así porque me es mas fácil debuggear y para mi le da mas claridad al código (construyo la respuesta paso a paso). Pero si a la catedra le gusta mas de la otra forma no hay problema, me adapto.

No es tanto una cuestión de gustos sino que no es necesario. Además, probablemente cuando vayas a hacer debugging muchas veces var a tener que hacer más modificaciones al codigo para poder realmente recorrerlo paso a paso (desde logs, hasta modificaciones drásticas de la lógica), y no por ello va a quedar en el código final. Además, siempre podrás utilizar herramientas como un evaluador de expresiones o un watch para obtener más información del código en ejecución, aún cuando no tenés variables.

Respecto de las prendas mutables tenia una duda que vengo arrastrando hace un par de clases. Como seteo el contexto de mi test si no puedo modificar el estado de una prenda? Creo prendas nuevas y ya?

¡Exacto! Podés crear esas prendas en un @Before si son realmente comunes a todos tus casos de prueba, y si no, en el propio método de test.

El diagrama pudiste verlo? Tendras algun feedback, porque por mas que este mal, quiero saber si al menos esta comunicado bien.

Sí, ¡lo veo bien!

mnigliazzo / qmp-grupo10

Correcciones y sugerencias #7