search

ealon2 / dds

Diseño de Sistemas
0 stars 0 forks source link

Correcciones y sugerecias #1

Open flbulgarelli opened 3 years ago

flbulgarelli commented 3 years ago

¡Buenas!

Te dejo nuestros comentarios:

  1. El directorio /target contiene archivos autogenerados (clases compiladas) y por tanto no debe ser subido al repositorio de código. Para resolver esto, deberías eliminarlo de git y luego agregarlo en el archivo .gitignore
  2. Cuestión de nombres: esta interfaz se llama Clima ... https://github.com/ealon2/dds/blob/91a19434dd1615a4697a047813530962e5ecf900/src/main/java/qmp/model/Clima.java#L3-L5 ... pero luego tiene un mensaje llamado getWeather que se traduce directamente como obtener clima, es decir que coloquialmente le estamos pidiendo el clima al clima. ¿Tiene sentido en español eso? Te propongo que renombres esta interfaz a ServicioDeClima, ServicioMetorologico, Meteorólogo, etc.
  3. Por ahora no nos preocuparemos por la interfaz HTTP del sistema. Si de todas formas querés ir chusemando, podés probar Spark que es lo que usaremos.
  4. Bien por hacer tests :clap:. Al respecto no es necesario que la prenda ... https://github.com/ealon2/dds/blob/91a19434dd1615a4697a047813530962e5ecf900/src/test/java/qmp/model/GuardarropaTest.java#L15-L16 ... sea un @Mock porque es un objeto fácil de crear y que no tiene dependencias externas.
  5. Intencionalmente en la materia no vamos por el camino de distinguir artificialmente entre Services y DAOs https://github.com/ealon2/dds/blob/91a19434dd1615a4697a047813530962e5ecf900/src/main/java/qmp/dao/UsuarioDAO.java#L7. Te recomiendo no aplicar esas ideas (que en general agregan complejidad innecesaria) y a lo sumo si necesitamos un lugar donde tener todas las instancias de una clase, modelar un repositorio.
  6. :eyes: Ojo, acá ... https://github.com/ealon2/dds/blob/91a19434dd1615a4697a047813530962e5ecf900/src/main/java/qmp/model/AccuWeather.java#L13-L17 ... estás asumiendo que AccuWeather (el objeto nativo) es capaz de contener un objeto EstadoClima, que es una abstracción de tu dominio, no del de AccuWeather. Esto en la realidad no sería posible: AccuWeather te dará otros objetos, y vos los tendrás que transformar y adaptar.
  7. Sugerimos evitar usar nombres de patrones para identificar nuestras abstracciones, como por ejemplo https://github.com/ealon2/dds/blob/91a19434dd1615a4697a047813530962e5ecf900/src/main/java/qmp/model/SugerenciaCommand.java#L3. ¿Por qué? Porque rara vez los objetos siguen la estructura de un patrón de libro, y por otro lado, porque ponerle el nombre del patrón nos hará pensar más en este que en el concepto de dominio, limitándonos nuestra capacidad de análisis del dominio y dificultando la búsqueda de una terminología más cerca a quienes usan el sistema. Si querés profundizar más sobre esto, recomiendo leer el primer capítulo del libro Domain Driven Design de Eric Evans.
  8. Me resulta muy extraño que tus atuendos conozcan una lista de usuarios. ¿Qué representan estos usuarios? Pensá que un atuendo es la combinación de ciertas prendas, entonces, ¿cómo podría un conjunto de prendas "tener un dueño"? Y aún si entendemos al usuario del atuendo como la persona que lo usa, ¿como podría ser usado por muchos usuarios? No le encuentro sentido.
  9. Relacionado con lo anterior, no encuentro una forma de leer en español estos mensajes: https://github.com/ealon2/dds/blob/91a19434dd1615a4697a047813530962e5ecf900/src/main/java/qmp/model/Atuendo.java#L22-L30. ¿Qué significa atuendo.add(usuario)? Un mensaje con nombre tan genérico como add tendría sentido para por ejemplo una colección (cuya acción primaria es la de agregar y quitar cosas), pero no para un atuendo. Tenga o no tenga sentido esta responsabilidad en el Atuendo, casi con seguridad el mensaje está mal llamado.
  10. En la misma línea, si bien tiene un nombre un poco más expresivo https://github.com/ealon2/dds/blob/91a19434dd1615a4697a047813530962e5ecf900/src/main/java/qmp/model/Atuendo.java#L32-L35, un atuendo no se "comparte" con un guardarropas; más bien se sugiere el agregado de una prenda al mismo. Nuevamente, tené en cuenta además que un atuendo es un objeto que es generado a partir de las prendas, y no creado a mano por une usuarie.
  11. No me termina de quedar claro qué haría este método, o si debo considerarlo, dado que está comentado: https://github.com/ealon2/dds/blob/91a19434dd1615a4697a047813530962e5ecf900/src/main/java/qmp/model/Atuendo.java#L37-L39
  12. ¿Por qué un atuendo saber decirte el clima? ¿Tiene sentido en español esto? Y más importante aún: ¿aporta el clima algún comportamiento, estado o polimorfismo que le sea propio y que sea necesario para la obtención del clima? Por lo que veo .... https://github.com/ealon2/dds/blob/91a19434dd1615a4697a047813530962e5ecf900/src/main/java/qmp/model/Atuendo.java#L18-L20 .... la única justificación estructural que se me ocurre es que el Atuendo tiene un Clima, pero hasta éste es una dependencia global que obtenés a través de un singleton y que por tanto se podría utilizar en cualquier parte del sistema.
  13. Cuidado con agregar clases vacías como ésta: https://github.com/ealon2/dds/blob/master/src/main/java/qmp/model/EstadoClima.java. Es importante saber qué estado buscás poner allí para que podamos evaluar si la existencia de esta clase está justificada o no.
  14. Como charlamos hoy en clase, el criterio ... https://github.com/ealon2/dds/blob/91a19434dd1615a4697a047813530962e5ecf900/src/main/java/qmp/model/Guardarropa.java#L16 ... probablemente sea algo que existe en la mente de quien use al sistema, y ni siquiera sea necesario contar con un string que lo tipifique.
  15. La interfaz de observable ... https://github.com/ealon2/dds/blob/91a19434dd1615a4697a047813530962e5ecf900/src/main/java/qmp/model/Observable.java#L6-L15 ... no expone mensajes que encajen con la semántica de algo que puede ser "observador" (sea lo que sea que eso signifique). Yo acá calculo que quisiste implementar una noción de patrón observer, ¿puede ser? Más allá de que aún no lo trabajamos, un objeto con una interfaz "observable" exponse mensajes que le permite agregar observadores que exponen una interfaz muy genérica, y cuenta con algún mecanismo para notificarlos a todos ellos. Sin embargo, acá tu interfaz "observadora" sería la del propio Usuario, que lejos de ser un concepto genérico y abstracto, es una entidad muy concreta en tu dominio.
  16. Acá .... https://github.com/ealon2/dds/blob/91a19434dd1615a4697a047813530962e5ecf900/src/main/java/qmp/model/Prenda.java#L14-L28 ... tenés tres constructores, pero uno carece de parámetros y (dado que no hay forma de mutar al objeto una vez creado) no permitirá crear objetos útiles, mientras que el segundo repite código que está en el último. Acá lo que te propongo es que borres el constructor vacío y el segundo constructor lo implementes delegando en el último.

