Functional Programming Battles GOTOzilla

Tuesday, June 16, 2009

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:

Functional programming battles the GOTO // 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
Matt Hamilton Tuesday, June 16, 2009
I like it. You could perhaps even take it a step further:

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.
Joel Tuesday, June 16, 2009
I think I would have written the original function with a try/finally block and return statements instead of goto, but I guess it would still have been a mixture of "what" and "how". I also don't see much point in comparing method calls with boolean return values to a literal boolean, but I guess that boils down to personal preference.

I do like your alternative, though.
Mike Tuesday, June 16, 2009
You are assuming that the steps all have the same signature. This is as much an issue of language syntax as it is style.

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

Ilia Jerebtsov Wednesday, June 17, 2009
When all you have is a hammer... Come on, Func<T>s to do basic structured programming?

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");
}
David L. Penton Wednesday, June 17, 2009
The GOTO solution bothers me for obvious reasons. The functional array seems like a screw & hammer solution. It is inventive, has its purpose, good to know, but I'd prefer something along these lines:

// 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.
BBQNinja Wednesday, June 17, 2009
I know that it's "cool" nowadays to eschew exceptions but seriously... How is that goto even a contender? And the func<bool> is a neat trick, but it's working around a CREATED problem.

Code should be readable. A simply try/catch (with each step throwing a StepFailedException) would have done wonders.
Rik Hemsley Wednesday, June 17, 2009
With the order of evaluation of && guaranteed as being LTR:

return Step1()
&& Step2()
&& Step3()
&& Step4()
&& Step5()
;

James Hart Wednesday, June 17, 2009
steps.All(step => step() == true)

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.
s awkward wh Wednesday, June 17, 2009
WTF is wrong with else?

if (step1() == true) {
} else if (step2() == true) {
} else if (step3() == true) {
...
}
Craig Wednesday, June 17, 2009
I think C# people are slowly discovering some of the things that makes Lisps great. :)
blorg Wednesday, June 17, 2009
// I'd do this with try/finally and returns
void DoProcess() {
LOG("DoProcess Started...");
try {
if (!step1()) return;
if (!step2()) return;
if (!step3()) return;
if (!step4()) return;
} finally {
LOG("DoProcess Complete.");
}
}
Doeke Zanstra Wednesday, June 17, 2009
For starters, I don't think you can't say which is the best solution, until you know what a "step" is, and what a "process" has to accomplish.

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);
}

scott Wednesday, June 17, 2009
@BBQ and @Doeke:

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.
joe Wednesday, June 17, 2009
I always liked the short circuit operators for things like this. Since I would actually want to know if the process succeeded or not, I would change the signature to return a bool and then I would have:

// 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
Kevin L. Kitchens Wednesday, June 17, 2009
How about

if (step1() && step2() && step3() .. &step6())
{
}

// log it...

They will run in order until one is false.
Dennis Gorelik Wednesday, June 17, 2009
Scott, thanks for the nice tip.
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!
askheaves Thursday, June 18, 2009
blorg has the exactly right and only correct possible solution (!) It's clear what the function does and executes as expected.

Early return wins the day again.
Daniel Earwicker Thursday, June 18, 2009
Another possibility, going for the prize for the most esoteric trendy solution:

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?
Brian Genisio Friday, June 19, 2009
This conversation thread is a blast to read. Seriously. Is Scott's method the best way? Probably not. BUT, it is more readable, IMO, than the GOTO mechanism.

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.
scott Friday, June 19, 2009
@Brian: Yes, I've enjoyed the comments, as surprising as some of them may be!
Paul Friday, June 19, 2009
Wow...I've seen a lot of very diverse solutions here...someone had mentioned trying to find the esoteric one. For my money, the best solutions out there was the two folks who SIMPLY said to AND the functions together. They will execute left-to-right and stop when one of them hits false. The others are kinda neat, but when you really need to get something done quickly that someone else is going to read and/or maintain, who has time to go through all of the other stuff??
scott Saturday, June 20, 2009
@Paul:

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.
Comments are now closed.
by K. Scott Allen K.Scott Allen
My Pluralsight Courses
The Podcast!