Revisión de código: inmutabilidad y otros básicos

 

Code review

Me gustaría compartir un pequeño análisis sobre una clase de código Java que he encontrado en un proyecto cualquiera del ecosistema en el que sobrevivo.

El objetivo es exponer un enfoque práctico de algunos conceptos clave de DDD, promoviendo la mejora del código inicial

El código base a revisar:

public class SampleType extends Expirable {

  private SampleTypeId id;

  private SampleTypeDescription description;

  private SampleTypeCode code;

  private SampleType(SampleTypeId id, SampleTypeDescription description, SampleTypeCode code,
      ExpirationDate expirationDate) {
    super(expirationDate);
    this.id = id;
    this.description = description;
    this.code = code;
  }

  public static SampleType of(SampleTypeId id, SampleTypeDescription description, SampleTypeCode code,
      ExpirationDate expirationDate) throws DomainException {
    if (id == null || id.getValue() == null) throw NullArgumentException.of(Constants.SAMPLE_TYPE, Constants.ID);
    if (code == null || code.getValue() == null) throw NullArgumentException.of(Constants.SAMPLE_TYPE, Constants.CODE);
    return new SampleType(id, description, code, expirationDate);
  }

  public static SampleType create(SampleTypeDescription description, SampleTypeCode code) {
    return new SampleType(null, description, code, null);
  }

  public static SampleType update(SampleType sampleType, SampleTypeDescription description, SampleTypeCode code) {
    if (description != null) {
      sampleType.description = description;
    }
    if (code != null) {
      sampleType.code = code;
    }
    return sampleType;
  }

  public SampleType expire(ExpirationDate expirationDate) {
      return new SampleType(this.id, this.description, this.code, expirationDate);
  }

  public SampleTypeId getId() {
    return id;
  }

  public SampleTypeDescription getDescription() {
    return description;
  }

  public SampleTypeCode getCode() {
    return code;
  }

}

El código tiene una base sólida y se nota que hay una intención clara de seguir los principios de DDD. A continuación, te presento una revisión detallada, destacando los puntos fuertes y las áreas donde se puede mejorar para alinearse aún más con un modelo de dominio rico.

Resumen General

El código utiliza conceptos clave de DDD como los Value Objects (asumo que SampleTypeId, SampleTypeDescription, etc., lo son) y los Factory Methods para controlar la creación de objetos. Sin embargo, hay una inconsistencia en el manejo del estado (mutabilidad vs. inmutabilidad) y una oportunidad perdida para hacer la entidad más responsable de su propio comportamiento.

Puntos Fuertes (Lo que está bien)

  1. Value Objects (VOs): El uso de clases como SampleTypeId, SampleTypeDescription y SampleTypeCode en lugar de tipos primitivos (long, String) es una práctica excelente en DDD. Encapsula la validación y el significado de cada atributo. ¡Muy bien hecho!
  2. Constructor Privado: Forzar la creación de la entidad a través de métodos estáticos (Factory Methods) es la forma correcta de hacerlo. Esto te da control total sobre el proceso de instanciación.
  3. Intención de Inmutabilidad: El método expire() que devuelve una nueva instancia es un patrón fantástico. Promueve la inmutabilidad, lo que hace que el estado del dominio sea más predecible y seguro, especialmente en entornos concurrentes.
  4. Separación de Creación vs. Reconstitución: Tener un método create (para nuevos objetos) y un método of (para reconstruir objetos existentes desde persistencia por ejemplo) es una distinción muy útil y clara.

Áreas de Mejora (Puntos a Refinar)

Aquí es donde podemos hacer que el código sea aún más robusto y alineado con DDD.

1. El método update rompe la encapsulación (El punto más importante)

Este es el principal problema del diseño actual.

  • Problema: El método public static SampleType update(...) es un procedimiento que opera sobre la entidad SampleType desde fuera. Toma una instancia, la modifica directamente (sampleType.description = description;) y la devuelve. Esto es característico de un Modelo de Dominio Anémico, donde las clases del dominio son solo contenedores de datos y la lógica de negocio reside en clases de servicio.
  • Impacto en DDD: En un Modelo de Dominio Rico, la entidad debe ser responsable de sus propias transiciones de estado. Debería proteger sus invariantes y exponer comportamiento, no datos para ser modificados por otros.
  • Solución: Reemplaza el método estático update por métodos de instancia que expresen la intención de negocio. Estos métodos contendrán la lógica y las validaciones necesarias para el cambio.

