Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

This question already has an answer here:

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.

share|improve this question

marked as duplicate by gnat, MichaelT, GlenH7, Kilian Foth, Yusubov Aug 30 '13 at 12:21

This question has been asked before and already has an answer. If those answers do not fully address your question, please ask a new question.

4  
There are other things in this class that would bother me, like ... else if (ContactAddress.GetBrokenRules().Count > 0) AddToBrokenRulesList(brokenRules, ContactAddress.GetBrokenRules()); instead of simply ... else brokenRules.AddRange(ContactAddress.GetBrokenRules()) –  sloth Aug 29 '13 at 13:15
1  
Moreover, the repetition of brokenRules.Add(new BrokenBusinessRule(...) should IMHO be refactored to one method. –  Doc Brown Aug 29 '13 at 14:10
3  
Is that code a starting point that he improves over the course of the book? Maybe it is bad on purpose. –  stonemetal Aug 29 '13 at 14:30
7  
Whoever introduced the term "validation" here has muddied the pool. The linked code snippet shows that these checks do not prevent the construction of the object; they're used somewhere else to decide whether or not the borrower gets a loan. A borrower object which fails these checks is not an invalid object, just a poor credit risk. All that anaemic model waffle is completely irrelevant. –  itsbruce Aug 29 '13 at 15:20
1  
One not to buy, then. Saved us all some money. –  itsbruce Aug 29 '13 at 21:37
show 7 more comments

9 Answers

up vote 12 down vote accepted

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
}
share|improve this answer
2  
You see now that is so much cleaner than the example in the book which is supposed to TEACH good design... –  iAteABug_And_iLiked_it Aug 29 '13 at 17:31
1  
I like this solution best so far. ( maybe because I have ben doing this myself so far) Although the specification pattern , as it has been suggested, works too , but this is simple, easy to read.And you can unit test each method separately –  iAteABug_And_iLiked_it Aug 29 '13 at 18:09
1  
There are some really insightful answers , yours has the code and it looks very clean to me. Accepted. +1 –  iAteABug_And_iLiked_it Aug 30 '13 at 13:05
add comment

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.

share|improve this answer
1  
"imagine that you work for a bank... policies should be loaded in from a database" -- this might be a good approach indeed. I once dealt with a legacy bank application designed like that; it has run just fine for something like 5 years without code changes (even source code was lost). The only reason it had to be dropped was that over time, it remained the only client of respective legacy database, which had pretty high maintenance cost; functionality wise, I never heard any complaints about it (have to admit, I have deepest respect to the unknown guy who designed it that way) –  gnat Aug 29 '13 at 16:53
3  
@iAteABug_And_iLiked_it: And Neil Gaiman agrees with me, so by transitivity, Neil Gaiman agrees with you as well. ericlippert.com/2013/07/03/… –  Eric Lippert Aug 29 '13 at 17:39
3  
@iAteABug_And_iLiked_it: a separate problem is of course that some of those business rules are not very good. The idea that everyone has to have a first and last name is nonsense; there are countries where it is very common to give people only one name, and even in countries where two-part names are common, you still want to be able to sell Teller a mortgage. This is interesting reading: kalzumeus.com/2010/06/17/… –  Eric Lippert Aug 29 '13 at 17:47
5  
"Rules change all the time in banks and rules have complex relationships with each other. [...] policies should be loaded in from a database" If the rules are changing in complex ways isn't that reason enough not to keep them in a database? Otherwise you end up with a meta language in your database whose evaluation must be updated all the time to make sure that the language can describe the complex rules. In the most extreme case you might actually need a Turing complete language in the database to describe your sufficiently complex rules. –  kahoon Aug 29 '13 at 19:11
1  
@kahoon: You certainly do end up with a metalanguage. There is practically an entire industry now just for building software that describes flowcharts for medical billing protocols. Those flowcharts change constantly. –  Eric Lippert Aug 29 '13 at 19:36
show 3 more comments

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.

