Why Defensive Programming is Rubbish

There are many ways to classify a programming style. One of the classifications I hear a lot at my current place of employment is Defensive Programming. Before I worked here I had noticed the style but I don’t think I had a word to describe the practice. Furthermore, if I had thought about a word to the describe the style I wouldn’t have called it Defensive. I would have called it Hide the Problem Programming. Before I get into my rant let me start by explaining the Defensive Programming philosophy.

Defensive programming is a form of defensive design intended to ensure the continuing function of a piece of software in spite of unforeseeable usage of said software. The idea can be viewed as reducing or eliminating the prospect of Murphy’s Law having effect. Defensive programming techniques are used especially when a piece of software could be misused mischievously or inadvertently to catastrophic effect.

Courtesy Wikipedia http://en.wikipedia.org/wiki/Defensive_programming

To be fair I don’t technically disagree with this practice, in fact I agree with it. However, defensive programming in practice (at least in my experience) is taken past what is described above to a very sad and ugly place. There are two things in particular that bother me:

  • Unnecessary Code
  • Null Checks

Let me break them down for you you.

Unnecessary Code

Most “Unnecessary Code” situations seem to be old habits that have been carried over from older languages. I would actually understand many of them in a more dynamic language where you have no compile time checks to ensure program validity, but I see them much more often in static languages like Java. This completely baffles me.

Would you ever do the following?

Integer one = 1;
Integer two = one + one;

if (two > one) {
    return "the world is right: 2 > 1 is true";
}

If the answer is yes, please stop reading now because there is no way I could ever convince you of anything. If the answer is no, there is hope for you yet. This example is a bit extreme but it is a good starting point to demonstrate the ridiculousness of the practice. Now let me try a more realistic scenario:

public class Person {
    private String name = null;

    public void setName(String name) {
        this.name = name;
    }

    public String getName() {
        return name;
    }
}

public Person newPerson(String name) {
    Person person = new Person(); 

    if (name != null) {
        person.setName(name);
    }

    return person;
}

public static void main(String[] args) {
    Person daniel = new Person("Daniel Roop");
}

This is a special case of a null check, which I will discuss next, but I see it all the time. Why are you checking for it to be null, if the default value is null? Why add the extra lines of code for no value at all. Which leads me to my number 1 issues with “defensive programming”.

Null checks

I would recommend to never get me started on this topic. I find null checks to be one of the most unnecessary, ridiculous, ugly practices in programming. It is right up there with Util classes. So let’s start with an example:

public void addFriendToList(List<Friend> friends, Friend newFriend) {
    if (friends != null && newFriend != null) {
        friends.add(newFriend);
    }   
}

This one isn’t obviously ridiculous. My problem with this scenario is that the person calling the method is requesting you add a friend to a list, but in the case where it provided incorrect inputs (like null parameters) it just won’t do anything. How is that helpful to anyone? Yes, you will avoid a null pointer if the friends list is not initialized but

  • It should always be initialized, if not the code calling the method is screwed up and appropriate feedback should be provided.
  • You should provide feedback when parameters are incorrect not hide the fact that something is wrong.

Taking this example to the extreme you might see something like this

public List<Friend> findFavoriteFriends(Person person) {
    List<Friend> favoriteFriends = new ArrayList<Friend>();

    if (person != null) {
        List<Friend> friends = person.getFriends();

        if (friends != null) {
            for (Friend friend : friends) {
                if (friend != null) {
                    if (friend.isFavorite()) {
                        favoriteFriends.add(friend);
                    }
                }
            }
        }
    }

    return favoriteFriends;
}

public static void main(String[] args) {
    Person person = PersonFactory.find("Daniel Roop");
    List<Friend> favoriteFriends = findFavoriteFriends(person);

    if (favoriteFriends != null && favoriteFriend.size() > 0) {
        for (Friend friend : favoriteFriends) {
            if (friend != null) {
                System.out.println(friend.getName());
            }
        }
    }
}

Why do I care? Does it hurt anything to have these extra checks? From a logic perspective the answer is no, but I would argue from a readability, correctness, and maintenance standpoint yes very much.

