¿Son malas las declaraciones if-else para validación? [duplicar]

Soy un desarrollador de .NET, así que escribí esto en el bloc de notas utilizando la sintaxis de C # (que podría ser idéntica a JAVA).

public class BorrowerValidator
{    
    private readonly Borrower _borrower;    

    public BorrowerValidator(Borrower borrower)
    {
        _borrower = borrower;
        Validate();
    }

    private readonly IList<ValidationResult> _validationResults;

    public IList<ValidationResult> Results
    {
        get
        {
            return _validationResults;
        }
    }

    private void Validate()
    {
        ValidateAge();
        ValidateFirstName();
        // Carry on
    }

    private void ValidateAge()
    {
        if (_borrower.Age < 18)
            _validationResults.Add(new ValidationResult("Age", "Borrower must be over 18 years of age");
    }

    private void ValidateFirstname()
    {
        if (String.IsNUllOrEmpty(_borrower.Firstname))
            _validationResults.Add(new ValidationResult("Firstname", "A borrower must have a first name");
    }   
}

Usage:

var borrower = new Borrower
{
    Age = 17,
    Firstname = String.Empty        
};

var validationResults = new BorrowerValidator(borrower).Results;

No tiene sentido pasar el objeto borrower a ValidateAge y ValidateFirstName ; en lugar de eso, simplemente almacénelo como un campo.

Para ir más lejos, puedes extraer una interfaz: IBorrowerValidator . Supongamos que tiene diferentes formas de validar a sus prestatarios en función de sus circunstancias financieras. Por ejemplo, si alguien está en quiebra, puede tener:

public class ExBankruptBorrowerValidator : IBorrowerValidator
{
   // Much stricter validation criteria
}

  

No soy un experto pero no me siento cómodo con

     

1- Demasiadas declaraciones if else.

     

2- La clase sirve como una entidad, Y tiene validación. ¿No es un diseño maloliente?

Tu intuición es acertada aquí. Este es un diseño débil.

El código pretende ser un validador de reglas de negocios . Ahora imagine que trabaja para un banco y se cambia la política de tener que ser 18, a tener 21, pero si hay un cofirmante aprobado, entonces se permite tener un joven de 18 años, a menos que el cofirmante viva en Maryland, en cuyo caso ...

Ves a dónde voy aquí. Las reglas cambian todo el tiempo en los bancos y las reglas tienen relaciones complejas entre sí. Por lo tanto, este código es un mal diseño porque requiere que un programador reescriba el código cada vez que cambie una política. La intención del código dado es producir una razón por la cual se ha violado una política, y eso es genial, pero en ese caso las políticas deben ser objetos , no if declaraciones , y las políticas deben estar cargadas desde una base de datos , no codificadas en la validación para la construcción de un objeto.

Mej. Normalmente, ese tipo de cosas deberían considerarse un olor de código, pero en este caso en particular creo que está bien. Sí, podrías buscar hacer anotaciones, pero son un dolor y medio de depurar. Sí, podría buscar mover la validación a una clase diferente, pero es poco probable que este tipo de validación varíe o se reutilice para otros tipos.

Esta implementación es efectiva, fácil de probar y fácil de mantener. Para muchas aplicaciones, este diseño logrará el equilibrio correcto de las compensaciones de diseño. Para otros, más desacoplamiento puede valer la ingeniería adicional.

Una cosa más con respecto al punto 2 de su pregunta: eliminar la lógica de validación de esa clase (o cualquier otra lógica que no sirva para el propósito de la clase como una entidad) elimina la lógica de negocios por completo. Esto te deja con un modelo de dominio anémico , que en su mayoría se considera un antipatrón.

EDITAR: Por supuesto, estoy totalmente de acuerdo con Eric Lippert en que la lógica de negocios específica que es probable que se cambie a menudo no debe incluirse en el objeto. Eso todavía no significa que sea un buen diseño para eliminar cualquier lógica empresarial de los objetos de negocios, pero este código de validación en particular es un buen candidato para ser extraído a un lugar donde se puede cambiar desde el exterior.

Sí, esto es un mal olor, pero la solución es fácil. Tienes una lista de BrokenRules, pero todo lo que veo en BrokenBusinessRule es, por lo que puedo ver, un nombre y una cadena explicativa. Tienes esto al revés. Aquí hay una forma de hacerlo (tenga en cuenta: esto no está relacionado con mi diseño y no implicaba mucho pensamiento de diseño; estoy seguro de que se podría hacer algo mejor con más reflexión):

1. Cree una clase BusinessRule y una clase RuleCheckResult.
2. Asígnele una propiedad de nombre, un método "checkBorrower" que devuelva una clase RuleCheckResult.
3. Haga lo que sea necesario para permitir que BusinessRule vea todas las propiedades verificables de Prestatario (por ejemplo, conviértalo en una clase interna).
4. Cree subclases para cada tipo específico de regla (o, mejor, use clases internas anónimas o una fábrica de reglas o un lenguaje sano que admita cierres y funciones de orden superior).
5. Rellene una colección con todas las reglas que realmente quiere usar

Luego, todas esas declaraciones if-then-else pueden ser reemplazadas por un simple bucle similar a este:

for (BusinessRule rule: businessRules) {
  RuleCheckResult result = rule.check(borrower);
  if (!result.passed) {
    for (RuleFailureReason reason: result.getReasons()) {
      BrokenRules.add(rule.getName(), reason.getExplanation())
    }
}

O podrías hacerlo más simple y solo tener esto

for (BusinessRule rule: businessRules) {
  RuleCheckResult result = rule.check(borrower);
  if (!result.passed) {
      BrokenRules.add(rule.getName(), result)
    }
}

Debería ser bastante obvio cómo se pueden componer RuleCheckResult y RuleFailureReason.

Ahora puede tener reglas complejas con múltiples criterios o reglas simples con ninguna. Pero no estás tratando de representar a todos esos en un largo conjunto de ifs y if-elses y no tienes que volver a escribir el objeto Borrower cada vez que creas una nueva regla o elimines una antigua.

Creo que en este tipo de escenarios de reglas, un buen patrón para aplicar es The Specification Pattern . No solo crea una forma más elegante de encapsular una regla de negocios individual, sino que también lo hace de manera que la regla individual se pueda combinar con otras reglas para crear reglas más complejas. Todo sin (IMHO) que requiere una infraestructura sobre-diseñada. La página de Wikipedia también proporciona una implementación de C # bastante impresionante que puedes usar de inmediato (aunque con cualquier cosa recomiendo entenderla y no solo pegarla a ciegas).

Para usar un ejemplo modificado de la página de Wikipedia:

ValidAgeSpecification IsOverEighteen = new ValidAgeSpecification();
HasBankAccountSpecification HasBankAccount = new HasBankAccountSpecification();
HasValidCreditScoreSpecification HasValidCreditScore = new HasValidCreditScoreSpecification();

// example of specification pattern logic chaining
ISpecification IsValidBorrower = IsValidAge.And(HasBankAccount).And(HasValidCreditScore);

BorrowerCollection = Service.GetBorrowers();

foreach (Borrower borrower in BorrowerCollection ) {
    if (!IsValidBorrower.IsSatisfiedBy(borrower))  {
        validationMessages.Add("This Borrower is not Valid!");
    }
}

Puede decir "¡Este ejemplo en particular no enumera las reglas individuales que se rompen!". El punto aquí es demostrar cómo se pueden componer las reglas. Puede mover las 4 reglas (incluida la compuesta) a una colección de ISpecification e iterar sobre ellas como lo demostró itsbruce.

Más adelante, cuando los usuarios superiores quieran crear un nuevo programa de préstamos que solo esté disponible para los adolescentes que tienen un cosignatario, crear una regla como esa es fácil y más declarativo. ¡Además, puedes reutilizar tu regla de edad existente!

ISpecification ValidUnderageBorrower = IsOverEighteen.Not().And(HasCosigner);

Eso no es reutilizable en absoluto. Yo usaría Data Annotations o un marco de validación basado en un atributo similar. Ver enlace por ejemplo.

Editar: mi propio ejemplo

using System;
using System.Collections.Generic;
using System.ComponentModel.DataAnnotations;
using System.Linq;

namespace ValidationTest
{
    public interface ICategorized
    {
        [Required]
        string Category { get; set; }
    }
    public abstract class Page
    {
        [Required]
        [MaxLength(50)]
        public string Slug { get; set; }
        [Required]
        public string Title { get; set; }
        public string Text { get; set; }

        public virtual ICollection<ValidationResult> Validate()
        {
            var results = new List<ValidationResult>();
            ValidatePropertiesWithAttributes(results);
            ValidatePropertiesFromInterfaces(results);
            return results;
        }
        protected void ValidatePropertiesWithAttributes(ICollection<ValidationResult> results)
        {
            Validator.TryValidateObject(this, new ValidationContext(this,null,null), results);
        }
        protected void ValidatePropertiesFromInterfaces(ICollection<ValidationResult> results)
        {
            foreach(var iType in this.GetType().GetInterfaces())
            {
                foreach(var property in iType.GetProperties())
                {
                    foreach (var attr in property.GetCustomAttributes(false).OfType<ValidationAttribute>())
                    {
                        var value = property.GetValue(this);
                        var context = new ValidationContext(this, null, null) { MemberName = property.Name };
                        var result = attr.GetValidationResult(value, context);
                        if(result != null)
                        {
                            results.Add(result);
                        }
                    }
                }
            }
        }
    }
    public class Blog : Page
    {

    }
    public class Post : Page, ICategorized
    {
        [Required]
        public Blog Blog { get; set; }
        public string Category { get; set; }
        public override ICollection<ValidationResult> Validate()
        {
            var results = base.Validate();
            // do some custom validation
            return results;
        }
    }
}

Es fácil de modificar para incluir / excluir propiedades ValidatePropertiesWithAttributes(string[] include, string[] exclude) o agregar un proveedor de reglas para obtener reglas de db Validate(IRuleProvider provider) etc.

La pregunta clave aquí es " es la forma más legible, clara y coherente de implementar la lógica de negocios ".

En el caso, creo que la respuesta es sí.

Todos los intentos de dividir el código solo enmascaran la intención sin agregar nada que no sea un número reducido de "if" s

Esta es una lista ordenada de pruebas, y una secuencia de afirmaciones "if" parece ser la mejor implementación. Cualquier intento de hacer algo inteligente será más difícil de leer y, lo que es peor, es posible que no pueda implementar una nueva regla en una implementación más "más bonita" y abstracta.

Las reglas de negocios pueden ser muy complicadas y, a menudo, no hay una forma "limpia" de implementarlas.

En la clase principal del programa pasaría la entidad Borrower a la clase BrokenBusinessRule y haría allí la lógica de negocios para simplificar un poco más la prueba y la legibilidad (Nota: si este fuera un programa más grande y complicado, la importancia de desacoplamiento sería mucho mayor)