Best practice for using goto

Question

Is it right to use goto in this code? Are there any alternatives?

return ExecuteReader(cmd, reader =>
{
    List<BEPartnership> partnerhip = null;

    //Partnership
    if (!((SqlDataReader) reader).HasRows)
        goto exit;

    partnerhip = 
        new List<BEPartnership>{new BEPartnership().GetFromReader(reader)};

    //Customers
    if (!reader.NextResult() && !((SqlDataReader) reader).HasRows)
        goto exit;

    foreach (BEPartnership p in partnerhip)
        p.Partner = new BECustomer().GetFromReader(reader);

    //Contracts
    if (!reader.NextResult() && !((SqlDataReader) reader).HasRows)
        goto exit;

    List<BEContractB2B> contracts = new List<BEContractB2B>();
    contracts.Add(new BEContractB2B().GetFromReader(reader));
    // contracts = new BEContractB2B().GetFromReader2(reader).ToList();

    exit:
    return partnerhip;
});

Answer 1

goto and "best practice" are mutually exclusive, in my opinion (and probably/hopefully in that of most others, too). The need for a goto indicates a faulty code design. In your case, the solution seems to be simple: I think you just have to replace goto exit by return partnerhip and to delete the label exit:. (Should it read "partnership" instead of "partnerhip"?)

Answer 2

You can replace each goto exit; with return null; or return partnerhip; if you wish to return the currently populated list. (I assume a partnerhip is a cool partner?)

Answer 3

I found following case when goto could be a bit useful: When To Use Goto When Programming in C. But "Never use GOTO" was first thing I learned on University and so I really never used it (at least not in C,C++,C#,Java,...).

The biggest problem of GOTO is that if you read piece of method, you don't see from where it could be called. For example:

int a = 1;
division:
int b = 4 / a;

... sounds OK. But you wrote 0-dividing crash, if there is following GOTO after the division block:

int a = 1;
division:
int b = 4 / a;
// ... hundreds of lines ...
a = 0;
goto division;

... Or null-exception crash if there is GOTO before the division block:

goto division;
// ... hundreds of lines ...
int a = 1;
division:
int b = 4 / a;

... this is only one example, GOTO leads to much more disputable situations. So please forget about GOTO and people (including you) will be happier during reading your code.

Use "return partnership;" instead of your goto's.

Answer 4

I would say no.

I've been programming in C# since 2001 and have never used goto!

If you want a "short circuit" exit in your code, why not replace the

goto exit:

with

return partnership

Answer 5

What you are doing at the end is loading contracts from the reader if it is there. It reads much better if you make this intention explicit with a simple if statement.

Change the end to be :

if (reader.NextResult() || ((SqlDataReader) reader).HasRows)
{
    List<BEContractB2B> contracts = new List<BEContractB2B>();
    contracts.Add(new BEContractB2B().GetFromReader(reader));
}

return partnerhip;

Although it does appear you are just ignoring the contracts list... it isn't doing anything. Unless creating a new BEContractB2B class has some global side effects (bad news), you could just get rid of it alltogether..

Change the first goto to be

if (!((SqlDataReader) reader).HasRows)
    return null;

Since that is what you are doing, you should make it obvious you will return null.