Readability

I believe code should be written in a way to express the intent through the source (not through commenting). Beside my wonderful method names, I don’t think you would have any clue what the method was attempting to do at first glance. Furthermore, even with those amazing method names, I would argue if you read the main method you couldn’t tell me what was going on.

Correctness

More importantly if something where to go wrong, and I actually did pass a null list I wouldn’t know. I would just get an empty list back and assume my person had no favorite friends. This is not the correct response, in fact it is the wrong response because I have many favorite friends (you the reader are of course one of them). There is no indication to me that there is anything wrong. I, the caller, received an empty list which is a perfectly valid response, but it is wrong.

Maintenance

This is really a combination of the two above. Imagine a large application (let’s say a photo sharing application) with this type of code all over the place. Now imagine you are given a defect that User A was not able to share an image with only their favorite friends. When they select only share with favorite friends no one can see the photos, but they can see their friends are flagged as favorite in their friend list. Where would you even start with that? If you hadn’t been so “defensive” you would probably have an exception written to the log because when User A request their friend list they get a null instead, thus causing a null pointer in the findFavoriteFriends method. This would not only give you a starting point for the bug, it would also trigger a notification of the problem before the User A complaint even reached the developers because your log monitor picked up the exception and notified the developers.

On the other side, you now don’t know if those null checks where put in place because they were meaningful, or just fluff. When I say meaningful I mean that a null value actually caries some information (e.g. null argument to a method means the parameter should not be used.) The developer implementing a fix on this method will have to assume that the null check has some meaning and work around those checks instead of improving the business logic

Loose Ends

I must admit the title of this article is a bit of a misnomer. When I began writing the article I agreed with the title, but after reading up on what Defensive Programming really is, I realized. I am a defensive programmer. Just not in the buzz word cop out sense. If you don’t believe me maybe you will believe Ward Cunningham who has a wiki article on Offensive Programming, and points out Defensive Programming is the same as a FailFast approach which is closer to how I would write my code.

Let me be clear, I am not suggesting you should show stack traces to your end user. But the fact of the matter is, something is wrong and often times I would imagine it is better to show nothing, then partial something. A good example of this is the recent twitter outage users were able to update their status, but there was no indication that no one could see the update. This caused a lot of confusion and I heard much of the interwebs (at least the small corner I pay attention to) say that it would much rather a fail whale than a semi-up system. What you should do instead is when there is an error with a request, catch it in your container (if it gets that far) and show a 500 message. Ideally logging the stack trace that will hopefully trigger a monitor to go off, and developers can investigate.

I can imagine this type of programming has it’s place, especially in embedded systems, but on the web they have no place. You have a simple request response, if one request doesn’t work oh well, display an error and ask the user to try again. Most of the time that will be better than sort-of working. And that is not to say there aren’t situations where non-mission critical pieces can be turned off if they don’t work, but the core functionality should not be written that way.

Conclusion

If I have convinced you of nothing else I hope I can get you to take away two things

  1. Think about every null check you implement, and ask yourself is this necessary?
  2. Stop calling what you are doing defensive programming, it is called “if I don’t see any errors there aren’t any errors programming” and it isn’t helpful to anyone.
This entry was posted in development, programming, rant, theory. Bookmark the permalink.

