There's a school of thought that methods should have only a single exit point. This school teaches that all the logic paths through the method should end in a single place from which there is a return of control and the method result (if any) to the caller. The arguments I have seen for this include alleged benefits in readability and correctness. I find these claims to be spurious.
If is my hypothesis (for which I've done no research whatsoever) that single exit point comes from unmanaged environments and this is where I first encountered it. Unmanaged environments require that resources be explicitly cleaned up and a single exit point in this situation means that the cleanup logic need not be duplicated. In .NET resources either do not need cleanup or better constructs (such as the using statement and Standard Dispose Pattern) exist that deal with much of the cleanup requirement in a fashion that does not require code duplication. Hence, much like Hungarian notation I feel single exit point to be a holdover from a previous environment who's (questionable) utility is long past.
In considering the disadvantages of the single exit point if must first be understood that a .NET method will have multiple exit points implicitly. Take for instance this rather simple example:
public static BusinessObject FindByCriteria(IEnumerable<BusinessObject> businessObjects, DecisionCriteria decisionCriteria)
{
if (businessObjects == null)
{
throw new ArgumentNullException("businessObjects");
}
if (decisionCriteria == null)
{
throw new ArgumentNullException("decisionCriteria");
}
BusinessObject result = null;
foreach (BusinessObject businessObject in businessObjects)
{
if (businessObject.MatchesCriteria(decisionCriteria))
{
result = businessObject;
break;
}
}
return result;
}
This example uses variable result to store the instance of BusinessObject that it finds. If no object matches the original value that the variable is set to (null) is used. In both cases the method returns from the last line. At least it does if we assume everything is OK.
If either of the parameters is null this method (which validates its arguments) with throw an ArgumentNullException. In these cases the method does not return from the exit point at the end. This is explicit and you could argue (not particularly well in my opinion) that this is an obvious special case. However there are still failure points in the method that are not covered. For instance if businessObjects contains a null value the method will fail with a NullReferenceException. We could guard against that by doing a null check in the if clause, but it may be that the list containing null is invalid, in which case we'd probably want to throw an exception explicitly, meaning another exit point. It is also possible that MatchesCriteria will throw an exception.
You could wrap the entire thing in a try-catch block and swallow any exceptions, but this is hardly the way to make robust software. Unless you want to return to the days of having result codes and checking them on every method call (some people do, please take their compilers away) exceptions are a reality of current development environments.
Even if we allow exceptions as a special case single exit point is still sub-optimal. I can tweak the method above to be:
public static BusinessObject FindByCriteria(IEnumerable<BusinessObject> businessObjects, DecisionCriteria decisionCriteria)
{
if (businessObjects == null)
{
throw new ArgumentNullException("businessObjects");
}
if (decisionCriteria == null)
{
throw new ArgumentNullException("decisionCriteria");
}
foreach (BusinessObject businessObject in businessObjects)
{
if (businessObject.MatchesCriteria(decisionCriteria))
{
return businessObject;
}
}
return null;
}
In this modified example we've made the method simpler. The variable is gone and in my opinion the behaviour is more explicit. This code clearly shows that as soon as we find something we return it to the caller and that null is the default return value (previously this was inferred from the setting of the variable value at declaration). It's not a huge change but not every improvement has to be, the effect of multiple changes is cumulative.
At this point someone will argue that if the method has additional logic then the variable will be required and I haven't improved anything. These people are wrong. This method is reasonably cohesive. It may be reused in any case where a BusinessObject matching a DecisionCriteria needs to be found. I can use the method in other logic and its name will make it apparent what is happening without me having to understand the logic of how the BusinessObject is found. Therefore arguing that the method may have additional logic in this example is wrong because adding additional logic to the method is itself wrong.
If you have multiple distinct paths through a method which you choose the correct option is not to have a nested method that gathers a result and returns it at the end of whichever path you choose. The correct option is to extract each path into a method then select a method to execute and return its result. There are multiple ways to do this, such as this.
Where methods have an execution path that diverges multiple times and includes common sections I'd argue even more strongly that logic needs to be broken into multiple methods (or otherwise refactored, for instance using strategies). The more paths there are through a method the more difficult it is to validate and the more likely you will experience bugs. Real world conditions are an excellent way to produce circumstances your consider impossible and a really terrible place to encounter them. It is by simplifying your logic by breaking it up into smaller more manageable pieces that we can best address this.
In summary, single exit point fails because:
- It's not necessary anymore due to improvements in how we can clean up resources.
- It doesn't match the reality of current managed environments.
- It promotes more complex and involved code that is harder to understand and test and more likely to fail.
As a last note, I am aware that as of C# 3.0 the above method is obsolete. The following is how I would write this code today:
var result = businessObjects.FirstOrDefault(businessObject => businessObject.MatchesCriteria(decisionCriteria));
This uses FirstOrDefault from System.Linq.Enumerable to return a value or a default (null) if no matching value is found. The First method is useful if you wish to fail if the collection does not contain a matching value. This code is sufficiently simple that I'd likely use it inline but for more complicated predicates extracting a method to encapsulate it may be appropriate.