Troppe dichiarazioni if-else per la validazione sono troppe? [duplicare]

Sono uno sviluppatore .NET, quindi l'ho scritto nel blocco note usando la sintassi C # (che potrebbe essere identica 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;

Non è necessario passare borrower object a ValidateAge e ValidateFirstName , invece, salvalo come campo.

Per andare oltre, puoi estrarre un'interfaccia: IBorrowerValidator . Supponiamo che tu abbia diversi modi per convalidare i tuoi mutuatari in base alla loro situazione finanziaria. Ad esempio, se qualcuno è in bancarotta, potresti avere:

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

I am no expert but I don't feel comfortable with

1- Too many if else statements.

2- The class both serves as an entity, AND has validation. Isn't that a smelly design?

La tua intuizione è perfetta qui. Questo è un design debole.

Il codice è destinato a essere un validatore di regole aziendali . Ora immagina di lavorare per una banca e la politica è cambiata da avere 18 anni, fino a 21, ma se c'è un co-firmatario approvato, è consentito avere un diciottenne, a meno che il co-firmatario non viva Maryland, nel qual caso ...

Vedi dove vado qui. Le regole cambiano continuamente nelle banche e le regole hanno relazioni complesse tra loro. Pertanto, questo codice è un cattivo progetto perché richiede a un programmatore di riscrivere il codice ogni volta che un criterio cambia. L'intenzione del codice dato è di produrre una ragione per cui una norma è stata violata, ed è grandioso, ma in quel caso le politiche dovrebbero essere oggetti , non se le istruzioni e le politiche devono essere caricate da un database , non codificate nella convalida per la costruzione di un oggetto.

Enh. Normalmente quel genere di cose dovrebbe essere considerato un odore di codice, ma in questo particolare caso penso che vada bene. Sì, si potrebbe cercare di fare annotazioni, ma sono un doloroso e mezzo per il debug. Sì, potresti cercare di spostare la convalida in una classe diversa, ma è improbabile che questo tipo di convalida possa variare o essere riutilizzato per altri tipi.

Questa implementazione è efficace, facile da testare e facile da mantenere. Per molte applicazioni, questo design raggiungerà il giusto equilibrio tra compromessi di design. Per gli altri, un maggiore disaccoppiamento può valere la pena di un'ingegneria aggiuntiva.

Un'altra cosa riguardo al punto 2 della tua domanda: rimuovere la logica di validazione da quella classe (o qualsiasi altra logica che non è funzionale allo scopo della classe di essere un'entità) rimuove completamente la logica di business. Questo ti lascia con un modello di dominio anemico , che è per lo più considerato un anti-pattern.

EDIT: Ovviamente, sono pienamente d'accordo con Eric Lippert sul fatto che la logica aziendale specifica che è probabile che venga cambiata spesso non dovrebbe essere codificata nell'oggetto. Ciò non significa che sia un buon progetto per rimuovere qualsiasi logica di business dagli oggetti business, ma questo particolare codice di validazione è un buon candidato per essere estratto in un posto dove può essere cambiato dall'esterno.

Sì, questo è un cattivo odore, ma la soluzione è facile. Hai una lista BrokenRules, ma tutta una BrokenBusinessRule è, per quanto posso vedere, un nome e una stringa esplicativa. Hai questo sottosopra. Ecco un modo per farlo (si prega di notare: questo è fuori di testa e non ha comportato molto pensiero di design - Sono sicuro che qualcosa di meglio potrebbe essere fatto con più pensiero):

1. Crea una classe BusinessRule e una classe RuleCheckResult.
2. Assegna una proprietà name, un metodo "checkBorrower" che restituisce una classe RuleCheckResult.
3. Fai tutto il necessario per consentire a BusinessRule di visualizzare tutte le proprietà controllabili del Borrower (rendilo, per esempio, una classe interna).
4. Crea sottoclassi per ogni specifico tipo di regola (o, meglio, usa classi interne anonime o una fabbrica di regole o un linguaggio sano che supporti chiusure e funzioni di ordine superiore).
5. Compila una raccolta con tutte le regole che desideri utilizzare

Quindi tutte le istruzioni if-then-else possono essere sostituite da un semplice ciclo simile a questo:

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

O potresti renderlo più semplice e avere questo

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

Dovrebbe essere abbastanza ovvio come si possano comporre RuleCheckResult e RuleFailureReason.

Ora puoi avere regole complesse con più criteri o regole semplici con none. Ma non stai cercando di rappresentare tutti quelli in un lungo set di ifs e if-els e non devi riscrivere l'oggetto Borrower ogni volta che ottieni una nuova regola o ne cancelli una vecchia.

Penso che in questi tipi di scenari di regole, un buon modello da applicare sia Il modello di specifica . Non solo crea un modo più elegante per incapsulare una singola regola aziendale, ma lo fa anche in modo che la singola regola possa essere combinata con altre regole per costruire regole più complesse. Tutti senza (IMHO) che richiedono un'infrastruttura sovraingegnerizzata. La pagina di Wikipedia offre anche un'implementazione C # davvero fantastica che puoi usare subito (anche se con qualsiasi cosa consiglio di comprenderlo e non solo di incollarlo ciecamente).

Per utilizzare un esempio modificato dalla pagina di 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!");
    }
}

Potresti dire "Questo particolare esempio non elenca le singole regole che vengono violate!". Il punto qui è dimostrare come le regole possono essere composte. Puoi facilmente spostare tutte e 4 le regole (inclusa quella composita) in una raccolta di ISpecification e scorrere su di esse come ha dimostrato il suo embrione.

Più tardi, quando i più alti vogliono fare un nuovo programma di prestito che è disponibile solo per gli adolescenti che hanno un cofirmatario, la creazione di una regola del genere è facile e più dichiarativa. Inoltre puoi riutilizzare la tua regola di età esistente!

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

Questo non è assolutamente riutilizzabile. Vorrei utilizzare Data Annotations o un framework di convalida basato su attributi simili. Ad esempio link .

Modifica: il mio esempio

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;
        }
    }
}

È facile modificare le proprietà di inclusione / esclusione ValidatePropertiesWithAttributes(string[] include, string[] exclude) o aggiungere il provider di regole per ottenere regole da db Validate(IRuleProvider provider) ecc.

La domanda chiave qui è " è questo il modo più leggibile, chiaro e consono per implementare la logica di business ".

Nel caso penso che la risposta sia sì.

Tutti i tentativi di scomposizione del codice mascherano l'intenzione senza aggiungere altro se non un numero ridotto di "se" s

Questo è un elenco ordinato di test e una sequenza di istruzioni "if" sembra essere la migliore implementazione. Ogni tentativo di fare qualcosa di intelligente sarà più difficile da leggere e peggio potresti non essere in grado di implementare una nuova regola in un'implementazione più "carina" più astratta.

Le regole aziendali possono essere davvero disordinate e spesso non esiste un modo "pulito" per implementarle.

Nella classe principale del programma vorrei passare l'entità Borrower nella classe BrokenBusinessRule e fare la logica aziendale lì per semplificare il test e la leggibilità ancora di più (Nota: se questo fosse un programma più complesso più grande l'importanza di disaccoppiamento sarebbe molto maggiore)