Defensive coding by Mark Seemann
This post examines the advantages and disadvantages of defensive coding.
One of my readers, Barry Giles, recently wrote me and asked a question that I think is interesting enough to warrant a discussion:
"Recently I came up against an interesting situation at work: I was getting a review and I had put defensive checks in – one for a null argument check against a constructor parameter, one for a null check on a property return value. I also had some assertions for private methods which I tend to use to state my assumptions.
"It seems the prevailing mood in my team is to not have any of the checks and just let it fail. To be honest I’m struggling with this concept as I am so used to developing in this way and thought it was good practice. I’m pretty sure it’s in most tutorials and blog posts.
"Can you advise on why it’s better to code defensively like this instead of just letting it fail and checking the stack trace please?"
Actually, I don't think defensive coding necessarily is better; I also don't think it's worse. As usual, there are different forces in play, so the short answer is the usual: it depends.
That answer is obviously useless unless you can answer the question: on what does it depend? In this article, I'll examine some of the forces at play. However, I'm not going to claim that this will be an exhaustive treatment of the subject.
Letting it fail #
What is the value of defensive coding, if all you're going to do is to immediately throw an exception? Wouldn't it be just as good to let the code fail? Let's look at some code.
public class ProductDetailsController { private readonly IUserRepository userRepository; private readonly IProductRepository productRepository; private readonly ICurrencyExchange exchange; public ProductDetailsController( IUserRepository userRepository, IProductRepository productRepository, ICurrencyExchange exchange) { this.userRepository = userRepository; this.productRepository = productRepository; this.exchange = exchange; } public ProductDetailsViewModel GetProductDetails( string userId, string productId) { var user = this.userRepository.Get(userId); var targetCurrency = user.PreferredCurrency; var product = this.productRepository.Get(productId); var convertedPrice = this.exchange.Convert(product.UnitPrice, targetCurrency); return new ProductDetailsViewModel { Name = product.Name, Price = convertedPrice.ToString() }; } }
In this simple ProductDetailsController, the GetProductDetails method produces a ProductDetailsViewModel instance by getting user and product information from injected Repositories, converting the product unit price to the user's desired currency, and returning data about the product for display purposes. For the sake of argument, let's focus only on null issues. How many things could go wrong in the GetProductDetails method? How many objects can be null?
Quite a few, it turns out. Even decoupled from its dependencies, this small piece of code can throw a NullReferenceException in at least six different ways. Imagine that you receive an error report from your production system. The stack trace points to the ProductDetailsViewModel method, and the exception is a NullReferenceException. Which of the six possible nulls caused the error?
By just letting the system fail, it's hard to answer that question. Keep in mind that this is even a demo example. Most production code I've seen has more than five to six lines of code, so looking for the cause of an error report can quickly become like looking for a needle in a haystack.
Just letting the code fail isn't particularly helpful. Obviously, if you write very short methods (a practice I highly recommend), the problem is less pronounced, but the longer your methods are, the less professional I think 'just letting it fail' sounds.
Defensive coding to the rescue? #
By adding explicit guards against null, you can throw more descriptive exception messages:
public class ProductDetailsController { private readonly IUserRepository userRepository; private readonly IProductRepository productRepository; private readonly ICurrencyExchange exchange; public ProductDetailsController( IUserRepository userRepository, IProductRepository productRepository, ICurrencyExchange exchange) { if (userRepository == null) throw new ArgumentNullException("userRepository"); if (productRepository == null) throw new ArgumentNullException("productRepository"); if (exchange == null) throw new ArgumentNullException("exchange"); this.userRepository = userRepository; this.productRepository = productRepository; this.exchange = exchange; } public ProductDetailsViewModel GetProductDetails( string userId, string productId) { var user = this.userRepository.Get(userId); if (user == null) throw new InvalidOperationException("User was null."); var targetCurrency = user.PreferredCurrency; var product = this.productRepository.Get(productId); if (product == null) throw new InvalidOperationException("Product was null."); var convertedPrice = this.exchange.Convert(product.UnitPrice, targetCurrency); if (convertedPrice == null) throw new InvalidOperationException("Converted price was null."); return new ProductDetailsViewModel { Name = product.Name, Price = convertedPrice.ToString() }; } }
Is this better? From a troubleshooting perspective it is, because with this version of the code, if you get an error message from your production system, the exception message will tell you exactly which of the six null situations your program encountered. If I were in maintenance mode, I know which of the two versions I'd like to support.
However, from a readability perspective, you're much worse off. The GetProductDetails method grew from five to eleven lines of code. Defensive coding more than doubled the line count! The flow through the method drowns in guard clauses, so it's less readable now. If you're a typical programmer, you read code much more than you write it, so a practice that makes it harder to read code should trigger your code smell sense. No wonder that many programmers consider defensive coding a bad practice.
Robustness #
Is it possible to balance the forces at play here? Yes it is, but in order to understand how, you need to understand the root cause of the problem. The original code example isn't particularly complicated, but even so, there are so many ways it can fail. When it comes to null references, the cause is a language design mistake, but in general, the question is whether or not you can trust input. The return values from invoking IUserRepository.Get is (indirect) input too.
Depending on the environment in which your program is running, you may or may not be able to trust input. Consider, for a moment, the situation where your software is running in the wild. It may be a reusable library or framework. In this case, you can't trust input at all. If fact, you may want to apply the robustness principle and make sure that, not only are you going to be very defensive about input, but you're also going to be careful and nice about the output of your program. In other words, you don't want to pass null (or other evil values) to your collaborators.
The sample code presented here may pass null to its dependencies, e.g. if userId
is null, or (more subtly) if user.PreferredCurrency
is null. Thus, to apply the robustness principle, you'd have to add even more defensive coding:
public class ProductDetailsController { private readonly IUserRepository userRepository; private readonly IProductRepository productRepository; private readonly ICurrencyExchange exchange; public ProductDetailsController( IUserRepository userRepository, IProductRepository productRepository, ICurrencyExchange exchange) { if (userRepository == null) throw new ArgumentNullException("userRepository"); if (productRepository == null) throw new ArgumentNullException("productRepository"); if (exchange == null) throw new ArgumentNullException("exchange"); this.userRepository = userRepository; this.productRepository = productRepository; this.exchange = exchange; } public ProductDetailsViewModel GetProductDetails( string userId, string productId) { if (userId == null) throw new ArgumentNullException("userId"); if (productId == null) throw new ArgumentNullException("productId"); var user = this.userRepository.Get(userId); if (user == null) throw new InvalidOperationException("User was null."); if (user.PreferredCurrency == null) throw new InvalidOperationException("Preferred currency was null."); var targetCurrency = user.PreferredCurrency; var product = this.productRepository.Get(productId); if (product == null) throw new InvalidOperationException("Product was null."); if (product.Name == null) throw new InvalidOperationException("Product name was null."); if (product.UnitPrice == null) throw new InvalidOperationException("Unit price was null."); var convertedPrice = this.exchange.Convert(product.UnitPrice, targetCurrency); if (convertedPrice == null) throw new InvalidOperationException("Converted price was null."); return new ProductDetailsViewModel { Name = product.Name, Price = convertedPrice.ToString() }; } }
This is clearly even more horrendous than the previous defensive programming example. Now you're not only defending yourself, but also standing up for your collaborators. Noble, but unreadable.
Still, when I write wildlife code, this is basically what I do, although I'd tend to refactor my code so that first I'd collect and check all the input, and then I'd pass on that input to another class that performs the logic.
Protected enclosures #
What if your code is a zoo animal? What if your code is running in an environment, where all the collaborators are other classes also part of the same code base, written by you and your colleagues? If you can trust each other to follow some consistent rules, you could skip much of the defensive coding.
In most of the teams I work with, I always suggest that we adopt the robustness principle. In practice, that means that null is never an acceptable return value. If a class member returns null, the bug is in that class, not in the consumer. Following that team rule, the code example can be reduced to this:
public class ProductDetailsController { private readonly IUserRepository userRepository; private readonly IProductRepository productRepository; private readonly ICurrencyExchange exchange; public ProductDetailsController( IUserRepository userRepository, IProductRepository productRepository, ICurrencyExchange exchange) { if (userRepository == null) throw new ArgumentNullException("userRepository"); if (productRepository == null) throw new ArgumentNullException("productRepository"); if (exchange == null) throw new ArgumentNullException("exchange"); this.userRepository = userRepository; this.productRepository = productRepository; this.exchange = exchange; } public ProductDetailsViewModel GetProductDetails( string userId, string productId) { if (userId == null) throw new ArgumentNullException("userId"); if (productId == null) throw new ArgumentNullException("productId"); var user = this.userRepository.Get(userId); if (user.PreferredCurrency == null) throw new InvalidOperationException("Preferred currency was null."); var targetCurrency = user.PreferredCurrency; var product = this.productRepository.Get(productId); if (product.Name == null) throw new InvalidOperationException("Product name was null."); if (product.UnitPrice == null) throw new InvalidOperationException("Unit price was null."); var convertedPrice = this.exchange.Convert(product.UnitPrice, targetCurrency); return new ProductDetailsViewModel { Name = product.Name, Price = convertedPrice.ToString() }; } }
That's better, but not quite good enough yet... but wait: readable properties are return values too, so you shouldn't have to check for those either:
public class ProductDetailsController { private readonly IUserRepository userRepository; private readonly IProductRepository productRepository; private readonly ICurrencyExchange exchange; public ProductDetailsController( IUserRepository userRepository, IProductRepository productRepository, ICurrencyExchange exchange) { if (userRepository == null) throw new ArgumentNullException("userRepository"); if (productRepository == null) throw new ArgumentNullException("productRepository"); if (exchange == null) throw new ArgumentNullException("exchange"); this.userRepository = userRepository; this.productRepository = productRepository; this.exchange = exchange; } public ProductDetailsViewModel GetProductDetails( string userId, string productId) { if (userId == null) throw new ArgumentNullException("userId"); if (productId == null) throw new ArgumentNullException("productId"); var user = this.userRepository.Get(userId); var targetCurrency = user.PreferredCurrency; var product = this.productRepository.Get(productId); var convertedPrice = this.exchange.Convert(product.UnitPrice, targetCurrency); return new ProductDetailsViewModel { Name = product.Name, Price = convertedPrice.ToString() }; } }
This is pretty good, because now you're almost back where you started. The only difference is the Guard Clauses at the beginning of each member. When following the robustness principle, most members tend to look like that. After a while, you get used to that, and you don't really see them any longer. I consider them a sort of preamble for each member. As a code reader, you can skip the Guard Clauses and concentrate on the program flow, without any interrupting defense checks interfering with the readability of the code.
Encapsulation #
If it's an error to return null, then how does the User class, or the Product class, adhere to the robustness principle? In exactly the same way:
public class User { private string preferredCurrency; public User() { this.preferredCurrency = "DKK"; } public string PreferredCurrency { get { return this.preferredCurrency; } set { if (value == null) throw new ArgumentNullException("value"); this.preferredCurrency = value; } } }
Notice how the User class protects its invariants. The PreferredCurrency property can never be null. This principle is also known by another name: it's called encapsulation.
Summary #
As always, it helps to understand the forces working or your code base. You have to be much more defensive if your code is running in the wild, than if it's running in a protected environment. Still, I generally think that it's a fallacy if you believe that you can get by with writing sloppy code; we should all be Ranger programmers.
Unstructured defensive coding hurts readability; it's just another excuse for writing Spaghetti Code. Conversely, structured defensive coding is encapsulation. I know what I prefer.