Are too many if-else statements for validation bad?

Question

From the book Professional Enterprise .Net, which has 5 star rating on Amazon that I am doubting after having a read through. Here is a Borrower class (In C# but it's pretty basic; anyone can understand it) that is part of a Mortgage application the authors built:

    public List<BrokenBusinessRule> GetBrokenRules()
    {
        List<BrokenBusinessRule> brokenRules = new List<BrokenBusinessRule>();

        if (Age < 18)
            brokenRules.Add(new BrokenBusinessRule("Age", "A borrower must be over 18 years of age"));

        if (String.IsNullOrEmpty(FirstName))
            brokenRules.Add(new BrokenBusinessRule("FirstName", "A borrower must have a first name"));

        if (String.IsNullOrEmpty(LastName))
            brokenRules.Add(new BrokenBusinessRule("LastName", "A borrower must have a last name"));

        if (CreditScore == null)
            brokenRules.Add(new BrokenBusinessRule("CreditScore", "A borrower must have a credit score"));
        else if (CreditScore.GetBrokenRules().Count > 0)
        {
            AddToBrokenRulesList(brokenRules, CreditScore.GetBrokenRules());
        }

        if (BankAccount == null)
            brokenRules.Add(new BrokenBusinessRule("BankAccount", "A borrower must have a bank account defined"));
        else if (BankAccount.GetBrokenRules().Count > 0)
        {
            AddToBrokenRulesList(brokenRules, BankAccount.GetBrokenRules());
        }
        // ... more rules here ...
        return brokenRules;
    }

Full code listing on snipt.org .

What is confusing me is that the book is supposed to be about professional enterprise design. Maybe I am a bit biased because the author confesses in chapter 1 that he didn't genuinely know what decoupling was, or what SOLID meant until the 8th year of his programming career (and I think he wrote the book in year 8.1)

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? (You might need to view the full class to get some context)

Maybe I am wrong, but I don't want to pick up bad practices from a book which is supposed to be teaching an enterprise design. The book is full of similar code snippets and it is really bugging me now. If it is bad design, how could you avoid using too many if else statements.

I obviously do not expect you to rewrite the class, just provide a general idea of what could be done.

Answer 1

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?

Your intuition is spot on here. This is a weak design.

The code is intended to be a business rule validator. Now imagine that you work for a bank and the policy is changed from having to be 18, to having to be 21, but if there is an approved co-signer then having an 18 year old is allowed, unless the co-signer lives in Maryland, in which case...

You see where I'm going here. Rules change all the time in banks and rules have complex relationships with each other. Therefore this code is a bad design because it requires a programmer to rewrite the code every time a policy changes. The intention of the given code is to produce a reason why a policy has been violated, and that's great, but in that case policies should be objects, not if statements, and policies should be loaded in from a database, not hard-coded into the validation for construction of an object.

Answer 2

I'm a .NET developer, so I wrote this in notepad using C# syntax (which might be identical to 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;

There is no point in passing borrower object to ValidateAge and ValidateFirstName, instead, just store it as a field.

To take things further, you can extract an interface: IBorrowerValidator. Say you then have different ways to validate your borrowers depending on their financial circumstances. For example, if somebody is ex-bankrupt, you may have:

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

Answer 3

Enh. Normally that sort of thing should be considered a code smell, but in this particular instance I think it's fine. Yes, you could look to do annotations but they're a pain and a half to debug. Yes, you could look to move the validation to a different class, but this sort of validation is unlikely to vary or be reused for other types.

This implementation is effective, easy to test, and easy to maintain. For many applications, this design will strike the right balance of design trade-offs. For others, more decoupling may be worth the extra engineering.

Answer 4

One more thing concerning point 2 of your question: removing the validation logic from that class (or any other logic which does not serve the purpose of the class beeing an entity) removes the business logic completely. This leaves you with an anemic domain model, which is mostly considered to be an anti-pattern.

EDIT: Of course, I agree fully to Eric Lippert that specific business logic which is likely to be changed often should not be hardcoded into the object. That still does not mean that it is a good design to remove any business logic from business objects, but this particular validiation code is a good candidate for beeing extracted to a place where it can be changed from outside.

Answer 5

Yes, this is a bad smell, but the solution is easy. You have a BrokenRules list, but all a BrokenBusinessRule is, as far as I can see, is a name and an explanatory string. You have this upside down. Here's one way to do it (please note: this is off the top of my head and didn't involve much design thought - I'm sure something better could be done with more thought):

1. Create a BusinessRule class and a RuleCheckResult class.
2. Give it a name property, a "checkBorrower" method that returns a RuleCheckResult class.
3. Do whatever is necessary to allow BusinessRule to see all the checkable properties of Borrower (make it an inner class, for example).
4. Create subclasses for each specific type of rule (or, better, use anonymous inner classes or a rule factory or a sane language that supports closures and higher order functions).
5. Populate a collection with all the rules you actually want to use

Then all of those if-then-else statements can be replaced by a simple loop something like this:

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

Or you could make it simpler and just have this

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

It should be pretty obvious how RuleCheckResult and RuleFailureReason might be composed.

Now you can have complex rules with multiple criteria or simple rules with none. But you're not trying to represent all of those in one long set of ifs and if-elses and you don't have to rewrite the Borrower object each time you come up with a new rule or drop an old one.

Answer 6

I think that in these types of rules scenarios, a good pattern to apply is The Specification Pattern. It not only creates a more elegant way to encapsulate an individual business rule, it also does it in a way so that the individual rule can be combined with other rules to build more complex rules. All without (IMHO) requiring an over-engineered infrastructure. The Wikipedia page also provides a pretty awesome C# implementation that you can use right out of the box (although with anything I recommend understanding it and not just blindly pasting it).

To use a modified example from the Wikipedia page:

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!");
    }
}

You may say "This particular example doesn't list the individual rules being broken!". The point here is to demonstrate how rules can be composed. You can just as easily move all 4 rules (including the composite one) into a collection of ISpecification and iterate over them like itsbruce has demonstrated.

Later on, when the higher ups want to make a new loan program that's only available to teenagers which have a cosigner, creating a rule like that is easy and more declarative. Plus you can reuse your existing age rule!

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

Answer 7

That's not reusable at all. I would use Data Annotations or similar attribute based validation framework. Check http://odetocode.com/blogs/scott/archive/2011/06/29/manual-validation-with-data-annotations.aspx for example.

Edit: my own example

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

It's easy to modify to include/exclude properties ValidatePropertiesWithAttributes(string[] include, string[] exclude) or add rule provider to get rules from db Validate(IRuleProvider provider) etc.

Answer 8

The key question here is "is this the most readable, clear and consise way to implement the business logic".

In the case i think the answer is yes.

All the attempts to split up the code only mask the intention without adding anything other than a reduced number of "if"s

This is an ordered list of tests, and a sequence of "if" statements would seem to be the best implementation. Any attempt to do something clever will be harder to read and worse you may not be able to implement a new rule in a "prettier" more abstract implementation.

Business rules can be really messy and there is often no "clean" way to implement them.

Answer 9

In the main class of the program I would pass the Borrower entity into the BrokenBusinessRule class and do the business logic there to simplify testing and readability some more (Note: If this were a larger more complicated program the importance of decoupling would be much greater)