Style for control flow with validation checks

Question

I find myself writing a lot of code like this:

int myFunction(Person* person) {
  int personIsValid = !(person==NULL);
  if (personIsValid) {
     // do some stuff; might be lengthy
     int myresult = whatever;
     return myResult;
  }
  else {
    return -1;
  }
}

It can get pretty messy, especially if multiple checks are involved. In such cases, I've experimented with alternate styles, such as this one:

int netWorth(Person* person) {
  if (Person==NULL) {
    return -1;
  }
  if (!(person->isAlive))  {
    return -1;
  }
  int assets = person->assets;
  if (assets==-1)  {
    return -1;
  }
  int liabilities = person->liabilities;
  if (liabilities==-1) {
    return -1;
  }
  return assets - liabilities;
}

I'm interested in comments about the stylistic choices here. [Don't worry too much about the details of individual statements; it's the overall control flow that interests me.]

Answer 1

For this kind issues Martin Fowler proposed Specification pattern:

_{...design pattern, whereby business rules can be recombined by chaining the business rules together using boolean logic.
 
A specification pattern outlines a business rule that is combinable with other business rules. In this pattern, a unit of business logic inherits its functionality from the abstract aggregate Composite Specification class. The Composite Specification class has one function called IsSatisfiedBy that returns a boolean value. After instantiation, the specification is "chained" with other specifications, making new specifications easily maintainable, yet highly customizable business logic. Furthermore upon instantiation the business logic may, through method invocation or inversion of control, have its state altered in order to become a delegate of other classes such as a persistence repository...}

Above sounds a bit high-brow (at least to me), but when I tried it in my code it went quite smoothly and turned out easy to implement and read.

The way I see it, main idea is to "extract" code that does the checks into dedicated method(s) / objects.

With your netWorth example, this could look about as follows:

int netWorth(Person* person) {
  if (isSatisfiedBySpec(person)) {
    return person->assets - person->liabilities;
  }
  log("person doesn't satisfy spec");
  return -1;
}

#define BOOLEAN int // assuming C here
BOOLEAN isSatisfiedBySpec(Person* person) {
  return Person != NULL
      && person->isAlive
      && person->assets != -1
      && person->liabilities != -1;
}

Your case appears rather simple so that all the checks look OK to fit in a plain list within a single method. I often have to split to more methods to make it read better.

I also typically group / extract "spec" related methods in a dedicated object, although your case looks OK without that.

  // ...
  Specification s, *spec = initialize(s, person);
  if (spec->isSatisfied()) {
    return person->assets - person->liabilities;
  }
  log("person doesn't satisfy spec");
  return -1;
  // ...

---

This question at Stack Overflow recommends a few links in addition to one mentioned above: Specification Pattern Example. In particular, answers suggest Dimecasts 'Learning the Specification pattern' for a walkthrough of an example and mention "Specifications" paper authored by Eric Evans and Martin Fowler.

Answer 2

I find it easier to move validation to its own function, it helps keep the intent of other functions cleaner, so your example would be like this.

int netWorth(Person* person) { 
    if(validPerson(person)) {
        int assets = person->assets;
        int liabilities = person->liabilities;
        return assets - liabilities;
    }
    else {
        return -1;
    }
}

bool validPerson(Person* person) { 
    if(person!=NULL && person->isAlive
      && person->assets !=-1 && person->liabilities != -1)
        return true;
    else
        return false;
}

Answer 3

One thing that I've seen work particularly well is introducing a validation layer into your code. First you have a method that does all the messy validation and returns errors (like -1 in your examples above) when something goes wrong. When the validation is done, the function calls another function to do the actual work. Now this function doesn't need to do all those validation steps because they should already be done. That is to say, the work function assumes that the input is valid. How should you deal with assumptions? You assert them in the code.

I think this makes the code very easy to read. The validation method contains all the messy code to deal with errors on the user side. The work method cleanly documents it's assumptions with asserts and then doesn't have to work with potentially invalid data.

Consider this refactoring of your example:

int myFunction(Person* person) {
  int personIsValid = !(person==NULL);
  if (personIsValid) {
     return myFunctionWork(person)
  }
  else {
    return -1;
  }
}

int myFunction(Person *person) {
  assert( person != NULL);  
  // Do work and return
}

Answer 4

The second style is called early return. You handle all the special cases and error conditions first and get them out of the way before handling the main logic.

There is no general consensus about which is best. I personally prefer early return because it makes the main logic appear clearer when not nested inside the special-case checks.

The counterargument is that multiple returns make the code flow harder to follow and that the distinction between "normal" and "special" cases are artificial. The single exit philosophy states a function should only have one return. (But since you also have two returns in the first example, it wont make the single-exit people happy either!)

It is true that deeply nested returns are easy to miss when reading the code. It is also problematic in case of resource cleanup - single exit makes it easier to ensure everything is cleaned up correctly in all cases.

Therefore I take the pragmatic approach to use early return for special cases, but clearly separate the early return section from the main logic. The early return section should not perform any complex computations and should not allocate any resources needing cleanup and should not have any side effects.