One of my personal pet peeves when working on code is encountering logic that checks for some unexpected state and when that unexpected situation applies... does nothing. This is a classic case of sweeping mistakes under the carpet. Consider the following C# code:
In this example we check the state of the order and if it is not in the expected state for processing, we do nothing. The problem here is that if the ProcessOrder() method gets called on an order that is not in the right state for processing, this is most certainly a bug in the code that is calling the method. By ignoring the unexpected state, the ProcessOrder() method is hiding the issue, making the underlying bug much harder to locate.
This is because the code calling ProcessOrder() will assume that after the method had been called, the order had been successfully processed. The if-condition in ProcessOrder() makes it so that this assumption is not true so that the calling code might proceed interacting with an unprocessed order as if it were a processed order. This would cause potential havoc, diverting attention away from the source of the problem.
The core problem with ProcessOrder() is that there are situations where the method would successfully return despite not having done what it was expected to do, i.e. processing the order.
The Solution
My personal rule of thumb for preventing this kind of situation is that a method should always do what it claims to do in its name (in this case processing the order), or otherwise throw an exception. If the method returns without an exception, this must imply that the operation was completed successfully. This is the central point of this post, so I will state it explicitly:
A method must return without throwing an exception if and only if it successfully did what it claims to do in its name. In any other case an exception must be thrown to indicate that what the method was expected to do could not be done.
Therefore a better implementation of the method above should look like:
The Try-Parse Pattern
There can be operations where it is not unusual that the operation can't be performed for one reason or another. That is, the reason for the operation not being able to be performed is not due to an error or a programming bug, but merely a valid possible scenario. Since exceptions are supposed to be used for exceptional cases only, such a situation calls for a different approach. Consider the following method signature:
The caller should expect that if the method successfully returns, it would provide a Route leading from the provided source to the destination. If there is no such route, then based on the method's name, it should throw an exception because it wasn't able to "calculate the route". But what if the non-existence of a route is a common and valid use-case? We could make the method return null if no route could be calculated, but this would amount to a false impression from the perspective of the caller that the route was calculated when that's not the case.
This kind of scenario is often a good candidate for the try-parse pattern. Based on this pattern the method signature above would change into:
Now, if no route is found, the method would return false to indicate the fact. When it does, it's not "lying" about having performed its task because it did, in fact, try to calculate the route like its name states.
The Microsoft documentation about the try-parse pattern presents this as a much more performant alternative to throwing exceptions where exceptions would be thrown for common scenarios. Another benefit of using this pattern is that it makes it clear to the method caller that a certain case needs to be handled explicitly.
Making a habit of ensuring all methods always do what their names say (or throwing an exception otherwise), makes it easier to reason about the code and helps make the source of programming bugs be more obvious when they occur.
Top comments (0)