search

suumoo5 / dds-monedero-java8

Monedero para refactoring
0 stars 0 forks source link

Comentarios y sugerencias #1

Open flbulgarelli opened 3 years ago

flbulgarelli commented 3 years ago

¡Buenas!

Te dejo nuestros comentarios:

  1. No me parece mal si extraés constantes que se repiten a lo largo de diferentes tests y tienen una semántica clara. Pero en este caso particular ... https://github.com/suumoo5/dds-monedero-java8/blob/aa51c65be97477586dd0e02e4dbaf125f4d64e10/src/test/java/dds/monedero/model/MonederoTest.java#L92-L106 ... en la línea 103 estás comparando contra el valor 800 y quien lean dicho código no entenderá de dónde sale. Por ese motivo, en tests en los que probás cuentas contra valores concretos, es mejor utilizar valores literales más que constantes definidas globalmente para todo el test y que pueden hacer difícil razonar sobre los resultados de las pruebas. En esa misma línea tiene sentido quizás tener constantes como cantidadNegativa o saldoChico y saldoMediano, pero ¿en qué se diferencian semánticamente estas dos constantes https://github.com/suumoo5/dds-monedero-java8/blob/aa51c65be97477586dd0e02e4dbaf125f4d64e10/src/test/java/dds/monedero/model/MonederoTest.java#L27-L28 ?
  2. Hay mucho código repetio en estas líneas ... https://github.com/suumoo5/dds-monedero-java8/blob/aa51c65be97477586dd0e02e4dbaf125f4d64e10/src/main/java/dds/monedero/model/Cuenta.java#L29-L48. Tienen validaciones idénticas, y la última línea es prácticamente idéntica.
  3. Detalle de implementación: para saber si un BigDecimal es negativo, en lugar de cantidad.max(new BigDecimal(0)).equals(new BigDecimal(0)) deberías hacer cantidad.compareTo(BigDecimal.ZERO) < 0
  4. Cuenta.setSaldo se debería eliminar ya que se usa una sóla vez dentro de la clase y expone estado mutable innecesariamente. Alternativamente, podrías dejarlo pero hacerlo privado para explicitar tu intención de que no se use.
  5. No me parece mal reificar como enum el TipoMovimiento pero fijate que de todas formas tenés varios métodos y envíos de mensajes en Cuenta que terminan dependiendo del tipo de movimiento. Si bien no son type-tests de libro, mantienen el concepto de este code-smell, porque no podes dejar de pensar del todo el tipo subyacente. ¿Se podría minimizar su uso de alguna forma?

¡Saludos!

suumoo5 commented 3 years ago

Dale, buen feedback, lo voy a tener en cuenta.