share|improve this answer
1  
Can't argue with 22.8K rep... My issue was that first we are told to decouple, decouple, decouple, and separate validation logic from data model and then you see this kind of code from people with years of experience doing the opposite and you just get so confused –  iAteABug_And_iLiked_it Aug 29 '13 at 13:22
14  
@iAteABug_And_iLiked_it - No, by all means you should argue with me if you think I'm wrong. And in general, you should go nuts with decoupling - but there's always some line where you need to stop or else you end up with classes that have only one variable making your code completely unusable. That line is a trade off, and one you'll be able to guess more accurately with experience. If you're light on experience, err towards more decoupling. –  Telastyn Aug 29 '13 at 13:30
2  
While I agree that this is pretty standard and possibly good enough (in the "we've got bigger problems elsewhere" kind of way), it's still not particularly pretty. It's not DRY (e.g. one or two classes down the line I'd get irritated that child object's validations have to be manually merged in, with the same code each time), encourages monolithic methods (with high cyclomatic complexity), has a low signal to noise ratio, etc. To be fair, the book might later on refactor some of it into a more generic form. –  Daniel B Aug 29 '13 at 13:52
2  
@iAteABug_And_iLiked_it you can always argue with rep. Simply accepting an argument because of rep is the "Argument from authority" logical fallacy (en.wikipedia.org/wiki/Argument_from_authority). Aristotle had (and still has) huge rep but that doesn't mean that vultures are impregnated by the wind. Not saying Aristotle was a rep-whore, of course ;) –  itsbruce Aug 29 '13 at 14:34
1  
I'm not that bright; I missed that this was a model with validation, I read the code snippet as an operational method that validated it's arguments. +1 this is the right approach. –  Jimmy Hoffa Aug 29 '13 at 14:40
show 7 more comments

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.

share|improve this answer
    
The logic should be "The borrower must satisfy these rules to get a loan" and should also cover "what we say to a borrower who doesn't satisfy the rules" etc. That doesn't mean all the validation has to be in the object. Are you confusing the (OO-context) validation of a created object with the (Business context) validation of a set of business rules? It does feel to me as if you are conflating the life-cycle of a software object with the business process that it is modelling. These are checks to see if a borrower will get a loan, not to determine whether the object is created. –  itsbruce Aug 29 '13 at 15:07
    
@DocBrown , I understand where you are coming from. If it is a service, e.g., one is bound to have some validation. But this particular class is neither just a service, nor is it just a plain entity ( like a simple person object e.g. which only stores name, age,dob etc), instead it is trying to do a bit of both, and that with loads of if else statements –  iAteABug_And_iLiked_it Aug 29 '13 at 17:54
2  
@iAteABug_And_iLiked_it: validation rules are business logic, and when creating a business model it mostly ok design to place object-related code expressing business logic in the object's class. However, I am with Eric Lippert here, see my edit. –  Doc Brown Aug 29 '13 at 18:40
add comment

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.

share|improve this answer
4  
BrokenBusinessRule in the original posting is not different from your BusinessRule class in your answer (though I think the name BusinessRule makes more sense). What bothers me more is your suggestion to create a subclass for each rule - for such simple rules, I have the strong feeling that this will overcomplicate a simple situation, and increases the "noise/signal" ratio, which does not increase maintainability. –  Doc Brown Aug 29 '13 at 14:29
    
@DocBrown An anonymous inner class to do the actual checking would be one way avoid subclassing (as would doing this in a language with proper support for closures, such as Scala or groovy). The book example has some rules dumping whole sets of extra broken rules into the main set, with no proper way after the event to identify those as a distinct group. How is polluting the data like that decreasing the signal to noise ratio? The example as given is completely impractical as anything other than a (bad) demo. –  itsbruce Aug 29 '13 at 15:30
    
yes, I suppose this is another way to do the same validation, much cleaner than the original. +1 –  iAteABug_And_iLiked_it Aug 30 '13 at 4:48
add comment

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

share|improve this answer
    
Thanks but My brain melted when I saw the wikipedia article about Specification Pattern. I'll probably be able to apply it blindfolded after using it half a dozen times or so but right now it just looks overengineered –  iAteABug_And_iLiked_it Aug 30 '13 at 4:49
add comment

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.

share|improve this answer
1  
But on the + side, the if is simple and straight forward. It is encapsulated in the business entity - easy to find and maintain. I'd probably create a set of typed validation rules and use some sort of strategy pattern. Annotations are often times to unflexible. –  Falcon Aug 29 '13 at 12:57
    
thats what I thought. Yet I got called a witch for posting it on stackoverflow...Annotations are better.Plus I just think its plain wrong to put validation in your entity class. Two separate classes! –  iAteABug_And_iLiked_it Aug 29 '13 at 13:01
1  
@David DV It's not too difficult to do both. Base class which validates attribute based properties via virtual method and subclasses validate more complex stuff by overriding that method. –  Mika Kolari Aug 29 '13 at 13:27
1  
Well no it's not difficult to do both, it's just not very clear when debugging when your validation rules are in annotations AND in code –  David DV Aug 29 '13 at 13:51
1  
IMHO this answer shows up how easy it is to get on the road to over-engineering. Though using attribut-based validation may be a good idea if there is not just one class of this kind with 7 rules, but >20 classes with >100 rules in total. –  Doc Brown Aug 29 '13 at 14:08
show 2 more comments

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.

share|improve this answer
add comment

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)

share|improve this answer
1  
how does this answer the question asked? –  gnat Aug 29 '13 at 16:21
add comment

Not the answer you're looking for? Browse other questions tagged or ask your own question.