How to prevent the arrowhead anti-pattern

Question

I'm a bit confused about how to best refactor my code into something more readable.

Consider this piece of code:

var foo = getfoo();
if(foo!=null)
{
    var bar = getbar(foo);
    if(bar!=null)
    {
        var moo = getmoo(bar);
        if(moo!=null)
        {
            var cow = getcow(moo);
            ...
        }
    }
}
return;

As you can see, lots of nested if blocks, needed because each nested if is dependent on the previous values.

Now I was wondering how to make my code a bit cleaner in this regard.

Some options I have thought of myself would be to:

return after each if clause, meaning I'd have multiple places where I leave my method
throw ArgumentNullExceptions, after which I'd catch them in the end and place the return statement in my finally clause (or outside the try/catch block)
work with a label and goto:

Most of these options seem a bit "dirty" to me, so I was wondering if there was a good way to clean up this mess that I've created.

This question is probably better suited at http://codereview.stackexchange.com/ — Spontifixus, Jul 23 '13 at 07:33
Why or how could foo, bar, moo or cow be null? Is it expected that they could be null or are you just trying to be safe? — Tobsey, Jul 23 '13 at 07:33
Monads! Seriously http://codepyre.com/2012/08/safe-dereference-inverse-null-coalescing-support-for-c-sharp/ — sehe, Jul 23 '13 at 07:35
@Spontifixus: yeah, wasn't sure which place would be better suited, ended up here because of laziness (didn't have to re-log) — Kippie, Jul 23 '13 at 07:38
@Kippie : You are right. I removed my answer since fixed version would look like GolezTrol's answer. — Mechanical Object, Jul 23 '13 at 07:44
possible root cause here is breaking http://en.wikipedia.org/wiki/Law_of_Demeter — jk., Jul 23 '13 at 09:50
not sure what the issue is, that block of code seems perfectly readable. — acolyte, Jul 23 '13 at 13:31
Related to: [Indented if/else approach](http://codereview.stackexchange.com/questions/5453/indented-if-else-approach) — Alfredo Osorio, Jul 23 '13 at 13:49
@sehe: Monads are the best solution to this, but that article has nothing to do with monads. — SLaks, Jul 23 '13 at 15:01
See Haskell's Maybe monad. http://en.wikipedia.org/wiki/Monad_(functional_programming)#The_Maybe_monad — SLaks, Jul 23 '13 at 15:02
This question appears to be off-topic because it is about code-review. — antony.trupe, Jul 23 '13 at 15:32
An extremely similar question was asked just last week: http://stackoverflow.com/questions/17672481/cleaner-way-to-do-a-null-check-in-c/ — Kevin, Jul 23 '13 at 16:14
Arrowhead is wrong in that is inherently [non-orthogonal](https://softwareengineering.stackexchange.com/a/122625/11110) and represents a maintenance and version control conflict disaster. — hlovdal, Oct 08 '21 at 13:17
give a look at this article that might give you some idea about a bit different approach: [Yet another way to deal with nested if/else conditions](https://joshgunh.medium.com/yet-another-way-to-deal-with-nested-if-else-conditions-176057a419d) — Joshgun, Nov 23 '22 at 08:25

score 47 · Accepted Answer · edited Jul 23 '13 at 18:50

47

I'd go for the multiple return statements. This makes the code easy to read and understand.

Don't use goto for obvious reasons.

Don't use exceptions because the check you are doing isn't exceptional, it's something you can expect so you should just take that into account. Programming against exceptions is also an anti-pattern.

edited Jul 23 '13 at 18:50

Peter Majeed

5,304
2
32
57

answered Jul 23 '13 at 07:35

Gerrie Schenck

22,148
20
68
95

Ended up accepting this as my final answer as it clearly explains why I *shouldn't* use the other 2 options I put forward. Upvoted rexcfnghk's answer for providing a clear example. – Kippie Jul 23 '13 at 07:47
15

Maybe I'm spoilt, but if I want to get a Foo and don't get one, I find that exceptional. I mean, I can expect everything. If I want to open a file, I can expect it to be locked, but still the results are different from my goal. Because I expect the software to do what I want, I think this is an exception. Or do you mean that try..catch should be banned completely, just like goto? – GolezTrol Jul 23 '13 at 07:55
5

The example above is clearly about business logic, to check for a condition that could occur. I would not use exceptions here. Your example is about an external resource that is not behaving as it should be, which is exceptional. – Gerrie Schenck Jul 23 '13 at 09:38
2

It should be noted why the arrowhead pattern exists, as well. By maintaining exactly one return statement instead of many, you reduce certain measures complexity. If your organization is using these measures, then using multiple return statements will impact the metrics. – atk Jul 23 '13 at 12:35
9

I don't see any obvious reason why not to use `goto`. `return` is best, followed by `return+try+finally`, but this is the exact case where `goto` is an acceptable option in cases if the clean-up is complex (in C++ RAII should handle everything, but C# finally does add the level of indentation and so is not equivalent). – Jan Hudec Jul 23 '13 at 14:12
@atk I've heard this argument too, but I find methods that do basic valid input checking at the top to be less complex. This is the same idea - checking a valid state of the application or loaded files / classes and exiting before the bulk of the work. – Kieveli Jul 25 '13 at 13:07
1

@Kieveli: I agree with you - checking the input and returning an error status immediately is a preferable approach. My comment was meant to point out that what we prefer isn't always how we're measured, and we need to take both into account when choosing how we will proceed :) – atk Jul 25 '13 at 13:12

score 44 · Answer 2 · answered Jul 23 '13 at 07:34

44

Consider inverting the null checks to:

var foo = getfoo();
if (foo == null)
{
    return;
}
var bar = getbar(foo);
if (bar == null)
{
    return;
}
...etc

answered Jul 23 '13 at 07:34

rexcfnghk

14,435
1
30
57

18

consider not using brackets for one line conditionals, use something like `if (foo == null) return;` – Lee Fogg Jul 23 '13 at 14:49
6

@LeeAllan Just an aside, but sometimes the coding convention used will dictate whether it's OK or not to use brackets on one line statements. – Shaz Jul 23 '13 at 15:17
75

consider not following @LeeAllan's suggestion. – antony.trupe Jul 23 '13 at 15:35
3

@LeeAllan I always include brackets for one line conditionals. IMHO, it's much easier to debug, especially when the indentation is messed up in your code... – rexcfnghk Jul 24 '13 at 01:25
Always using brackets is indeed a good practice, but if you have a very short `if` body, I think it is ok to have them all on the same line: `if (foo == null) { return; }`. – Felix Dombek Jul 24 '13 at 17:29
1

@LeeAllan apart from other reasons, in such non-typical program flow returns there'll be a debug entry before the return and the brackets will be necessary anyway. For this reason alone it's better to write them immediately. – Dariusz Jul 25 '13 at 05:23
3

They're called BRACES. Brackets are these-> [] – MikeFHay Aug 23 '13 at 15:35

score 27 · Answer 3 · answered Jul 23 '13 at 07:40

27

You can chain expressions. An assignment returns the assigned value, so you can check its result. Also, you can use the assigned variable in the next expression.

As soon as an expression returns false, the others are not longer executed, because the entire expression would return false already (because of the and operation).

So, something like this should work:

Foo foo; Bar bar; Moo moo; Cow cow;

if( (foo = getfoo()) != null &&
    (bar = getbar(foo)) != null &&
    (moo = getmoo(bar)) != null &&
    (cow = getcow(moo)) != null )
{
  ..
}

answered Jul 23 '13 at 07:40

GolezTrol

114,394
18
182
210

1

Upvoted this as it's a great idea, but it would make my code pretty much unreadable in the end: The above example is oversimplified, and this would probably even make things worse for me. – Kippie Jul 23 '13 at 07:43
1

I was afraid of that. I just wanted to show this way of chaining, because it can be very usefull to write more compact code. But I use the 'check and return' as suggested by @rexcfnghk a lot as well. – GolezTrol Jul 23 '13 at 07:48
2

If the code is bigger and more complex, you can move the code inside an if to a separate method. I think that's a better approach than having multiple separate chunks of code above each other. Especially if you are tempted to add 'comment seperators', explaining what the next part does, it is a sign that you need to break up your code. – GolezTrol Jul 23 '13 at 07:57
Initialize initialize initialize! Also, that code makes me cringe. – Kieveli Jul 25 '13 at 13:08
@Kieveli Could you explain please? – GolezTrol Jul 28 '13 at 22:45
This code is much more difficult to debug than code with several "return". – KindDragon Jul 31 '13 at 22:01

score 26 · Answer 4 · answered Jul 23 '13 at 16:36

26

This is one of few scenarios where it is perfectly acceptable (if not desirable) to use `goto`.

In functions like this, there will often be resources that are allocated or state changes that are made mid-way which need to be undone before the function exits.

The usual problem with return-based solutions (e.g., rexcfnghk's and Gerrie Schenck's) is that you need to remember to undo those state changes before every return. This leads to code duplication and opens the door to subtle errors, especially in larger functions. Do not do this.

CERT actually recommends a structural approach based on `goto`.

In particular, note their example code taken from copy_process in kernel/fork.c of the Linux kernel. A simplified version of the concept is as follows:

    if (!modify_state1(true))
        goto cleanup_none;
    if (!modify_state2(true))
        goto cleanup_state1;
    if (!modify_state3(true))
        goto cleanup_state2;

    // ...

cleanup_state3:
    modify_state3(false);
cleanup_state2:
    modify_state2(false);
cleanup_state1:
    modify_state1(false);
cleanup_none:
    return;

Essentially, this is just a more readable version of the "arrowhead" code that doesn't use unnecessary indentation or duplicate code. This concept can easily be extended to whatever best suits your situation.

As a final note, especially regarding CERT's first compliant example, I just want to add that, whenever possible, it is simpler to design your code so that the cleanup can be handled all at once. That way, you can write code like this:

    FILE *f1 = null;
    FILE *f2 = null;
    void *mem = null;

    if ((f1 = fopen(FILE1, "r")) == null)
        goto cleanup;
    if ((f2 = fopen(FILE2, "r")) == null)
        goto cleanup;
    if ((mem = malloc(OBJSIZE)) == null)
        goto cleanup;

    // ...

cleanup:
    free(mem); // These functions gracefully exit given null input
    close(f2);
    close(f1);
    return;

answered Jul 23 '13 at 16:36

user1354557

2,413
19
29

4

I hope people actually read your answer before downvoting. We developers have quite the knee-jerk reaction to GOTO, and I think you address it beautifully. – Jesse Smith Jul 23 '13 at 17:33
4

I feel that this is only appropriate for C (or other languages w/o exeptions) This feels like a C re-implementation of try/finally. Can you convince me this is better (or at least different) for an upvote? – tobyodavies Jul 24 '13 at 03:38
1

This is super useful in C. The second option, I mean, not the first one (which is just as ugly and verbose). Write simple initialization code, and have your cleanup code handle any failures. – Thomas Jul 24 '13 at 07:24
1

+1, "single exit point" is a common practice to reduce code complexity in functions. goto doesn't hurt readability in those cases. – Sedat Kapanoglu Jul 24 '13 at 09:21
1

In C++, you can automate the resource cleanup with the RAII idiom, by wrapping the resource aquisition in the constructor and the release in the destructor of a little class (in C++11, you can also pass a custom *deleter* function to a `unique_ptr` for the same effect). Not sure how far this is applicable to other languages, but this leads to even cleaner code than lots of `goto cleanup`s. – Felix Dombek Jul 24 '13 at 15:45
@tobyodavies: Well, they are certainly different. If used improperly, try/finally could catch unexpected/unguarded exceptions in child functions, obscuring potential errors. And if abruptly leaving the child function creates an inconsistent state, this could be a vulnerability. Exceptions negatively impact performance as well, but this obviously only matters if you expect to fail rather often. Otherwise, try/finally is conceptually similar to the second example. However, for more complex cleanup code (like the first), try/finally doesn't work well. – user1354557 Jul 25 '13 at 15:21
@Thomas: Although it can often be avoided by modularizing your code, there are cases where the second option doesn't apply as well. Imagine, for example, that you need to remove several individual elements from complex data structures or undo certain transformations. In these cases, I find that the first option is easier to maintain. Sometimes your only other option is to keep a set of booleans that denote which changes you've made that need to be undone at `cleanup`. This is more verbose than using multiple cleanup labels, but I suppose that is a matter of preference. Both are good solutions. – user1354557 Jul 25 '13 at 15:35
1

@user1354557 True, I was mostly thinking of flat memory allocations (with maybe a couple levels) but there is a case for nested or hierarchical data structures. But this goes to say that `goto` is occasionally the right solution and one shouldn't shy away from it. In any case, embracing `goto` in my C code has made my life a lot simpler (in more ways than one). – Thomas Jul 26 '13 at 02:44
https://xkcd.com/292/ – k314159 Jun 13 '23 at 15:27

score 16 · Answer 5 · answered Jul 23 '13 at 07:51

16

First your suggestion (return after each if clause) is quite a good way out:

  // Contract (first check all the input)
  var foo = getfoo();

  if (Object.ReferenceEquals(null, foo))
    return; // <- Or throw exception, put assert etc.

  var bar = getbar(foo);

  if (Object.ReferenceEquals(null, bar))
    return; // <- Or throw exception, put assert etc.

  var moo = getmoo(bar);

  if (Object.ReferenceEquals(null, moo))
    return; // <- Or throw exception, put assert etc.

  // Routine: all instances (foo, bar, moo) are correct (not null) and we can work with them
  ...

The second possibility (in your very case) is to slightly modify your getbar() as well as getmoo() functions such that they return null on null input, so you'll have

  var foo = getfoo();
  var bar = getbar(foo); // return null if foo is null
  var moo = getmoo(bar); // return null if bar is null

  if ((foo == null) || (bar == null) || (moo == null))
    return; // <- Or throw exception, put assert(s) etc.    

  // Routine: all instances (foo, bar, moo) are correct (not null)
  ...

The third possibility is that in complicated cases you can use Null Object Desing Patteren

http://en.wikipedia.org/wiki/Null_Object_pattern

answered Jul 23 '13 at 07:51

Dmitry Bychenko

180,369
20
160
215

2

+1 for Null Objects pattern. It's tricky to get used to, but it can certainly simplify code like this if you apply it right. – GolezTrol Jul 23 '13 at 10:33
Can you explain your use of `Object.ReferenceEquals` for your first example solution but `if (foo == null)` expressions for the second? – deed02392 Jul 23 '13 at 11:22
@deed02392: ReferenceEquals means that reference (not instance) is equal to null; this kind of comparison can't be change. If reference is null it is null. On the contrary, you can override Equals method and ==/!= operators such that == or Object.Equals will never say that you instance is equal to null (just return false in Equals and == operator and true in != operator) even when it's null. – Dmitry Bychenko Jul 23 '13 at 11:36
3

Need to be careful with Null object pattern; its not always appropriate, and while you eliminate run time exceptions, you can still get unexpected behavior if you rely on the object actually doing something. I've found NOP is best for cases where you want optional logic to be plugged in, but not where you must do something. – Andy Jul 23 '13 at 12:10
1

And you can even drop all but last condition in the final if -- if any of them is null, the last is null and if the last is not null, neither of the preceding are. So it's enough to `if (moo == null) return`. – Jan Hudec Jul 23 '13 at 14:17

OldCurmudgeon · Answer 6 · 2013-07-24T22:24:17.603

12

Do it old-school:

var foo;
var bar;
var moo;
var cow;
var failed = false;
failed = failed || (foo = getfoo()) == null;
failed = failed || (bar = getbar(foo)) == null;
failed = failed || (moo = getmoo(bar)) == null;
failed = failed || (cow = getcow(moo)) == null;

Much clearer - no arrow - and extendable forever.

Please do not go over to the Dark Side and use goto or return.

edited Jul 24 '13 at 22:24

answered Jul 23 '13 at 14:20

OldCurmudgeon

64,482
16
119
213

If foo is null `failed` will become `true` and no other get will be called because `|=` will not allow it. At least - that would be true in `C` and `C++` - am I wrong for `C#`? – OldCurmudgeon Jul 23 '13 at 14:34
I think it should have been `||=`. Not familiar with `C#`. – OldCurmudgeon Jul 23 '13 at 14:40
2

This is correct as currently written, but the reason why it works is not the best-known feature of logical operators (short-circuiting; if the first expression of an OR is true, or false for an AND, then the second condition is never checked). – KeithS Jul 23 '13 at 15:48
3

This is a derivation of the concept of a [Guard](http://en.wikipedia.org/wiki/Guard_%28computer_science%29). – OldCurmudgeon Jul 23 '13 at 15:53
3

+1 imho the best solution on here, but i'd use `var validArgument = true` and use `&=` operator + `!= null` check, because readability says you don't want to negate in the following if clause – Christian R. Jul 23 '13 at 23:49
1

@KeithS any programmer who doesn't understand short-circuiting is not a programmer. I can't think of a single (non-esoteric) language that does not have exactly those semantics for logical operators, or a single application I've worked on that doesn't rely on them somewhere. – tobyodavies Jul 24 '13 at 03:41
||= doesn't exist as a C# operator does it? – Firedragon Jul 24 '13 at 08:54
@tobyodavies It's 15 years old now, but many programmers still (have to) work with it today … VB6 – Felix Dombek Jul 24 '13 at 15:51
@Firedragon unfortunately no, and neither does it exist in C++, or C, or Java. – Felix Dombek Jul 24 '13 at 16:07
@Felix Dombek see [c#](http://msdn.microsoft.com/en-gb/library/h5f1zzaw(v=vs.80).aspx) - [vc++](http://msdn.microsoft.com/en-us/library/5bk6ya5x(v=vs.71).aspx) - [java](http://docs.oracle.com/javase/tutorial/java/nutsandbolts/operators.html) – Christian R. Jul 24 '13 at 16:20
The shortcutting `or` assignment **does** exist in C, C++ and Java but as `|=`. The C# `|=` does not shortcut because the C# `|` does not shortcut (you have to use `||` to shortcut in expressions). It looks like there is no shortcutting `or` assignment in C# but if there were it would almost certainly be `||=` and the above method would then work. – OldCurmudgeon Jul 24 '13 at 16:22
@OldCurmudgeon @Christian R. I don't know where you have this info from, I can't find it in your linked documents, and MSVC++10: `bool boolfalse() { std::cout << "boolfalse\n"; return false; } int main(){ bool bt = true; bt |= boolfalse(); }` will print `boolfalse` which IMHO means that it does not short-circuit; neither does GCC http://ideone.com/CiDISV – Felix Dombek Jul 24 '13 at 17:18
@FelixDombek tested and in fact, you are right - the existing assignment operator doesn't short circuit like i expected it - stated in the definitions of [c#](http://stackoverflow.com/a/13047189/2188682) and [java](http://docs.oracle.com/javase/specs/jls/se7/html/jls-15.html#jls-15.7.2) (and to lazy to search for c/c++). nonetheless, OldCurmudgeons style of avoiding returns/gotos is still the best answer imho, but without shortcircuiting the validity bool has to be reassigned in each step with the `||` operator. – Christian R. Jul 24 '13 at 17:56
@ChristianR. I tested it in Java too, http://ideone.com/77eRKU and it does not support `||=` and it does not short-circuit `|=`. So I think we can safely say that neither C#, nor C++, nor C, nor Java support short-circuiting compound assignment operators. (Your linked documents don't say anything on this topic, really.) – Felix Dombek Jul 24 '13 at 18:38
I stand corrected - there are no shortcutting assignment operators in C# or Java. I have modified my post. My apologies for misleading you all. However, the technique still stands. – OldCurmudgeon Jul 24 '13 at 22:26

score 5 · Answer 7 · answered Jul 23 '13 at 16:20

5

var foo =                        getFoo();
var bar = (foo == null) ? null : getBar(foo);
var moo = (bar == null) ? null : getMoo(bar);
var cow = (moo == null) ? null : getCow(moo);
if (cow != null) {
  ...
}

answered Jul 23 '13 at 16:20

Rex Kerr

166,841
26
322
407

1

I was about to post a similar strategy then saw this. This is the way I usually do it, too. While some folks do take exception to the "redundant" conditions (e.g. you could argue that if `foo == null` then the following checks serve no purpose), it is very easy to read, and, more importantly, *very* easy to modify. You can add steps in the middle, you can modify earlier logic to be as complex as needed, etc., because at no point do any of the conditions care about *how* the value they were testing was obtained. Every piece is completely self-contained, and there's only one single return point. – Jason C Apr 29 '17 at 17:49

score 3 · Answer 8 · edited Jul 23 '13 at 13:10

3

If you can change the stuff you are calling, you can change it to never ever return null, but a NULL-Object instead.

This would allow you to lose all the ifs completely.

edited Jul 23 '13 at 13:10

TRiG

10,148
7
57
107

answered Jul 23 '13 at 11:42

Jens Schauder

77,657
34
181
348

This is how Cocoa works in Mac OS X / iOS. `[Null foo]` (equivalent to something along the lines of `((object)Null)->foo()` in other C-like languages) returns `Null`. – Nicholas Shanks Jul 23 '13 at 13:15
@Nicholas not quite. The objective-c message dispatcher is hard-coded to return 0x0 whenever 0x0 is passed in. It is not a special object whatsoever. Also, with floating point operations, it's possible that 0 won't be returned, but the remnants of the last floating point operation made. – Richard J. Ross III Jul 23 '13 at 14:46
@RichardJ.RossIII Sorry, I was trying to draw an analogy rather than make a statement of direct equivalence. – Nicholas Shanks Jul 23 '13 at 14:54

score 1 · Answer 9 · answered Jul 24 '13 at 10:04

An alternative is to use a "fake" single loop for controlling program flow. I can't say I'd recommend it, but it's definitely better looking and more readable than arrowhead.

Adding a "stage", "phase" or sth like that variable may simplify debugging and/or error handling.

int stage = 0;
do { // for break only, possibly with no indent

var foo = getfoo();
if(foo==null) break;

stage = 1;
var bar = getbar(foo);
if(bar==null) break;

stage = 2;
var moo = getmoo(bar);
if(moo==null) break;

stage = 3;
var cow = getcow(moo);

return 0; // end of non-erroreous program flow
}  while (0); // make sure to leave an appropriate comment about the "fake" while

// free resources if necessary
// leave an error message
ERR("error during stage %d", stage);

//return a proper error (based on stage?)
return ERROR;

So many brain calories spent trying to figure out why a do while(false) loop is used. Remember to write code for someone else to easily read it. — Kieveli, Jul 25 '13 at 13:15
@Kieveli well, the `return` just one line above seems like a pretty good hint. Anyway, this pattern is ugly by definition, but once you realize what it does, you'll recognize it anywhere. — Dariusz, Jul 25 '13 at 14:39

score 1 · Answer 10 · answered Jul 24 '13 at 12:21

try
{
  if (getcow(getmoo(getbar(getfoo()))) == null)
  {
    throw new NullPointerException();
  }
catch(NullPointerException ex)
{
  return; //or whatever you want to do when something is null
}

//... rest of the method

This keeps the main logic of the method uncluttered, and has just one exceptional return. Its disadvantages are that it can be slow if the get* methods are slow, and that it is difficult to tell in a debugger which method returned the null value.

score 1 · Answer 11 · answered Apr 30 '17 at 22:13

Rex Kerr's answer is indeed very nice.
If you can change the code though,Jens Schauder's answer is probably better (Null Object pattern)

If you can make the example more specific you can probably get even more answers
For example ,depending on the "location" of the methods you can have something like:

namespace ConsoleApplication8
{
    using MyLibrary;
    using static MyLibrary.MyHelpers;

    class Foo { }
    class Bar { }
    class Moo { }
    class Cow { }


    internal class Program
    {
        private static void Main(string[] args)
        {
            var cow = getfoo()?.getbar()?.getmoo()?.getcow();
        }
    }
}

namespace MyLibrary
{
    using ConsoleApplication8;
    static class MyExtensions
    {
        public static Cow getcow(this Moo moo) => null;
        public static Moo getmoo(this Bar bar) => null;
        public static Bar getbar(this Foo foo) => null;
    }

    static class MyHelpers
    {
        public static Foo getfoo() => null;
    }
}

score 0 · Answer 12 · answered Oct 26 '15 at 21:42

Strange, nobody mentioned method chaining.

If you create once a method chaining class

Public Class Chainer(Of R)

    Public ReadOnly Result As R

    Private Sub New(Result As R)
        Me.Result = Result
    End Sub

    Public Shared Function Create() As Chainer(Of R)
        Return New Chainer(Of R)(Nothing)
    End Function

    Public Function Chain(Of S)(Method As Func(Of S)) As Chainer(Of S)
        Return New Chainer(Of S)(Method())
    End Function

    Public Function Chain(Of S)(Method As Func(Of R, S)) As Chainer(Of S)
        Return New Chainer(Of S)(If(Result Is Nothing, Nothing, Method(Result)))
    End Function
End Class

You can use it everywhere to compose any number of functions into an execution sequence to produce a Result or a Nothing (Null)

Dim Cow = Chainer(Of Object).Create.
    Chain(Function() GetFoo()).
    Chain(Function(Foo) GetBar(Foo)).
    Chain(Function(Bar) GetMoo(Bar)).
    Chain(Function(Moo) GetCow(Moo)).
    Result

score -3 · Answer 13 · answered Jul 23 '13 at 14:40

-3

This is the one case where I'd use goto.

Your example might not be quite enough to push me over the edge, and multiple returns are better if your method is simple enough. But this pattern can get rather extensive, and you often need some cleanup code at the end. While using most of the other answers here if I can, often the only legible solution is to use goto's.

(When you do, be sure to put all references to the label inside one block so anyone looking at the code knows both the goto's and the the variables are confined to that part of the code.)

In Javascript and Java you can do this:

bigIf:  {

    if (!something)      break bigIf;
    if (!somethingelse)  break bigIf;
    if (!otherthing)     break bigIf;

    // Conditionally do something...
}
// Always do something else...
return;

Javascript and Java have no goto's, which leads me to believe other people have noticed that in this situation you do need them.

An exception would work for me too, except for the try/catch mess you force on the calling code. Also, C# puts in a stack trace on the throw, which will slow your code way down, particularly if it usually kicks out on the first check.

answered Jul 23 '13 at 14:40

RalphChapin

3,108
16
18

Why not just return? In fact, if goto works and return doesn't work then that's because you've written some really obfuscated, hard-to-follow code already. And if you didn't use goto you'd be forced to address this, but using goto, indeed, you can write much more bad code more easily than you would otherwise. So no, this isn't an exception to the rule against using goto, it's exactly why not using goto is orthodoxy by now. – djechlin Jul 23 '13 at 19:13
@djechlin: If return works, then use it. That's much better than a goto. But in the real world you often have some code that ought to run whether the if code runs or not. (Indeed, you might have several of these "if" sets, one after the other.) Consider that in a real world program there's going to be more than 3 ifs, each requiring more than 1 intermediate variable. Then try and find a solution that's not uglier than a goto. I'd love to see it. – RalphChapin Jul 23 '13 at 20:11
@RalphChapin the solution is to refactor so your code is simpler. Often this means identifying an inner function from which you can return, but the function should be clear and cohesive, which increases readability and understandably. If you *can't* find a solution other than a goto then your code is too complex and should be simplified (in C#, Javascript and Java, which have ample other ways to control flow). – djechlin Jul 23 '13 at 20:14
@djechlin: if you _can_ simplify it, great. I hate gotos. But I find myself with complex ifs like this on occasion and there's no other good way to deal with it in C#. (There is in JS and Java as my answer demonstrates.) – RalphChapin Jul 23 '13 at 20:22
You *always* can simplify it. My Java code base has no gotos nor named breaks, so I've somehow always been able to not have this problem. – djechlin Jul 23 '13 at 21:37
@djechlin: This is C#. _Java_ lets you label a block and then break out of it. That solves this problem neatly without a GOTO. You can't do that in C#. To emulate that you need a GOTO. C# _needs_ a GOTO for this precise case. Java _needs_ the labeled block for this precise case because it does not have a GOTO. – RalphChapin Jul 24 '13 at 00:29
1

@RalphChapin You're missing the point. I use neither the labeled block nor the goto in Java. When I would need such a kludge I make my code clearer and more straightforward so I don't. – djechlin Jul 24 '13 at 02:11

How to prevent the arrowhead anti-pattern

13 Answers13

This is one of few scenarios where it is perfectly acceptable (if not desirable) to use `goto`.

CERT actually recommends a structural approach based on `goto`.

Linked

How to prevent the arrowhead anti-pattern

13 Answers13

This is one of few scenarios where it is perfectly acceptable (if not desirable) to use goto.

CERT actually recommends a structural approach based on goto.

Linked

This is one of few scenarios where it is perfectly acceptable (if not desirable) to use `goto`.

CERT actually recommends a structural approach based on `goto`.