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. Pingback: Failfast programming for the lazy programmer

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

  3. Pingback: Defensive null checks | Bad Java Coder

Leave a Reply

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