Security programming

One of my readers, Barry Giles, recently wrote to me and asked a rather interesting question, which, in my opinion, is worthy of discussion:

“Recently, I ran into one interesting situation at work: I did a review of the code and inserted security checks — one to check the constructor argument to null, one to check the null value returned from the property. I also had statements for closed methods, which I used to explicitly state my assumptions.
“It seems that the prevailing practice among my teammates is to omit checks and allow falls. To be honest, I’m struggling with this concept, as I’m used to developing through defensive programming and considered it a good practice. I am pretty sure that this is also the case for most of the manuals and blog entries.
“Could you give advice on why it is better to program in a defensive style, instead of letting the code fail and then checking the stack trace?”

Generally speaking, I do not think that defensive programming is worse or better than another approach. As usual, various factors influence the solution of a problem, so the short answer will be this: it all depends on the circumstances.
Such an answer is certainly useless as long as we cannot answer the question: from what circumstances does this dependence arise? In this article, I will consider some factors. However, I am not going to state that this article is a comprehensive statement of the topic.

Letting the code fall.

What is the value of defensive programming, if all you are going to do is to immediately throw an exception? Wouldn't it be a good decision to just let the code fall? Let's look at sample 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 class, the GetProductDetails method creates an instance of ProductDetailsViewModel , getting user and product information through repository injections, converting the price of the product into the user's preferred currency and returning the product data that is supposed to be displayed on the screen. In order to achieve the goal of the article, let's concentrate only on the problems of checking for null . How many calls can go through a GetProductDetails script in the GetProductDetails method? How many objects can be null links?
Quite a lot, as it turns out. Even separated from its dependencies, this small piece of code can throw a NullReferenceException , at least in six cases. Imagine that you get an error report from your in-use system. The stack trace indicates the GetProductDetails method and the type of exception thrown is NullReferenceException . Which of the six possible objects with a zero reference caused an error?
Letting the system simply fall, it will be difficult for us to answer this question. And do not forget that this is just a learning example. Most of the code I’ve exploited had more than 5 or 6 lines of code, so finding out the cause of the error could quickly become something like a needle search in a haystack.
Just letting the code fall is not very useful. Obviously, if you write very short methods (a practice that I strongly recommend), the problem that arises is not so urgent, but the longer your methods are, the less professional, in my opinion, the phrase “just let the code fall” sounds.

Protective rescue programming?

By adding explicit gatekeys to null , you can throw out more descriptive error 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 code better? In terms of analyzing the causes of errors, yes. In this version of the code, if you receive an error message from your operating system, the message in the exception will tell you which of the six possible situations with the null link your program encountered. If I were in support mode, I would know which version of the two presented I would prefer.
However, in terms of readability, everything has greatly deteriorated. The GetProductDetails method GetProductDetails extended from five to eleven lines of code. Security programming has more than doubled the number of lines! Passing through the body of the method is drowning in protective structures, so the method has become worse read. If you are a typical programmer, then you read the code significantly more than you write it, so the practice that makes your code more difficult to read should make you feel plowed. There is no doubt that many programmers consider defensive programming a bad practice.

Sustainability.

Is it possible to balance the factors influencing the problem and its solution? Yes, it is possible, but in order to understand how, you must understand the root cause of the problem. The initial code sample is not particularly complicated, but even so, there are many cases in which this code fails. As for null references, the reason is a mistake in the design of the language , but in general, the question is whether you can trust the input data or not. Returning values ​​via a call to IUserRepository.Get this (indirectly) is also input.
Depending on the environment in which your program operates, you either have the opportunity to trust the input data, or you do not have such an opportunity. Imagine, for a moment, the situation in which your software is operated in the "wilderness . " Your software can be a reusable library or framework. In this case, you can’t trust the input at all. If this is the case, then you may want to apply the principle of stability and remain convinced that you are acting in a protective style not only with respect to the input, but also the output. In other words, you do not want to pass null references (or other devilish values) to another interacting code.
The sample code given earlier may pass null links to its dependencies, for example, if userId = null, or (more sophisticated), if user.PreferredCurrency = null. Thus, following the principle of sustainability, you would have to add even more protective expressions:

  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 obviously looks even more awful than the previous example using the defensive style. Now you are protecting not only yourself, but also interacting code. Commendable, but completely unreadable.
Until now, when I write code that lives in the "wilderness", such defensive programming is exactly what I do, although I still tend to refactor in such a way that, first, I would collect and I checked all the input data and then I would transfer this data to another class that implements all the logic.

Protected area.

What if your code lives in a protected area ? What if your code works in an environment, all interacting code is part of the same code base written by you and your colleagues? If you can trust each other by following certain sequential rules, you can omit most of the protective structures.
In most of the teams in which I worked, I always assumed that we use the principle of sustainability. In practice, this means that null never a valid return value. If the class member returns null , then the bug is in this class, not in the consumer of this class. Taking into account the following of this rule, the previous code can be shortened 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() }; } } 

This is better, but still not good enough ... but wait: the properties for reading are also returned values, so we can not check them 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() }; } } 

Now it's quite good, since now we are almost back to the original state of the code. The only difference is the presence of the gatehouse at the very beginning of each member. When you follow the principle of sustainability, most of the members begin to look about the same. A little later, after you get used to it, you stop noticing them. I consider them a preamble for each member. As a reader of the code, you can skip the gatelines and concentrate on the flow of logic, without interrupting you security checks that interfere with the reading of the code.

Encapsulation.

If returning a null link is an error, then how can the User class or the Product class follow the sustainability principle? In 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: encapsulation .

Conclusion

As always, understanding the factors underlying the problem or understanding your codebase helps. You should write significantly more defensive constructions in case your code lives in the “wilderness” than when it lives in a protected environment. To this day, I consider it a delusion to believe that you can get away with writing a sloppy code; we must all be rangers programmers.
A structureless security code harms readability. The security code is just another excuse for writing spaghetti code. In contrast, structured defensive programming is encapsulation. I know what I prefer.