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;
});
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?)
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
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"?)
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.
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.