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.

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.]

share|improve this question
2  
Allow me to point out that you have a fairly serious specification error in your example. If, for example, assets == 42 and liabilities == 43, you will declare the person nonexistent. –  John R. Strohm May 22 '12 at 23:14

3 Answers 3

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 withing 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.

share|improve this answer

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;
}
share|improve this answer

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
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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