Ejemplo de refactorización:

// Eliminar el método estático update

// Añadir métodos de instancia en la clase SampleType
public SampleType changeDescription(SampleTypeDescription newDescription) {
    // Aquí podrías añadir validaciones si fueran necesarias
    // Por ejemplo, que la descripción no sea la misma que la actual.
    if (newDescription == null) {
      throw NullArgumentException.of(Constants.SAMPLE_TYPE, Constants.DESCRIPTION);
    }
    return new SampleType(this.id, newDescription, this.code, this.expirationDate);
}

public SampleType changeCode(SampleTypeCode newCode) {
    if (newCode == null || newCode.getValue() == null) {
      throw NullArgumentException.of(Constants.SAMPLE_TYPE, Constants.CODE);
    }
    // Lógica de negocio: Imagina que el código no puede cambiarse si el tipo de muestra ya ha sido usado.
    // if (this.hasBeenUsed()) {
    //     throw new DomainException("Cannot change the code of a sample type that is already in use.");
    // }
    return new SampleType(this.id, this.description, newCode, this.expirationDate);
}

2. Inconsistencia en la Mutabilidad

  • Problema: El método expire() es inmutable (crea una nueva instancia), pero el método update() es mutable (modifica la instancia existente). Esto es confuso y propenso a errores.
  • Impacto: Un desarrollador que use la clase no sabrá si un método modifica el objeto actual o devuelve uno nuevo, lo que puede llevar a bugs difíciles de rastrear.
  • Solución: Sé consistente. El enfoque inmutable (el de expire()) es generalmente superior. Haz que todos los métodos que cambian el estado devuelvan una nueva instancia del objeto. Mi propuesta en el punto 1 ya sigue este principio.

3. Centralizar las Validaciones en el Constructor

  • Problema: Las validaciones de nulidad están en el factory of, pero no en create ni en el constructor. ¿Qué pasa si description o code son nulos en el método create? El objeto se crearía en un estado inválido.
  • Impacto: La entidad no está protegiendo sus invariantes (reglas que deben ser siempre verdaderas). Una entidad nunca debería existir en un estado inválido.
  • Solución: Mueve las validaciones fundamentales al constructor privado. De esta manera, garantizas que cualquier camino que lleve a la creación de un SampleType pasará por las mismas reglas de validación.

Ejemplo de refactorización:

private SampleType(SampleTypeId id, SampleTypeDescription description, SampleTypeCode code,
                   ExpirationDate expirationDate) {
    // Validaciones de invariantes aquí
    if (code == null || code.getValue() == null) {
        throw NullArgumentException.of(Constants.SAMPLE_TYPE, Constants.CODE);
    }
    // La descripción puede ser opcional, pero si no lo es, también debería validarse aquí.
    if (description == null) {
        throw NullArgumentException.of(Constants.SAMPLE_TYPE, Constants.DESCRIPTION);
    }

    this.id = id;
    this.description = description;
    this.code = code;
    super.initExpiration(expirationDate); // Ver siguiente punto
}

// Los factories ahora solo delegan la creación
public static SampleType of(...) {
    if (id == null || id.getValue() == null) {
        throw NullArgumentException.of(Constants.SAMPLE_TYPE, Constants.ID);
    }
    return new SampleType(id, description, code, expirationDate);
}

public static SampleType create(SampleTypeDescription description, SampleTypeCode code) {
    // El ID y la fecha de expiración son nulos en la creación inicial
    return new SampleType(null, description, code, null);
}

4. Herencia (extends Expirable) vs. Composición

  • Problema: La herencia es una relación muy fuerte ("es un"). A veces, la capacidad de expirar es más una característica o un comportamiento que la identidad fundamental del objeto. La herencia puede ser rígida. ¿Qué pasa si en el futuro necesitas que un SampleType también sea Auditable? No puedes heredar de dos clases.
  • Impacto: Limita la flexibilidad del diseño a largo plazo.
  • Solución (Opcional, pero recomendada): Considera la composición. En lugar de heredar de Expirable, SampleType podría tener un objeto que gestione la expiración. No obstante si Expirable es muy simple (solo un campo y un getter), el enfoque inicial podría ser aceptable. Pero si Expirable tiene más lógica, la composición es más flexible.

Ejemplo de refactorización:

// La clase Expirable podría ser reemplazada por un Value Object o componente
public class Expiration {
    private final ExpirationDate date;
    // ... lógica como isExpired(), etc.
}
public class SampleType { // ya no extiende Expirable
    // ... otros campos
    private final Expiration expiration; // Composición
    // El constructor manejaría esto
    private SampleType(..., ExpirationDate expirationDate) {
        this.expiration = new Expiration(expirationDate);
        // ...
    }
    public boolean isExpired() {
        return this.expiration.isExpired();
    }
    
    // El método expire devolvería una nueva instancia con un nuevo objeto Expiration
    public SampleType expire(ExpirationDate expirationDate) {
        return new SampleType(this.id, this.description, this.code, expirationDate);
    }
}

El informe que he expuesto no lo he elaborado yo, sólo lo transcribo aquí. Yo hice una pequeña revisión previa que quería contrastar, pero que fue bastante coincidente. En mi caso como tenía más contexto quizá vi alguna cosilla extra, que no viene al caso.

Por último dejo una propuesta de refactorización, aplicando las sugerencias principales (inmutabilidad, métodos de comportamiento y validaciones centralizadas), auque sigo dejando la extensión de Expirable que sería la responsable de exponer un método isExpired(). La clase podría quedar así:

/**
 * Representa un Tipo de Muestra en el dominio.
 * Esta entidad es inmutable. Cualquier operación que modifique su estado
 * devolverá una nueva instancia con el estado actualizado.
 */
public final class SampleType extends Expirable { // 'final' para reforzar la inmutabilidad

  private final SampleTypeId id;
  private final SampleTypeDescription description;
  private final SampleTypeCode code;

  private SampleType(SampleTypeId id, SampleTypeDescription description, SampleTypeCode code, ExpirationDate expirationDate) {
    super(expirationDate);
    
    // Centralizar la validación de invariantes aquí
    if (code == null) throw NullArgumentException.of(Constants.SAMPLE_TYPE, Constants.CODE);
    // Asumiendo que la descripción es obligatoria. Si no, se puede omitir esta línea.
    if (description == null) throw NullArgumentException.of(Constants.SAMPLE_TYPE, Constants.DESCRIPTION);

    this.id = id;
    this.description = description;
    this.code = code;
  }

  /**
   * Factory method para reconstituir una entidad desde persistencia.
   */
  public static SampleType load(SampleTypeId id, SampleTypeDescription description, SampleTypeCode code, ExpirationDate expirationDate) {
    if (id == null) throw NullArgumentException.of(Constants.SAMPLE_TYPE, Constants.ID);
    return new SampleType(id, description, code, expirationDate);
  }

  /**
   * Factory method para crear una nueva entidad.
   */
  public static SampleType create(SampleTypeDescription description, SampleTypeCode code) {
    return new SampleType(null, description, code, null);
  }

  /**
   * Cambia la descripción del tipo de muestra, devolviendo una nueva instancia.
   * @param newDescription La nueva descripción.
   * @return Una nueva instancia de SampleType con la descripción actualizada.
   */
  public SampleType changeDescription(SampleTypeDescription newDescription) {
    return new SampleType(this.id, newDescription, this.code, getExpirationDate());
  }

  /**
   * Cambia el código del tipo de muestra, devolviendo una nueva instancia.
   * @param newCode El nuevo código.
   * @return Una nueva instancia de SampleType con el código actualizado.
   */
  public SampleType changeCode(SampleTypeCode newCode) {
    // Aquí se podrían añadir reglas de negocio más complejas antes de crear la nueva instancia.
    return new SampleType(this.id, this.description, newCode, getExpirationDate());
  }

  /**
   * Marca el tipo de muestra como expirado, devolviendo una nueva instancia.
   * @param expirationDate La fecha en la que expira.
   * @return Una nueva instancia de SampleType con la fecha de expiración establecida.
   */
  public SampleType expire(ExpirationDate expirationDate) {
    if (expirationDate == null) {
      throw NullArgumentException.of(Constants.SAMPLE_TYPE, "expirationDate");
    }
    return new SampleType(this.id, this.description, this.code, expirationDate);
  }
  
  // Getters
  public SampleTypeId getId() {
    return id;
  }

  public SampleTypeDescription getDescription() {
    return description;
  }

  public SampleTypeCode getCode() {
    return code;
  }
}

¿Qué os parece? ¿alguna aportación?

Dejo los comentarios para ver si alguien se anima y adivinar quién elaboró el informe.

Comentarios

Publicar un comentario