¡Saludos!

flbulgarelli commented 3 years ago

PD: Veo que en Usuario agregaste una lista para llevar el registro de las propuestas ... https://github.com/ealon2/dds/blob/91a19434dd1615a4697a047813530962e5ecf900/src/main/java/qmp/model/Usuario.java#L12-L13 ... pero también que tenés modelado al Guardarropas y que tenés múltiples de ellos por usuarie. Me pregunto entonces por qué si las acciones de agregar o quitar son siempre respecto a un guardarropas particular ....

Como usuarie de QuéMePongo, quiero que otro usuario me proponga tentativamente agregar una prenda al guardarropas.

...entonces agregás las sugerencias al usuario, perdiendo el contexto de dónde agregar las cosas. Aún si los objetos propuesta tuvieran una referencia al guardarropas afectado, sería más complejo eso que dotar al guardarropas de su lista de sus propuestas.

Por otro lado, cuidado con agregar anotaciones tecnológicas a la clase Usuario. ¿Para qué tenés ésto https://github.com/ealon2/dds/blob/91a19434dd1615a4697a047813530962e5ecf900/src/main/java/qmp/model/Usuario.java#L19? Me imagino que esto viene de la mano de que empezaste a implementar una interfaz HTTP, pero estás volviendo al código de dominio más complejo, con dependencias de bibliotecas externas, y en este momento no es necesario. Quizás, dependiendo de cómo expongamos nuestro código en la presentación, éstas no sean necesarias

Por último, dado que ésto es un Mock ... https://github.com/ealon2/dds/blob/master/src/main/java/qmp/api/AccuWeatherAPI.java ... no debería formar parte del código productivo, sino que debería estar en el directorio de test: src/test/java.

ealon2 commented 3 years ago

Buenas Lean! Antes que nada muchas gracias por tomarte tiempo en realizar las correcciones. Estas semanas estuve corriendo con un deploy que tuve que hacer en el laburo y no pude dedicarle el tiempo que queria revisarlo como lo amerita hasta hoy. Con respecto a los correcciones:

  1. Listo. Agregue el .gitignore (se me escapo)
  2. Corregido las firmas de los métodos. Es verdad, voy a usar ESP.
  3. Sip, en donde laburo (Mercado Libre) usan Spark. Ahi lo cambie por Jersey igual dado que lo estuve chusmeando para la entrega del TPI.
  4. Ok
  5. Cierto, me quedo el @mock para una prueba que pensaba hacer y no hice al final.
  6. Es verdad. Me quedo esa entidad EstadoClima para la API, la cual esta mal dado que obtendría un ResponseDTO (con el nombre correspondiente).
  7. Listo. Si, me acuerdo tu comentario en la Clase 8 al respecto, de no ponerle a las clases los nombres de los patrones.
  8. Lo reviso. La idea que tenia en mente es en Atuendo mas como un Notificador de Sugerencias con respecto a los Atuendos. De ahi, el uso del patro Observer.