Using a Goto to enhance the DRY principle and code clarity: a good idea?

Question

I have some code that is structured like the following:

if (someStatement)
{
    //...
    if (SomeOtherStatement)
    {
        //..., possibly more cases like this
    }
    else
    {
        //goto warning;
        //would otherwise repeat
        //the MessageBox.Show here
    }
}
else
{
    //goto warning;
}
//...
warning:
MessageBox.Show("some warning");

As i abhor copying code, is this one of the few useful applications of goto or is there a better structure i can use?

Answer 1

I personally consider centralized error handling as "almost the only case" where goto is acceptable. However, for a better structure, I would put the label fully outside of the else, at the very end of your function.

Answer 2

What about this?

if (someStatement)
{
    //...
    if (SomeOtherStatement)
    {
        //..., possibly more cases like this

        return; // in the inner-most case
    }
}

MessageBox.Show("some warning");

Answer 3

Are you displaying the same error message on both lines? Then you should write a function that calls just that line and call that function on both occasions.

if (someStatement)
{
    //...
    if (SomeOtherStatement)
    {
        //..., possibly more cases like this
    }
    else
    {
        showError();
    }
}
else
{
    showError();
}

BTW: this is a bad example, it can be solved with a single else-branch catching the error. Here is a better one:

if (!doSomething())
{
    showError();
}
doSomethingElse();
if (!anotherCall())
{
    showError();
}

Answer 4

Yes, but only if by goto you mean "create a function and call it multiple times".

Answer 5

the best approach depends on what the rest of your code looks like. if the code shown is the last code in that method:

if(someStatement) {
    // ...
    if(SomeOtherStatement)
    {
        // a couple of lines of code
        return; // !
    }
}

MessageBox.Show("someWarning");

if not, you probably have to retort to something like this:

if(someStatement) {
    // ...
    if(SomeOtherStatement)
    {
        // a couple of lines of code
    }
    else 
    {
        showWarning("not someOtherStatement");
    }
}
else
{
    showWarning("not someStatement");
}

Answer 6

Exceptions are the proper way to handle errors in .NET, throw them, but use them wisely:

try
{
    if (someStatement)
    {
        //...
        if (SomeOtherStatement)
        {
            //..., possibly more cases like this
        }
        else
        {
            throw new MyException();
            //would otherwise repeat
            //the MessageBox.Show here
        }
    }
}
catch(MyException e)
{
    MessageBox.Show(e.Message);
    // log the exception, if necessary
}
finally
{
    // tidy up
}

With the exception you get a strongly typed answer to what went wrong which you can easily implement into logging systems.

Answer 7

If you are going to get into a deep nasty error testing statement like you have, then you might want to consider wrapping it with a single try/catch, and throw the appropriate application specific exceptions from within the if statement, and catch those specific exceptions in the catch block, letting all the others go to be caught further up the chain.

Answer 8

Since (starting with FrameWork 1.1) logical-and "&&" : "only evaluates its second operand if necessary."

What's wrong with :

if(someStatement && someOtherStatement)
{
       // more whatever
}
else
{
       // raise an exception, call a method, show messagebox, whatever
}

Depending on the complexity of logical evaluation going on if the first clause evaluates to true : and you, possibly, implement lots of other tests, any number of which could result in errors : all of which you want handled in the identical way : I'd consider it mandatory to either raise a specific exception, call a method to handle the error, or put the whole thing in a try-catch block (which can be quite ugly if there's a lot of code).

A low-down-dirty-sneaky method I would use if it was a reasonable presumption that the possibility of error was quite remote 99% of the time the code was called : I'd set a boolean flag to reflect an error state : evaluate it on completion of the complex code block, and do what needed to be done : of course if your code branches off somewhere as a result of these internal evaluations : that would not be a good thing.

Answer 9

I think goto is a bad idea. If you have to jump, throw an exception, as proposed by joshcomley; if you just want to output the warning message, but continue without a jump, call a method.