What’s wrong with that – robust and unquestionable code

I recently had a session about proper exception handling and, more intensively, defensive programming. As exception handling is one of the most important as well as one of the most unattented tasks of a developer, the session was based on real code from the code base of the products.

Don’t get me wrong here: It is not about nerving the developers. It is about showing with a real example what can be done wrong and how to improve.

From my point of view, defensive programming is not just “checking inputs” but a mindset that leads to more robust and unquestionable code. This can only be accomplished when the developer not just codes the “happy case” plus he/ she is aware of every problem that this piece of code will be able to handle. Certainly, it is much easier to do that, when the code itself is based on certain principles that

• classes are small, methods are small. We are used to say, a class with more than 50 lines is “big”. More than 150 is subject to be refactored
• allows the class to focus on exactly one responsibility, following the single responsibility principle. When considering possible issues within a certain class, there is certainly less work when the class itself has only one reason to change. Anyway, there can be a lot of issues even then
• allows to easily incorporate “services” that provides functionality that the current class uses to fulfill the necessary actions. This again makes code more simple, understandable and reduces the possible count of exceptions. This follows the D in SOLID.

Let’s start with this sample. All the arrows show places in code that are not defensive, and therefore just not well thought through.

First of all – there is no exception handling at all here. Sure there is a try catch and there is even logging, with the usual log4net interface. But what does it tell to the caller of the method? It swallows the actual exception, so the caller will not be able to understand if calling the function was successful or not. Additionally – and that makes me a little nervous – it writes a debug message in case of an exception? There are things that make me angry and this is part of it. Whoever wrote that method didn’t think at all. I always expect the developers to well thought through every single line of code.

Also, if you’ve ever read don’t make me think which actually applies to UX/ UI, I tend to use the term also in coding. If the code makes me think, it isn’t well written, well structured or thought to be understandable for the next guy maintaining your code.

But let’s start from the beginning, just listing what kind of problems this code will raise:

• if Selection is null, or not an Excel.GroupObject which can easily be the case as Selection is an object and can contain pretty much everything, this will lead to a pretty ugly COMException.
• As we use DI, this piece of code  “_excelApplicationManager?” make me nervous. This is complete overuse of ? operator which came with Visual Studio 2015. As services are always added to a class in ctor and saved in readonly field variables, this question mark ruins my piece.
• So this service give you “Application” which is the Excel.Application object. I would like to see a method here that is called “GetApplication()” as it should anyway not possible to write to that property. Certainly a get-only-property would be okay, but I tend to think, GetApplication() would be more readible.
• When that application object is null, what then? The ? operator helps in detecting that and makes that more readible, but the code really doesn’t handle that at all. This also applies to “ActiveWorkbook”.
• I really don’t understand “Excel” here in that case because I would expect that the ActiveWorkbook property is used to get one of the sheets of that particular workbook. The term “Excel” really doesn’t fit and makes me think.
• Last but not least AlternativeText is some kind of risky, as every property in Excel can be changed via the User Interface or via VBA, so if there is no worksheet being called like the text in AlternativeText, the code will be broken ungracefully.

Let’s stop here for a moment. That have just been two lines of code! A lot of thoughts that actually should have been done before writing. But let’s go one with that analysis before I am going to shed some light on possible improvements.

• SetActiveChart returns a boolean. Normally I would think, it would like to tell me if the actions was successful or not – where I would prefer no return value at all, but an exception when failing. Yes, throwing exceptions is expensive and time consuming but this doesn’t look like high performance code, right? Unluckily SetActiveChart returns an indication whether the chart is “new” or not. This is pretty unexpected, and makes me think again.
• ViewQuickAccessBar does give me an impression what will be accomplished, but  this method should be renamed to “ShowQuickAccessBar”, as the code won’t view anything. Yes, the code is not named from the perspective of the user.
• Next method call “ComparePositions” need a closer look. What is the return value? Is it true or false when position matched?

There are even more, but let’s stay with these and come up with some solutions:

• Chained calls shall be avoided as much as possible („Don‘t tell the legs of the dog to run, tell the dog to run“). The used services should apply most of these functions and also implement the exception handling.
  • If Application object is not available
  • If there is no ActiveWorkbook
  • If a certain sheet is not part of the sheets collection of the ActiveWorkbook
  • If the selection contains a GroupObject
• SetActiveChart must implemented more robust behavior – it is not visible by this piece of code, but the method would also fail ungracefully when a null value would be passed over. Additionally, detecting if a chart is new or not must be separated to another function
• ComparePositions must be renamed to DoPositionsMatch() or AreSamePositions() to indicate what kind of information will come back
• I expect a log message on at least every public method when the methods starts. We kept that pretty simple with log.Debug(). This will take the caller’s method name and push it to the log, so the effort for the programmer is close to no hurdles at all.
• The exception handling is not appropriate at all.
  • Check if the incoming information is what the method needs to perform its task. If it doesn’t throw exceptions that the caller can handle – even if it is only stopping the action and raising a message box. That is surely much more appropriate then a not-working or unexpected-working program
  • Throw strong typed exceptions in several cases
    • When the AlternativeText is not set to a worksheet that is present. The AlternativeText is set by the program and should not be changed from the outside and will rarely be done. Nevertheless it can happen
    • Everything can fail, activation, repositioning, etc. The developer has to decide which of these situations he wants to handle with strong-typed exceptions or just generic ones. The decision certainly should be based on the fact how manageable situations are. Excel can be just busy for a moment and return something like “try later” ComException. The code should be aware of it.

Please – don’t make me think. And don’t be that lazy to have more work afterwards. This is going to be not maintainable, the user will probably have bad experience. I personally don’t want to waste my time with bug fixing that I can avoid from the beginning and so should you.