0
\$\begingroup\$

I want to get a second-opinion on my use of goto.

SqlCommand and SqlDataReader only read the RETURN-value of a Stored Procedure after the SqlDataReader has been closed. This is a problem if you want to interrogate the return-value to make a decision regarding how you want to read the data.

In my case, the stored procedure I'm calling returns multiple output data tables followed by an integer return value. The number of output data tables is variable.

// ret is a POCO domain entity object: a strongly-typed representation of the sproc's returned data
using( DbDataReader rdr = cmd.ExecuteReader() ) {

    if( !rdr.Read() )
        goto readReturnValue;

    ret.Foo  = ReadFoo( rdr );

    //////////

    if( !rdr.NextResult() )
        goto readReturnValue;

    List<Bar> bars = new List<Bar>();
    while( rdr.Read() ) bars.Add( ReadBar( rdr ) );
    ret.Bars = bars.ToArray();

    //////////

    if( !rdr.NextResult() )
        goto readReturnValue;

    ret.Qux = ReadQux( rdr );

    //////////

    if( !rdr.NextResult() )
        goto readReturnValue;

    List<Blargh> blarghs = new List<Blargh>();
    while( rdr.Read() ) blarghs.Add( ReadBlargh( rdr, ) );
    ret.Blarghs = blarghs.ToArray();

    // repeat a few more times...
}

readReturnValue:
Int32 returnValue = (Int32)returnParameter.Value;
switch( returnValue ) {
    case 0: // do stuff
    case 1: // do other stuff
    case ...
}

The first alternative that comes to mind is to remove the negation operator (!) from each if( !rdr.NextResult() ) and move each subsequent operation to a branch under the if, but this results in code many layers deep as there's about 8 or more result sets.

Another alternative is to replace goto readReturnValue with a throw and throw my own custom exception, e.g. throw new NoNextResultException() and in the catch block that surrounds the using I would swallow the exception and have the return-value interrogation code in the finally block, or in code immediately following the try block:

try {
    using( DbDataReader rdr = cmd.ExecuteReader() ) {

        if( !rdr.Read() )
            throw new NoNextResultException();

        ret.Foo  = ReadFoo( rdr );
        ...
    }
} catch(NoNextResultException) {
}
Int32 returnValue = (Int32)returnParameter.Value;
switch( returnValue ) {
...

But the problem with that is that throw statements don't define where the exception lands: so removing the catch statement means the program will compile, but crash due to an unhandled exception when it runs, whereas removing the goto label readReturnValue means the program won't compile, which is safer. Furthermore it avoids the expensive cost of throwing and catching exceptions as well as sticking to the mantra of "use exceptions for exceptional circumstances, not for program flow" - which is what it would be if I did convert it to a try/throw/catch.

Is my use of goto then justified?

\$\endgroup\$
3
\$\begingroup\$

Is my use of goto then justified?

First of all, let me push back on the question. A goto does not require a justification any more than any other language feature requires a justification.

That said, a goto is widely considered to be a sign that the code is inelegant, confusing or hard to maintain. It is definitely a sign that you should be thinking hard about how the control flow of the code works, and whether there is a refactoring that would make it more clear.

You are doing that, and that's excellent. The problem is that you've only identified two alternatives to a goto: refactoring into nested "if" statements, which as you note may be harder to read than the goto version, and using "throw" to simulate a goto. That's a bad practice; throw should be used to indicate an unexpected error condition, not as a very expensive way to simulate a goto.

You missed the preferred way to remove a goto: turn it into a return.

void DoTheFirstThing()
{
  using( DbDataReader rdr = cmd.ExecuteReader() ) 
  {
    if( !rdr.Read() ) return;
    ret.Foo  = ReadFoo( rdr );
    if( !rdr.NextResult() ) return;
    List<Bar> bars = new List<Bar>();
    while( rdr.Read() ) bars.Add( ReadBar( rdr ) );
    ret.Bars = bars.ToArray();
    if( !rdr.NextResult() ) return;
    ret.Qux = ReadQux( rdr );
    if( !rdr.NextResult() ) return;
    List<Blargh> blarghs = new List<Blargh>();
    while( rdr.Read() ) blarghs.Add( ReadBlargh( rdr, ) );
    ret.Blarghs = blarghs.ToArray();
    ...
   }
}

void DoTheSecondThing()
{
  Int32 returnValue = (Int32)returnParameter.Value;
  switch( returnValue ) {
    case 0: // do stuff
    case 1: // do other stuff
    case ...
  }
  ...
}

void DoTheThings()
{
  DoTheFirstThing();
  DoTheSecondThing();
}

There, no gotos required, and your program is now refactored into smaller methods that each do one task well, rather than one god method that tries to create the whole universe in one go.

While we are criticizing your code, I note that this also looks a bit dodgy:

    if( !rdr.NextResult() ) return;
    List<Blargh> blarghs = new List<Blargh>();
    while( rdr.Read() ) blarghs.Add( ReadBlargh( rdr, ) );
    ret.Blarghs = blarghs.ToArray();

Suppose NextResult is false, and so we take the return. What now is the value of the blarghs field of object ret? Is it null? Is it intended to be null? Is the user of this field supposed to understand that it can and will be null if the reader ran out of results, and that if it is not null, then it is a non-empty array? This is a lousy contract. It is much better to say "blarghs will be a non-null array that might be empty", and not "blarghs is either null or a non-empty array". I would be inclined to re-think your deserialization strategy here so that more of the deserialized object is in a good state:

IEnumerable<Bar> ReadBars(Reader rdr)
{
    if( !rdr.NextResult() ) yield break;
    while( rdr.Read() ) 
      yield return ReadBar( rdr );
}

IEnumerable<Blargh> ReadBlarghs(Reader rdr)
{
    if( !rdr.NextResult() ) yield break;
    while( rdr.Read() ) 
      yield return ReadBlargh( rdr );
}


void DoTheFirstThing()
{
  using( DbDataReader rdr = cmd.ExecuteReader() ) 
  {
    // Move the check for the goodness of the reader into ReadFoo,
    // readQux, etc.

    ret.Foo  = ReadFoo( rdr );
    ret.Bars = ReadBars( rdr ).ToArray();        
    ret.Qux = ReadQux( rdr );
    ret.Blarghs = ReadBlarghs ( rdr ).ToArray();
   }
}

OMG look at how much better that code reads now.

BUT WAIT THERE'S MORE. Let's make this an extension method and parameterize it with a function:

static class Extensions
{

  public static IEnumerable<T> ReadThings<T>(this Reader rdr, Func<Reader, T> f)
  {
    if( !rdr.NextResult() ) yield break;
    while( rdr.Read() ) 
      yield return f( rdr );
  }
  public static Foo ReadFoo(this Reader rdr)
  {
     if (!rdr.Read) return DefaultFoo;
     ...
  }
  // Similarly for ReadQux ...
}
...
void DoTheFirstThing()
{
  using( DbDataReader rdr = cmd.ExecuteReader() ) 
  {
    ret.Foo  = rdr.ReadFoo();
    ret.Bars = rdr.ReadThings(ReadBar).ToArray();        
    ret.Qux = rdr.ReadQux();
    ret.Blarghs = rdr.ReadThings(ReadBlargh).ToArray();
  }
}

Isn't that about a million times nicer to read than what you wrote? Look at how in this code the meaning shines through. In your code it looks like the most important thing is the reader and managing whether it is done or not. In this code it looks like the most important thing is that we are reading a Foo, some Bars, a Qux and some Blarghs. Which way do you think will be easier to understand and maintain?

We've used higher level functions and extension methods for the win. You could be making this code tiny and super easy to read, compared to its present enormous sprawling repetitive goto-laden spaghetti.

Think about where this refactoring gets you. Now if you want to someday in the future say hey, you know, I actually only want to deserialize the frobby bars:

    ret.Bars = rdr.ReadThings(ReadBar).Where(bar => IsFrobby(bar)).ToArray();        

DONE.

Or, you know what, we serialized out a bunch of bars, but what we need in memory is actually a bunch of fubars:

    ret.Fubars = rdr.ReadThings(ReadBar).Select(bar => MakeFubar(bar)).ToArray();        

Nice. You want to make a change like that, you make it in one place with clear, self-documenting code.

And now you see the true power of thinking "is this goto a bad idea?" By thinking it through carefully you can take the intuition that the goto is inelegant and manage to refactor your program into something that is small, powerful, and cheap to change when requirements change. So by all means, keep on thinking about whether those gotos are inelegant -- just think the problem all the way through, rather than stopping on some not-actually-better solution like using a throw or a break or a continue to simulate a goto.

\$\endgroup\$
  • \$\begingroup\$ I wouldn't have thought of refactoring it that way, thanks! \$\endgroup\$ – Dai Feb 21 '16 at 22:11
  • \$\begingroup\$ There's a minor bug here: I can use ReadThings for every result-set, even single-row result sets (by calling .SingleOrDefault() instead of ToArray() however this means NextResult() is called 1 time too many. Is there a good way to avoid that? \$\endgroup\$ – Dai Feb 25 '16 at 3:54
0
\$\begingroup\$

The code implementation looks good. The second alternative (throw) is not recommended.

Wondering, if a while loop and an inside switch for each result set would make an option for you as well.

\$\endgroup\$
0
\$\begingroup\$

Code readability is always one of the most important considerations. If using goto makes your code more readable in your opinion, then the use is justified.

If despite that, you still feel dirty for using goto, here are some alternative ways to get around it:

1) Wrap your code in a do { } while(false); loop.

do
{
    // Reading stuff from database
    // ....

    if( !rdr.NextResult() )
        break;

    //read more from database
    // ....

    if(...)
        break;
} while(false);

Semantically, it's not much different than just using a goto while explicitly avoiding it's use. (I've used this trick before and it made me feel better, while at the same time minimally impacting code readability).

2) Put your switch statement inside a function.

// reading stuff from database
// ....

if( !rdr.NextResult() )
    return processResults(rdr, bars, blargs);

and then something like:

bool processResults(DbDataReader rdr, params object[] thingsFromDb)
{
    Int32 numThingsFromDb = (Int32)returnParameter.Value;
    for(int ix=0; ix<numThingsFromDb); ix++)
        //addtional processing

   return true; //or whatever
}

I'm obviously guessing at your post reading needs but I think you get the idea. Whenever you reach the end of your data, call the post processing function, do whatever you need to do and exit up the call stack.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.