gasparhabif / macowins

0 stars 0 forks source link

Observaciones #1

Open ggallici opened 3 years ago

ggallici commented 3 years ago

Buenas, Te voy marcando un par de cositas para que tengas en cuenta:

  1. La lógica de venta.getFecha().compareTo(dia) == 0 podrías encapsularla como el mensaje esDeLaFecha(dia) dentro de Venta
  2. Venta tiene una colección de Prenda. Esto no está mal, pero podría estar mejor. Imaginate que una venta tuvo 10 camisas nuevas, estaria bueno no tener que instanciar 10 veces la misma clase ya que el hecho de que sean objetos distintos no nos aporta nada y consume memoria al cuete. Una opción válida sería modelar la idea de Item, el cual contenga la prenda y su cantidad vendida. De paso le podríamos dar algunas responsabilidades como darnos su precio //cantidad * prenda.precio()
  3. Fijate que tenes un poco de lógica repetida en los MedioDePago. Se me ocurre que podrías hacer algo asi:
    
    public class Tarjeta implements MedioDePago {
    public double recargo(Prenda unaPrenda) {
        unaPrenda.precio() + 0,01;
    }
    }

public class Efectivo implements MedioDePago { public double precioConRecargo(Prenda unaPrenda) { unaPrenda.precio(); } }

//Despues podrias mandar esta parte del método calcularTotal(...) a Venta o hacer MedioDePago abstracta y subirlo ahi prendas.stream().map(prenda -> precioConRecargo(prenda)).reduce(0.0, Double::sum);`

4. Usar `String` para modelar el tipo de prenda no es la mejor idea. Imaginate que quisiera filtrar "pantalones", tendría que acordarme como es la cadena de caracteres que representa ese tipo ("Pantalones", "PANTALONES", "pantalón", etc) y, peor todavía, repetir esa cadena en cada lugar que quiera filtrar.
Por eso existen los Enums que representan un conjunto acotado de valores (si cursaste pdp hace poco y te acordas algo de wollok, serían algo parecido a los WKO) que te permitirían hacer algo como esto:
```java
public class Prenda {
    ...
    private Tipo tipo;
}

public enum Tipo {
    SACO, PANTALON, CAMISA
}

//para crear una camisa
Prenda prenda = new Prenda(..., Tipo.CAMISA);

//para filtrar pantalones
prendas.stream().filter(p -> p.getTipo().equals(Tipo.PANTALON));
  1. Te recomiendo usar alguna herramienta que te permita hacer diagramas de clases con las flechas que utilizamos en la materia. Por ejemplo plantuml
  2. Hacer teeeeests

Igualmente, tu solución está muy muy bien :partying_face:

gasparhabif commented 3 years ago

Hola @ggallici como va? Ante todo muchísimas gracias por el feedback. Se me habían escapado varios detalles que ya estuve corrigiendo. En cuanto a los tests lo tenia en mente, de hecho si ves mis commits en varios puse "Missing tests", pero la verdad no llegue con el tiempo, y como la entrega de ayer solo era diseñar la solución no me preocupe por demás.

Ahora sí agregue varios, y lo deje en un test coverage de 85% aproximadamente.

También modifique el diagrama y lo hice con la herramienta que me sugeriste, pero la verdad que nunca había hecho este tipo de diagramas. Podrías por favor indicarme si ahí esta bien?

Nuevamente gracias!! Buen finde.