Best practice for using goto

Question

Is it possible 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

You can replace every goto exit; on return null; or return partnerhip; if you want to return the current list. (I assume partnership is a cool partner?)

Answer 2

I would say no.

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

If you want a “short circuit” in your code, why not replace

 goto exit: 

from

 return partnership 

Answer 3

goto and "best practice" are mutually exclusive, in my opinion (and probably / hope most of the others too). The need for goto indicates an erroneous code construction. In your case, the solution seems simple: I think you just need to replace goto exit with return partnerhip and remove the exit: label. (Should you read "partnership" instead of "partnership"?)

Answer 4

What you do at the end is downloading contracts from the reader, if any. It reads much better if you make this intent explicit with a simple if statement.

Change the end:

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

Although it looks, you are simply ignoring the list of contracts ... it does nothing. If only creating a new BEContractB2B class does not have any global side effects (bad news), you can just get rid of it all together.

Change the first goto as

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

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

Answer 5

A slightly different design can eliminate most of them quite easily:

Why are you testing HasRows at all? Consider the benefits of returning an empty list, rather than null when there are no entries. You will probably clear your code too.

Answer 6

I found the following example when goto might be a little useful: When to use Goto when programming in C. But “ Never Use GOTO ” was the first thing I learned at the university, and therefore I never used it (at least not in C, C ++, C #, Java, ...).

GOTO's biggest problem is that if you read a piece of the method, you don’t see where it could be called. For instance:

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

... sounds normal. But you wrote a 0-dividing failure if there is the following GOTO after the division block:

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

... Or null-exception fails if there is GOTO in front of the separation block:

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

... this is just one example, GOTO leads to much more controversial situations. So, please forget about GOTO, and people (including you) will be happier to read your code.

Use a “return partnership”; instead of your goto's.