23 Responses to Why Defensive Programming is Rubbish

  1. Brian says:

    It’s ok, Groovy has an operator for that. The ? can be used to chain null checks for more readable code. So you could say:

    if(person?.size() > 0) …

    Now all you have to do is use Groovy where you want to place null checks. Seems like a reasonable goal to me ;)

  2. Michael Sica says:

    I agree. Imagine if you actually tested every path through the code? The danger in your code samples (for a large application) is that understanding the different states your application could end up in is insane. There are much better ways to message a user when something goes wrong than to allow about half your code to work.

  3. Daniel Roop says:

    @Brian
    This does help solve the readability problem, but it still keeps the “hide the problem” aspect of your code. That being said I agree there are times when a null check is appropriate, and I am a fan of what the dynamic languages do to make this easier. Groovy has the ? operator (which is probably the best) and ruby let’s you add the conditional at the end of the line (allowing you to focus on the action not the conditions)

  4. Russell Centanni says:

    I’m a fan of the fail-fast approach. Lately I’ve been writing methods with parameter assertions up front that throw descriptive exceptions when there’s a problem. Spring has some utility methods in org.springframework.util.Assert that are used for this purpose.

    In my opinion, the only appropriate time for null checks is when null means something in the system.. and if that’s the case the check should be done in a method with a meaningful name. Having said that, I feel like groovy’s ? operator makes playing “hide the problem” even easier, resulting in a drop in code quality.

  5. Corey says:

    Well written my friend. I don’t remember if I’ve ever “devensive coded” before, but I’ll be on the lookout for nulls.

  6. kc says:

    I totally disagree .. haha

    why not reinvent the bluescreen+poweroff in windows for all errors .. then you would REALLY know something had gone wrong and can start assembler-coding.

  7. Daniel Wu says:

    >>why not reinvent the bluescreen+poweroff in windows for all errors .. then you would REALLY know something had gone wrong and can start assembler-coding.

    @kc
    If windows was done by “defensive programming” you will never see bluescreen, but, *sometimes* your mouse cannot click anything, no error reported; *sometimes* the explorer shows duplicated files and you can only delete one of them, no error dialog; *sometimes*, one of your credit card transaction will fail but windows tells you success but no invoice printed, no error dialog; because of the following line:

    if (transaction != null && transaction.responseCode==OK) {
    printInvoice(transaction);
    } else {
    //defensive, hide the problem, my boss won’t kick my ass because it will take at least a week for the customer to find out the mythic periodical issue, hooray!
    }

    so, spending 10+ times effort to debug the issue (defensive programming), or got a call from your customer at the first day your system goes live (fail-fast), which one you want?

  8. sc says:

    if the variable can’t be null, your method should enforce it:
    if(arg == null) throw new ArgumentNullException(“arg is null”);

    otherwise let it through.

    your app should an error logging/handling layer that decides to display errors in a friendly manner so the end user never sees a 500 (or any code error). To show a system error message (in the terms of something code-wise unexpected, not a “friendly” error message) to a user is just bad user experience design. It proves you never actually thought about the end user’s experience through your application.

  9. Christian Schlichtherle says:

    I think you show how the term defensive programming leads to a public misperception. The examples you show try to accept any parameter values, even if they are illegal. This isn’t really defensive programming, but rather a silently failing garbage-in/garbage-out effect.

    Consider the fail-fast approach: When given parameters in a public method which must not be null, dereference them early or check explicitly. The effect is a healthy NullPointerException to tell the caller that they did something wrong.

    In fact, I consider the fail-fast approach to be part of a defensive-programming strategy.

    There is one more important thing to consider: The range of legal parameter values must be documented! For brevity of your examples I guess, you left out the Javadoc. It should be clearly documenting what is a legal value however. This task can be simplified by using a @NonNull or @Nullable or @CheckForNull annotation like they are provided in FindBugs.

  10. Barry Kelly says:

    Would you ever do the following?

    Integer one = 1;
    Integer two = one + one;

    if (two > one) {
    return “the world is right: 2 > 1 is true”;
    }

    The reason why you’d do something like that is for integer overflow (but rather you’d do something like “if (two < one) { overflow_error(); }"). If this wasn't blindingly obvious to you, I don't want you anywhere near code in a language with fixed-sized integers.

  11. Giorgio says:

    I would code findFavoriteFriends() differently, and still use defensive programming:

    public List findFavoriteFriends(Person person)
    {
    try
    {
    List friends = person.getFriends();
    List favoriteFriends = new ArrayList();
    for (Friend friend : friends)
    {
    if (friend.isFavorite())
    {
    favoriteFriends.add(friend);
    }
    }

    return favoriteFriends;
    }
    catch (NullPointerException ex)
    {
    return null;
    }
    }

    Of course, in a language where dereferencing a NULL pointer crashes the application, you would have to use more if’s. But notice that I return a NULL pointer in case of error, which does provide information to the code that called findFavoriteFriends().

  12. james says:

    The scope of defensive programming is to avoid unknown compiler issues, it’s true that most people don’t need to do null-checks or even explicit boolean checks because they know the code that their compiler will produce, but there are some cases where from one version to another or from some update to another the compiler changes and it could produce a possible flaw without the programmer knowing about it, in case nobody reads their compiler user’s guide, it’s a must at least have a copy for reference (and the manuals for all the tools used).

    I remember reading about a GNU/Linux kernel`s bug because gcc didn’t set a boolean variable value when created, and inside a conditional it was readed as false and it was very hard to find.

    A lot of C++ code can be hidden with macros (i know a lot people don’t like them because they look not so straight-forward).

  13. Jannik says:

    A bit late in the discussion, but I want to add a (in my view) crucial point:

    it really depends on the domain of your program.
    If you are programming an IM (Twitter client, Mail, someting not really life-saving), then you might want to fail it early and often to find all problems and fix bugs.
    If you programm the anti-lock brake system in a car and are already in production, I rather don’t want it to fail completely, because somebody missed some check in the programm because of “fail early and often”.

    Of course one does not overdue it. So if the system is broken, the warning light should appear in the cockpit and not be filtered out by an overly defensive program.

  14. Pingback: Failfast programming for the lazy programmer

  15. Really?? The examples you have shown are nothing compared to what I have to deal with DAILY. Apart from the numerous lines of closing braces due to extra null checks, finding a bug is a nightmare. The program is too big to fail they say (like a bank). It should just give rubbish output. Guess how many hours it takes, stepping through lines and lines of code, writing down variable values down on paper and comparing them. And people wonder why one would be p***d when interrupted for say a Scrum meeting. They don’t know that the process has to start all over again.
    Personally, I would prefer my program to crash than to have it output wrong data.

  16. Shawn Brock says:

    I think you should re-title this to “Bad Defensive Coding Is Rubbish”. When I think of good defensive coding, it is the anti-thesis to what you have here. I do null checks – and then I crash if they fail. Continuing with unexpected data is awful. They way you seem to define it is I could wrap everything in “ON ERROR GOTO NEXT”… which is to me the opposite of defensive coding. Maybe I don’t know what I’m talking about, but I have always seen it as, “Query database, wrapped in try/catch; if fail give detailed error message and end gracefully.”

    Anyway, nice read.

    • Shawn Brock says:

      And I guess to clarfiy – any time my program doesn’t do exactly what I expect, I want it to fail, but fail gracefully. The “gracefully” is the key to me. I don’t want blue screens in Windows if the mouse fails. I want a message saying, “the mouse driver had an unexpeceted error, try again”. Or something like that.

  17. Alex Shyshkov says:

    Totally wrong!

    The author makes up some badly written code to prove that the right concept is wrong.

    The code samples are deliberately made unreadable to prove poor readability of the code.

    Ans sadly, the defensive programming methods were not used in those samples.

    Alex

  18. Joel says:

    Did I miss something? Passing in a NULL pointer is the kind of bug you look for in Alpha and Beta testing; if any such event occurs, log it fully and don’t go the field until you stop getting this kind of log message.

  19. VBAsassin says:

    I wouldn’t consider the null checks to be part of “defensive coding” but more part of bad design i.e. objects doing to much or incorrectly constructed. The null checks issue can often (but not always) be reduced by correctly using factories, inheritance and polymorphism. This is why i would not consider this part of “defensive coding”.

    What I would consider “defensive coding” is more like when you said “failfast” near the end of your article. Checking the combination of values given to a factory were correct and hence, fails before the object is even constructed.

    Nice article though and interesting to see how much you detest all the null checks.

    Kind regards,
    Scott

  20. Pingback: Find the bug | Andrzej's C++ blog

  21. Pingback: Defensive null checks | Bad Java Coder

Leave a Reply

Your email address will not be published. Required fields are marked *

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>