search

peyk0v / DDS-2021

tps/pruebas/ejs
0 stars 0 forks source link

Correcciones y sugerencias #3

Open flbulgarelli opened 3 years ago

flbulgarelli commented 3 years ago

¡Buenas!

Te dejo nuestros comentarios:

  1. Cuanto más próximo sea el pseudocódigo al código, mejor. En particular, acá ... https://github.com/NPeykov/DDS-2021/blob/3e8e6fa3749c67d316e4b63a1df7a4de341701f9/TP%20-%20QMP/pseudocodigo.java#L1-L6 ... no queda del todo claro qué relaciones tiene esta clase con el resto del sistema, ni exactamente el tipo del parámetro del método sugerirAtuendo. Si, es cierto que se puede inferir algo de todo esto por el nombre del parámetro y el diagrama de clases (:+1:) pero todavía hay algunos huecos:
    1. ¿prendas es una lista de prendas? ¿Es un set?
    2. ¿Por qué el Guardarropas recibe las prendas por parámetro si según el diagrama de clases tiene una relación fuerte con una lista de prendas? ¿Estamos hablando de la misma lista de prendas del parámetro u otra? ¿Hay un atributo o es una flecha incorrecta o una inconsistencia del diagrama?
    3. ¿Cómo obtiene al GeneradorDeAtuendos? ¿Se inyecta?
    4. ¿Qué comportamiento delega exactamente en el GeneradorDeAtuendos? No está dicho ni en el diagrama ni en el código cuales son los mensajes que éste entiende. Tampoco queda claro cuales son sus responsabilidades. ¿Qué le compete al guardarropas y qué al generador? ¿Por qué no son lo mismo?
    5. ¿Cómo es el diálogo entre Guardarropa, Generador y ServicioDeClima? ¿En qué momento se utiliza la temperatura? ¿Cuántas veces?
  2. No me queda claro por qué GeneradorDeAtuendos tiene un setter ... https://github.com/NPeykov/DDS-2021/blob/3e8e6fa3749c67d316e4b63a1df7a4de341701f9/TP%20-%20QMP/pseudocodigo.java#L12-L16 ... en lugar de inyectar por constructor su dependencia.
  3. Relacionado con el punto 1. el GeneradorDeSugerencias que planteamos en el mail Aclaraciones QMP4 tiene una interfaz similar (si interpreto tu código) , pero vos acá dotaste a ese componente de una relación con el sistema de clima y de nuevas responsabilidades que originalmente no le eran propias. Si realmente es un componente ya desarrollado, cerrado, del cual no vamos a asumir su implementación y hasta incluso sea externo, no podemos agregarle dependencias o responsabilidades, porque eso implicaría que cambiamos el code ownership, pasa a ser nuestro y ya no podemos asumir que la implementación no nos importa y es irrelevante. En definitiva, si vas a asumir que el GeneradorDeSugerencias ya existe y genera sugerencias basadas en combinatorias, no debés modificar ese comportamiento, debés asumir que está "cerrado". En caso contrario, esperaremos que desarrolles completamente el algoritmo o al menos con el suficiente detalle como para que se entienda como interacciona con el resto de los componentes.
  4. :eyes: Ojo, acá ... https://github.com/NPeykov/DDS-2021/blob/3e8e6fa3749c67d316e4b63a1df7a4de341701f9/TP%20-%20QMP/pseudocodigo.java#L34-L40 .... justamente es relevante lo que haga getClima, porque tenés que demostrar que esa adaptación es posible. ¿Cómo convierte a la lista de diccionarios en ese double? No es un mero detalle.
  5. Esto quedó de entregas pasadas: tu clase Material .... https://github.com/NPeykov/DDS-2021/blob/3e8e6fa3749c67d316e4b63a1df7a4de341701f9/TP%20-%20QMP/pseudocodigo.java#L118-L122 ... expone un método que muta a la misma, pero el mismo en principio no necesita ser mutable para que el sistema funcione. Por ese motivo, en lugar de especificar la trama mediante un setter, debería ser inyectado por constructor. Lo mismo vale para TipoPrenda: https://github.com/NPeykov/DDS-2021/blob/3e8e6fa3749c67d316e4b63a1df7a4de341701f9/TP%20-%20QMP/pseudocodigo.java#L89-L91
  6. En éste método ... https://github.com/NPeykov/DDS-2021/blob/3e8e6fa3749c67d316e4b63a1df7a4de341701f9/TP%20-%20QMP/pseudocodigo.java#L65-L78 ... hay dos cosas para observar:
    1. Estás haciendo una validación múltiple: eso lo que implica es que cuando alguna de las condiciones falla, una excepción genérica será lanzada, sin saber realmente cual fue la causa. Sugiero que escribas tres validaciones independientes, para material y colorPrimario
    2. No es necesario utilizar elses, dado que la excepción corta el flujo de ejecución y podes obviarlo. Lo mismo aplica acá: https://github.com/NPeykov/DDS-2021/blob/3e8e6fa3749c67d316e4b63a1df7a4de341701f9/TP%20-%20QMP/pseudocodigo.java#L60-L62
    3. Más allá de proponer una estructura de clase y (pseudo)código, sería deseable que explicites cómo harás para testear el código del clima, dado que es un requerimiento explícito de QMP4.

¡Saludos!

flbulgarelli commented 3 years ago

PD: otra cosa que no queda claro es qué es exactamente una ciudad (¿es un string? ¿un número? ¿un objeto de alto nivel?) y sobre todo cómo se la obtiene (en ningún lugar se explicita quién conoce a la ciudad que usaremos con el WeatherService)

peyk0v commented 3 years ago

Buenas. Gracias por el feedback. Es verdad, me descuidé bastante con el código. Seguramente arme un nuevo repo para arrancar de cero teniendo todo mas ordenado y aplicando todo esto que dijiste. Saludos!