Steve Wellens had a recent blog post arguing for the use of a goto in C# (see: Why goto Still Exists in C#). Steve had a series of methods he wants to execute, but he wants to stop if any given method returns false. At the end of the post, Steve decided that the following code with goto was better than setting boolean variables:
// DoProcess using goto void DoProcess3() { LOG("DoProcess Started..."); if (Step1() == false) goto EXIT; if (Step2() == false) goto EXIT; if (Step3() == false) goto EXIT; if (Step4() == false) goto EXIT; if (Step5() == false) goto EXIT; EXIT: LOG("DoProcess Finished"); }
In the comments, a remark from a different Steve stood out. Steve suggested using an array of Func<bool>. The comment didn’t generate any further discussion, but it’s worthy to call out.
I don’t think the problem with the above code is with the goto per-se. I think the problem is how the code conflates “what to do” with “how to do it”. In this scenario both are a little bit tricky. Let’s assume we might need to add, remove, or change the order of the method calls. But, the method calls are so intertwined with conditional checks and goto statements that it obscures the process. Using an array of Func<bool> is a simple approach, yet it still manages to create a data structure that isolates “what to do”.
void Process() { Func<bool>[] steps = { Step1, Step2, Step3, Step4, Step5 }; ExecuteStepsUntilFirstFailure(steps); }
You could argue that all this code does is push the problem of “how to do it” further down the stack. That’s true, but we’ve still managed to separate “what” from “how”, and that’s a big win for maintaining this code. The simplest thing that could possibly work for “how” would be:
void ExecuteStepsUntilFirstFailure(IEnumerable<Func<bool>> steps) { steps.All(step => step() == true); }
The All operator is documented as stopping as soon as a result can be determined, so the above code is equivalent to the following:
void ExecuteStepsUntilFirstFailure(IEnumerable<Func<bool>> steps) { foreach (var step in steps) { if (step() == false) { break; } }
}
With this approach it’s easy to change the order of the steps, or to add steps and delete steps, without worrying about missing a goto or conditional check. The next step up in complexity (excuse the pun) would be to create a Step class and encapsulate the Func with other metadata and state. I’m sure you could also imagine the execution phase relying on an IStepExector interface as the base for executing steps under a transaction, or with step-level logging, or even in parallel – and all this without changing how the steps are arranged. Take this to an extreme, and you’ll have a technology like Windows Workflow Foundation. :)
The ability of functional and declarative programming to separate the “what” and the “how” is powerful, but you don’t need a new language, and you can start simple. In this scenario it’s another tool you can use to save your city from the goto-zilla monster.
What do you think?
Comments
void ExecuteStepsUntilFirstFailure(params Func<bool>[] steps)
{
steps.All(step => step() == true);
}
So now it's:
ExecuteStepsUntilFirstFailure(
Step1,
Step2,
Step3,
Step4,
Step5
);
I haven't tried compiling that, but I think it'd work.
I do like your alternative, though.
e.g. in VB.net
Private Sub DoProcess3()
Try
LOG("DoProcess Started...")
If Step1() = False Then Exit Try
If Step2() = False Then Exit Try
Finally
LOG("DoProcess Finished")
End Try
End Sub
void DoProcess3()
{
LOG("DoProcess Started...");
bool success = (Step1() && Step2() && Step3() && Step4() && Step5());
LOG("DoProcess Finished");
}
If you don't want to use newfangled things like short-circuiting, or your check's more complicated than a boolean comparison, you could always use more than one function.
void DoProcess3()
{
LOG("DoProcess Started...");
DoSteps();
LOG("DoProcess Finished");
}
void DoSteps()
{
if (Step1() == false)
return;
if (Step2() == false)
return;
if (Step3() == false)
return;
if (Step4() == false)
return;
if (Step5() == false)
return;
}
An array of Func<T>s is useful when you have a *variable set of functions* to execute and you want to write a generic function to do it:
void DoLoggedActions(Func<T> funcs)
{
LOG("DoProcess Started...");
bool success = funcs.All(f => f());
LOG("DoProcess Finished");
}
// DoProcess using functions
void DoProcess()
{
LOG("DoProcess Started...");
DoProcessInternal();
LOG("DoProcess Finished");
}
void DoProcess()
{
if (Step1() == false)
return;
if (Step2() == false)
return;
if (Step3() == false)
return;
if (Step4() == false)
return;
Step5();
}
--- or ---
if I wanted everything in one method I could do this
// DoProcess using while
void DoProcess3()
{
LOG("DoProcess Started...");
do
{
if (Step1() == false)
break;
if (Step2() == false)
break;
if (Step3() == false)
break;
if (Step4() == false)
break;
Step5();
}
while (false);
LOG("DoProcess Finished");
}
Before someone says SRP or something like that, remember YAGNI.
Code should be readable. A simply try/catch (with each step throwing a StepFailedException) would have done wonders.
return Step1()
&& Step2()
&& Step3()
&& Step4()
&& Step5()
;
is a pretty obtuse way of phrasing what you want, though, isn't it? yes, yes, of course the actual behaviour does what you want, but it feels more like it does what you want as a side-effect, than because that's what it's for. All() has a couple of problems here. One: it returns a bool, and using it without capturing the return value because all you want are the side effects is pretty abusive of the API. And two: it's called 'all'. Even though it is guaranteed to shortcircuit, its name implies that it will operate on every member of the enumeration.
The fact you're evaluating a bool for equality with 'true' is also a bit of a codesmell - since it's completely unnecessary, you're presumably trying to communicate something to the maintenance coder about the significance of the result value from the function. If we're adding unnecessary code to make it more comprehensible, I'd suggest putting 'bool allOperationsSucceeded = ' before the .All() call to indicate what the method is doing.
LINQy code always becomes awkward when you write lambdas with side-effects (and in this case, the side effects are actually the entire effect you're after). It has the same awkwardness as having side effects in the control condition of a while loop - it leaves a nasty taste.
In this case, foreach with a break on a fail would be much clearer in its intent.
if (step1() == true) {
} else if (step2() == true) {
} else if (step3() == true) {
...
}
void DoProcess() {
LOG("DoProcess Started...");
try {
if (!step1()) return;
if (!step2()) return;
if (!step3()) return;
if (!step4()) return;
} finally {
LOG("DoProcess Complete.");
}
}
One additional remark about the code itself: what's the added value in defining the function ExecuteStepsUntilFirstFailure? Maybe the name of "All" is not choosen too well, but creating a seperate method doesn't solve this...
void Process()
{
Func<bool>[] steps =
{
Step1,
Step2,
Step3,
Step4,
Step5
};
steps.All(step => step() == true);
}
Yes, I thought All wasn't very descriptive. Even worse would be something like Any() == false. That was the only reason for a separate method.
// DoProcess using goto
bool DoProcess3()
{
LOG("DoProcess Started...");
return Step1()
&& Step2()
&& Step3()
&& Step4()
&& Step5();
LOG("DoProcess Finished");
}
That seems much simpler to me than all the other framework required for the functional solution.
joe
if (step1() && step2() && step3() .. &step6())
{
}
// log it...
They will run in order until one is false.
It's useful in winservice that runs behind my web site www.postjobfree.com.
There are multiple methods that I need to run from winservice (e.g. GenerateCommentJobNotifications, EmailPreparedNotifications, RecalculateAllCategoryJobBlobs, DeleteResumeBlobsForDeletedResumes and many more).
All methods have no parameters, are in separate library and tested separately.
My goal is to have as little code as possible in winservice to run all that methods in sequence.
Every method call has to be wrapped in try-catch block in order to make sure that all methods in sequence are successfully executed.
I trimmed your code a little and use "Action[] steps" instead of "Func<bool> steps".
So now my winservice code is getting cleaner, because in essence I need only to specify list of methods to call, without all that junk like try-catch-log.
The minimal amount of code in winservice is critical, because winservice code is hard to test, and also because we frequently add, remove, or move methods between different timer handlers (EveryMinuteSequence(), Every10MinutesSequence(), EveryNightSequence()).
Great tip, thanks again!
Early return wins the day again.
IEnumerable<bool> Operations()
{
yield return Step1();
yield return Step2();
yield return Step3();
}
To run them:
bool allGood = !Operations().Select(r => !r).FirstOrDefault(r => r);
Not that there's much point to this, for such a simple case. If there was some more complex setup/teardown logic to put around each step of the sequence, I could understand it. Otherwise, why not use && or try/finally?
Even reading Daniel's solution was fun, though I [obvioiusly] would never use it.
This type of banter is really important to flush out ideas, and come up with the best practices of our craft.
The more we talk and debate about our technique, the better our product will be. I really believe this.
Yes, a number of people have suggested this.
I think it's ok, but it really doesn't express intent the way I like, and the moment you have to do something else (like log per method call), things get tricker. that's why I'd like to keep the order and the execution separate.