Avoiding the use of Goto

Question

All -- I am refactoring some older code and I am looking at ways to reduce (or if not eliminate it altogether) the use of the GoTo statement. I have a section of code as follows:

public void GetData()
{
  TryAgain:
      Foo foo = bar.GetData();

      if(foo == null)
      {
          bar.addItem("Test");
          goto TryAgain;
      }

      //Use the bar object
}

Replacing it with the following:

public void GetData()
{
      Foo foo = bar.GetData();

      if(foo == null)
      {
          bar.addItem("Test");
          GetData();
          return;
      }

      //Use the bar object

}

Any thoughts or a better way to handle this?

UPDATE

First of all this isn't my actual code, I created this snippet for brevity purposes. Next please assume that once a value has been added to bar then the IF statement will be bypassed and the code section will continue and use the bar object. I want to create only one method that first checks to make sure that the bar object is not null and if it isn't then proceed runing the remainder of the code in the method. Sorry for the confusion.

I don't understand your comment, why would my first example be a loop? — Mark Kram, Jun 03 '13 at 17:26
Though it's written with a `goto` instead of a normal loop function, the first is code that loops back on itself, thus it is a loop. — Tim S., Jun 03 '13 at 17:29
Because you go back to the beginning of the code snippet conditionally; you continue doing that over and over again until a condition isn't met, and then you stop going back to the start and just end. That's pretty much the *definition* of a loop. — Servy, Jun 03 '13 at 17:29
Your refactoring puts a limitation on the number of checks you can do (until a StackOverflowException) while the first will check indefinitely. — Austin Salonen, Jun 03 '13 at 17:31
@Servy the second example isn't quite the same as removing the `goto` and doing nothing else, because of this: `bar.addItem("Test")` changes an object. If this method didn't change anything, but simply returned a value or did some validation and optionally throw an exception or something, then that would be equivalent to doing nothing else. — Tim S., Jun 03 '13 at 17:31
@MarkKram, not sure if I got it right, but please find the answer's update. — Andrei, Jun 03 '13 at 17:38
is `foo` used after the test succeeds? If not then why not simply `if (bar.GetData() == null) { bar.AddItem(); bar.GetData(); }` -- why have a local `foo` and a `goto` at all? — Eric Lippert, Jun 03 '13 at 18:38

Andrei · Answer 1 · 2013-06-03T17:37:46.187

Use while loop

public void GetData()
{
    Foo foo = bar.GetData();

    while (foo == null)
    {
        bar.addItem("Test");
        foo = bar.GetData();
    }
}

Update. If I understood your real purpose right:

public void GetData()
{
    Foo foo = bar.GetData();    
    if (foo == null)
    {
        bar.addItem("Test");
        // following the assumption
        // "once a value has been added to bar then the IF statement will be bypassed"
        // there is no need for another GetData call - bar object is in valid state now
    }

    //Use the bar object
}

score 3 · Answer 2 · answered Jun 03 '13 at 17:21

3

public void GetData()
{
    Foo foo;    
    while ((foo = bar.GetData()) == null)
        bar.addItem("Test");
}

answered Jun 03 '13 at 17:21

Frank Hileman

1,159
9
19

3

I would strongly advise you to avoid using assignment in conditional expressions; tends to result in code that's more confusing to read than equivalent refactors that perform the assignment outside of the conditional. – Servy Jun 03 '13 at 17:22
While I share your concerns, in C# this is the only way to boil the code down to a single GetData call. Those with a background in unix C programming will recognize this pattern, as it is commonly used to iterate streams, and the pattern is sometimes recommended in C#, for example with [StreamReader.ReadLine](http://www.dotnetperls.com/streamreader). – Frank Hileman Jun 04 '13 at 23:13

score 1 · Answer 3 · answered Jun 03 '13 at 17:17

1

public void GetData()
{
  while(true)
 {
      Foo foo = bar.GetData();

      if(foo == null)
      {
          bar.addItem("Test");
      }
      else break;
 }
}

answered Jun 03 '13 at 17:17

Tim S. · Answer 4 · 2013-06-03T17:34:32.870

1

Update based on your update, I might go with this code:

public void GetData()
{
    Foo foo = bar.GetData();

    if (foo == null)
    {
        bar.addItem("Test");
        foo = bar.GetData();

        Debug.Assert(foo != null, "Hey, I thought that would give me a value!");
    }

    // do something with foo
}

edited Jun 03 '13 at 17:34

answered Jun 03 '13 at 17:22

Tim S.

55,448
7
96
122

score 1 · Answer 5 · answered Jun 03 '13 at 19:24

1

You aren't using the foo object, so you can get rid of it.

public void GetData()
{
    while (bar.GetData() == null)
        bar.addItem("Test");

    //Use the bar object
}

answered Jun 03 '13 at 19:24

djs

220
1
5

score -3 · Answer 6 · answered Jun 03 '13 at 17:38

-3

You're basically describing the await/async pattern created in .Net 4.5. If you control enough of the code to implement this, the code in question becomes this:

// bar.addItem would need to be done in GetDataAsync if it is important
Foo foo = await bar.GetDataAsync();

The best answer otherwise is the while-loop approach described by @Andrei.

MSDN Asynchronous programming

answered Jun 03 '13 at 17:38

Austin Salonen

49,173
15
109
139

@ZachLeighton: The problem effectively describes a "poll until" problem, which is exactly how an await/async solution would be implemented. We have no idea what `bar.GetData` actually does nor does it matter but we don't care until it's not null; ie, we _wait_ until the method call "succeeds." – Austin Salonen Jun 03 '13 at 19:52

Avoiding the use of Goto

UPDATE

6 Answers6