Monday, 8 April 2013

Does Defensive Programming Deserve Such a Bad Name?


The other day I went to an hour's talk on erlang, merely as an observer; I know nothing about erlang except that it does sound interesting and that the syntax is... well... unusual. The talk was given to some Java programmers who had recently learnt erlang and was a fair critic about their first erlang project, which they were just completing. The presenter said that these programmers needed to stop thinking like Java programmers and start thinking like erlang programmers1 and in particular to stop programming defensively and let processes fail fast and fix the problem.

Now, apparently, this is good practice in erlang because one of the features of erlang, and please correct me if I'm wrong, is that work is split into supervisors and processes. Supervisors supervise processes, creating them, destroying them and restarting them if required. The idea of failing fast is nothing new and is defined as the technique to use when your code comes across an illegal input. When this happens your code just falls over and aborts the point being that you fix the supplier of that input rather than your code. The sub-text of what the presenter said is that Java and defensive programming is bad and fail-fast if good, which is something that really needs closer investigation.

The first thing to do is to define Defensive Programming and the first definition I came across was in what is now possibly a legendary book: Writing Solid Code by Steve Maguire published by Microsoft Press. I read this book many years ago when I was a C programmer, which was then the defacto language of choice. In the book Steve demonstrates the use of an _Assert macro:

/* Borrowed from Complete Code by Steve Maguire */
#ifdef DEBUG

    void _Assert(char *,unsigned)     /* prototype */
    #define ASSERT(f)        \
         if(f)                          \
             { }                        \
         else
             _Assert(__FILE__,__LINE__)
#else
    #define ASSERT(f)
#endif

// ...and later on..

void _Assert(char *strFile,unsigned uLine) {

    fflush(NULL);
    fprintf(stderr, "\nAssertion failed: %s, line %u\n",strFile,uLine);
    fflush(stderr);
    abort();
} 

/////// ...and then in your code

void my_func(int a).  {

    ASSERT(a != 0);

    // do something...
}

…as his definition of defensive programming. The idea here is that we're defining a C macro that when DEBUG is turned on, my_func(…) will test it's input using the ASSERT(f) and that will call the _Assert(…) function if the condition fails. Hence, when in DEBUG mode, in this sample my_func(int a) has the ability to abort execution if arg a is zero. When DEBUG is switched off, checked aren't carried out, but the code is leaner and quicker; something which was probably more of a consideration back in 1993.

Looking at this definition, several things come to mind. Firstly, this book was published in 1993, so is this still valid? It wouldn't be a good idea to kill Tomcat by using a System.exit(-1) if one of your users typed in the wrong input! Secondly, Java being more recent is also more sophisticated, has exceptions and exception handlers, so instead go aborting the program we'd throw an exception that would, for example, display an error page highlighting bad inputs.

The main point that comes to mind, however, is that this definition of defensive programming sounds a lot like fail-fast to me, in fact it's identical.

This isn't the first time that I've heard programmers complain about defensive programming, so, why has it got such a bad reputation? Why did the elrang talk presenter denigrate it so much? My guess is that there's good use of defensive programming and bad use of defensive programming. Let me explain with some code…

In this scenario I'm writing a Body Mass Index (BMI) calculator for a program that tells users whether or not they're over weight. A BMI value between 18.5 to 25 is apparently okay whilst anything over 25 ranges from overweight to severely obese with lots of life limiting issues. The BMI calculation uses the following simple formula:

BMI = weight (kg) / (height(m)2)

The reason I chosen this formula is that is presents the possibility of a divide by zero error, which the code I write must defend against.

public class BodyMassIndex {

 
/**
   * Calculate the BMI using Weight(kg) / height(m)2
   *
   *
@return Returns the BMI to four significant figures eg nn.nn
   */
 
public Double calculate(Double weight, Double height) {

   
Validate.notNull(weight, "Your weight cannot be null");
    Validate.notNull
(height, "Your height cannot be null");

    Validate.validState
(weight.doubleValue() > 0, "Your weight cannot be zero");
    Validate.validState
(height.doubleValue() > 0, "Your height cannot be zero");

    Double tmp = weight /
(height * height);

    BigDecimal result =
new BigDecimal(tmp);
    MathContext mathContext =
new MathContext(4);
    result = result.round
(mathContext);

   
return result.doubleValue();
 
}
}

The code above uses the idea put forward in Steve's 1993 definition of defensive programming. When the program calls calculate(Double weight,Double height) four validations are carried out, testing the state of each input argument and throwing an appropriate exception on failure. As this is the 21st century I didn't have to define my own validation routines, I simply used those provided by the Apache commons-lang3 library and imported:

import org.apache.commons.lang3.Validate;

…and added:

    <dependency>
        <groupId>org.apache.commons</groupId>
        <artifactId>commons-lang3</artifactId>
        <version>3.1</version>
    </dependency>

…to my pom.xml.

The Apache commons lang library contains the Validate class, which provides some basic validation. If you need more sophisticated validation algorithms take a look a the Apache commons validator library.

