My opinions and choices on style of coding evolves and changes over time. By blogging about this now, I can refer back later when I change my mind. I can also elicit the opinions of others to influence me to change my ways if I happen to be wrong and not know it yet.
Lets talk about returns. There is an opinion that methods should only have a single return. This gives them a precise flow of control, and forces you to essentially either process data through a few processes and return the final result (which can be very functional) or store a result variable and correctly populate it within control flow logic (‘if’/’else’, ‘switch’, ‘while’ commands, for instance). If you don’t actually return a result, you can still think about flow of control and where you exit from.
public string WhenIPipeDataIntoOtherFunctions(string fileName)
{
var input1 = Loader.LoadInput();
var input2 = FileStore.ReadAllContent(fileName);
return WhenIUseAResultVar(input1, input2);
}
public string WhenIUseAResultVar(int input1, string input2)
{
string result;
if (input1 > 5)
{
var part1 = new string('a', input1);
result = part1 + " " + input2;
}
else
{
result = null;
}
return result;
}
public void DoStuff(string foo, int bar)
{
if (bar > 10)
{
var something = WhenIUseAResultVar(bar, foo);
OutputGuy.Take(something);
}
else
{
var somethingElse = WhenIPipeDataIntoOtherFunctions();
Printer.Print(somethingElse);
}
}
The first one seems self explainatory, but I will come back to that one soon. Lets discuss ‘WhenIUseAResultVar’.
WhenIUseAResultVar
I have a method with a very awkward control flow. But here is the catch: This control flow actually is enforced (in C#) by the compiler that result must be initialised.
My method could decalare at the start that ‘result’ is initialised to ’null’, telling me that a null could be returned if all logic falls through. In this case I have gone for the opposite, which means the compile ensures that all my cases through my control flow logic will set the value of result. for building and modifying this kind of method is like a free unit test.
For example, lets modify it as follows:
public string WhenIUseAResultVar(int input1, string input2)
{
string result;
if (input1 > 5)
{
var part1 = new string('a', input1);
if (input2 == "Business")
{
result = part1 + " " + input2;
}
}
else
{
result = null;
}
return result;
}
This code actually will not compile. It has compiler error CS0165 Use of unassigned local variable 'result.
For me, this is gold.
Breaking Example Three
To explain later why I like doing this, even when there is no return value, lets look at an alternative way to write ‘DoStuff’:
public void DoStuff(string foo, int bar)
{
if(bar > 10)
{
var something = WhenIUseAResultVar(bar, foo);
OutputGuy.Take(something);
return;
}
var somethingElse = WhenIPipeDataIntoOtherFunctions();
Printer.Print(somethingElse);
}
It might look better to you for now, but stick with me. I will refer back to this in a moment.
Why I break my own rules.
I don’t usually break this rule. But when I do, I do so strictly and with intent.
Preconditions and validations.
There is a time and a place to exit early, and it is preconditions and validations. throwing OutOfRangeException ArgumentNullException ArgumentException and other preconditions make sense to exit early.
To prove this, Lets try to strictly applying my ’laws’ to a slightly more detailed version of the WhenIUseAResultVar method:
public string WhenIUseAResultVar(int input1, string input2)
{
string result;
if (input1 <= 0)
{
// Technically, this is already an early return.
throw new IndexOutOfRangeException();
}
if (string.isNullOrEmpty(input2))
{
// Technically, this is already an early return.
throw new IndexOutOfRangeException();
}
if (input1 > 100)
{
string result = "Case 1 Result";
}
else if (input1 > 5)
{
var part1 = new string('a', input1);
result = part1 + " " + input2;
}
else
{
result = "Some Fallback Result";
}
return result;
}
Hmmm… turns out this is a poor example and I can’t actually acheive it with exceptions. For arguments sake, lets use ’null’ as failed, rather than throwing.
public string WhenIUseAResultVar(int input1, string input2)
{
string result;
if (input1 <= 0)
{
result = null;
}
else if (string.isNullOrEmpty(input2))
{
result = null;
}
else if (input1 > 100)
{
string result = "Case 1 Result";
}
else if (input1 > 5)
{
var part1 = new string('a', input1);
result = part1 + " " + input2;
}
else
{
result = "Some Fallback Result";
}
return result;
}
I can already feel the pain of this. What I would actually do:
public string WhenIUseAResultVar(int input1, string input2)
{
if (input1 <= 0) return null;
if (string.isNullOrEmpty(input2)) return null;
string result;
if (input2 > 100)
{
string result = "Case 1 Result";
}
else if (input1 > 5)
{
var part1 = new string('a', input1);
result = part1 + " " + input2;
}
else
{
result = "Some Fallback Result";
}
return result;
}
I may even use the single line format above without the braces on my returns, to further distinguish them for critical method logic.
When there isn’t any return value
Even when you have a ‘void’ method, the method still returns.
Remember DoSomething from above? Let’s do something similar to above and strictly apply a single point of return (i.e. the end of the method).
public void DoStuff(string foo, int bar)
{
if (input1 > 0 &&!string.isNullOrEmpty(input2) && input2 < 100)
{
if (bar > 10)
{
var something = WhenIUseAResultVar(bar, foo);
OutputGuy.Take(something);
}
else
{
var somethingElse = WhenIPipeDataIntoOtherFunctions();
Printer.Print(somethingElse);
}
}
}
Or
public void DoStuff(string foo, int bar)
{
if (input1 > 0)
{
if (!string.isNullOrEmpty(input2))
{
if (input2 < 100)
{
if (bar > 10)
{
var something = WhenIUseAResultVar(bar, foo);
OutputGuy.Take(something);
}
else
{
var somethingElse = WhenIPipeDataIntoOtherFunctions();
Printer.Print(somethingElse);
}
}
}
}
}
Or
public void DoStuff(string foo, int bar)
{
if (input1 <= 0)
{
}
else if (string.isNullOrEmpty(input2))
{
}
else if (input2 > 100)
{
}
else if (bar > 10)
{
var something = WhenIUseAResultVar(bar, foo);
OutputGuy.Take(something);
}
else
{
var somethingElse = WhenIPipeDataIntoOtherFunctions();
Printer.Print(somethingElse);
}
}
Is this an improvement over alternatives? Not really in my opinion. (Remember I said this was my all about my opinions?) I actually felt worse and worse as I typed out each successive example. (I think I am trying to extract out intent by the above refactorings, but by this point I can’t be sure).
Again, with breaking the rule on purpose:
public void DoStuff(string foo, int bar)
{
if (input1 <= 0) return;
if (string.isNullOrEmpty(input2)) return;
if (input2 > 100) return;
if (bar > 10)
{
var something = WhenIUseAResultVar(bar, foo);
OutputGuy.Take(something);
}
else
{
var somethingElse = WhenIPipeDataIntoOtherFunctions();
Printer.Print(somethingElse);
}
}
I’m liking this. Get validation out of the way, get on with main logic control flow.
Exceptions
I’ve already touched on this. Exceptions break the flow of control. Exceptions are early returns already. They also shouldn’t be actually used for flow of control. So it’s probably fine? And since I am happy with validations performing early return, these fit that case anyway. Consistency! (Phew!)
If my method actually returns Errors as a use case, I opt for something like I do with my Beefeater library and have Result<TValue,TError> response objects, and control flow rule (and early-return-for-validation exception-to-sed-rule) applies again.
An extra section about short circuiting
I’ve written these on the assumption that you do not want to evaluate more conditions than you need to.
For instance, to make the WhenIUseAResultVar example more readable we could have pulled the conditional checks into named variables:
public string WhenIUseAResultVar(int input1, string input2)
{
if (input1 <= 0) return null;
if (string.isNullOrEmpty(input2)) return null;
string result;
var isLargeCase = input2 > 100;
var isSmallCase = input1 > 5;
if (isLargeCase)
{
string result = "Case 1 Result";
}
else if (isSmallCase)
{
var part1 = new string('a', input1);
result = part1 + " " + input2;
}
else
{
result = "Some Fallback Result";
}
return result;
}
This aids readability, but is functionally different to the first example. Even if isLargeCase is true, we still evaluate and execute isSmallCase. In the original example, isLargeCase being true shortcircuits and stops the evaluation of isSmallCase entirely. If this was a real example, and isSmallCase required extra database access or web calls, you probably don’t want to always evaluate this up front.
Having said that, If the cost of execution is low, and the improvement to readability and comprehensibility is high, Then do it. Also if the condition is that complex, push it out into a method call anyway:
if(theSkyIsBlue(timeOfDay))
{
...
}
else if (ASimpleCheckPasses(input1) && AWebCallCheck(input1, input2))
{
....
}
(Thanks [Graham Mace}(https://github.com/maceage) for the addition. )
Conclusion
I’m not even sure if I have actually convinced myself 100% at this point. Reality is, I have all of these techniques in my tool box, and I will probably pick and choose to enhance readability where applicable. Pragratism usually wins in these matters, but I find it useful to stop and think about concepts like this from time to time.
I was originally prompted to write this after reading the lecterror blog article Code formatting and readability and related StackOverflow question Should I return from a function early or use an if statement?.
(I thought, “Hey! I have opinions too! And a Blog!”)