Once validated the calculate(…) method calculates the BMI and rounds it to four significant figures (e.g. nn.nn). It then returns the result to the caller. Using Validate allows me to write lots of JUnit tests to ensure that everything goes well in case of trouble and to differentiate between each type of failure:

public class BodyMassIndexTest {

 
private BodyMassIndex instance;

 
@Before
 
public void setUp() throws Exception {
   
instance = new BodyMassIndex();
 
}

 
@Test
 
public void test_valid_inputs() {

   
final Double expectedResult = 26.23;

    Double result = instance.calculate
(85.0, 1.8);
    assertEquals
(expectedResult, result);
 
}

 
@Test(expected = NullPointerException.class)
 
public void test_null_weight_input() {

   
instance.calculate(null, 1.8);
 
}

 
@Test(expected = NullPointerException.class)
 
public void test_null_height_input() {

   
instance.calculate(75.0, null);
 
}

 
@Test(expected = IllegalStateException.class)
 
public void test_zero_height_input() {

   
instance.calculate(75.0, 0.0);
 
}

 
@Test(expected = IllegalStateException.class)
 
public void test_zero_weight_input() {

   
instance.calculate(0.0, 1.8);
 
}
}

One of the "advantages" of the C code is that you can turn the ASSERT(f) of and on using a compiler switch. If you need to do this in Java then take a look at using Java's assert keyword.

The above sample is what I'd hope we'd agree is the well written sample - the good code. So, what's needed now is the badly written sample. The main criticism of defensive programming is that it can hide errors and that's very true, if you write bad code.

public class BodyMassIndex {

 
/**
   * Calculate the BMI using Weight(kg) / height(m)2
   *
   *
@return Returns the BMI to four significant figures eg nn.nn
   */
 
public Double calculate(Double weight, Double height) {

   
Double result = null;

   
if ((weight != null) && (height != null) && (weight > 0.0) && (height > 0.0)) {

     
Double tmp = weight / (height * height);

      BigDecimal bd =
new BigDecimal(tmp);
      MathContext mathContext =
new MathContext(4);
      bd = bd.round
(mathContext);
      result = bd.doubleValue
();
   
}

   
return result;
 
}
}

The code above also checks against both null and zero arguments, but it does so using the following if statement:

    if ((weight != null) && (height != null) && (weight > 0.0) && (height > 0.0)) {

Looking on the bright side, the code won't crash if the inputs are incorrect, but it won't tell the caller what's gone wrong, it'll simply hide the error and return null. Although it hasn't crashed, you have to ask what's the caller going to do will a null return value? It'll either have to ignore the problem or process the error there and using something like this:

  @Test
 
public void test_zero_weight_input_forces_additional_checks() {

   
Double result = instance.calculate(0.0, 1.8);
   
if (result == null) {
     
System.out.println("Incorrect input to BMI calculation");
     
// process the error
   
} else {
     
System.out.println("Your BMI is: " + result.doubleValue());
   
}
  }

If this 'bad' coding technique is used throughout a code base, then there will be a large amount of extra code required to check each return value.

It's a good idea to NEVER return null values from a method. For more information take a look at this set of blogs.

In conclusion, I really think that there's no difference between defensive programming and fail-fast programming as they're really the same thing. Isn't there, as always, just good coding and bad coding? I'll let you decide.


This code sample is available on Github.

1There's always a paradigm shift in thinking when learning a new language. There will be the point where the penny drops and you "get it", whatever it is.


4 comments:

Ricardo said...

I agree with fail-fast, but the validation code for your BMI calculator seems to break the SRP (it validates *and* calculates BMI).

How would you get around this?

Roger Hughes said...

Strictly speaking I guess that you're right, it does break the SRP. If this were a 'proper' web app and I was validating user inputs, then I'd use a 'proper' validation technique. In writing this blog, I was thinking more about detecting programming errors in an API, which means that I'm really using the Validate class rather like Java's assert with the idea being to let it fail and then to fix it.

Frisian said...

Au contraire, I think there is a difference between defensive programming and fail-fast programming:
Fail-fast programming is about letting something wrong fail as soon as possible. Defensive programming on the other hand about not letting something bad happen and telling about the problem in a concise way.
java.util.Iterator is a good example for fail-fast programming, by virtue of throwing an exception as soon as a concurrent modification is detected. Yet, the contract doesn't go further than that, i.e. the exception doesn't tell you the actual cause.
Defensive programming, like shown in your first example, tells the reason of failure as exact as possible. So, it would be perfectly fine to catch an ArithmeticException after the calculation occurred and then tell the caller about why it happened (i.e. height is 0).
Putting validation at the begin of a method is a mere convention to to separate it from the actual business logic.
Especially with transactions fail-fast code looks quite different from defensive code. Validation code can be put everywhere, because the transaction manager does the clean up. Deferred database constraints are a good example for defensive programming as the violation is signalled only after the commit.

EZ said...

@Frisian would you agree that there are good cases for defensive programming and bad ones as illustrated in the article?