Reputation: 31813

C#/Is GOTO right here?

Oh boy, the passion around GOTO statements in C#; I dread even asking this question.

So many questions similar to this; that also makes me a bit nervous. But I am serious.

Please resist the responses that simply dismiss the GOTO statement wholesale.

However, I am a little stumped to see why this implementation is not ideal for GOTO:

public event CancelEventHandler DeleteSnapshotStarted;
public event AsyncCompletedEventHandler DeleteSnapshotCompleted;
public void DeleteSnapshot(Guid documentId, Action<Exception> callback)
{
    if (!this.Snapshots.Where(x => x.DocumentId == documentId).Any())
        throw new Exception("Snapshot not found; ensure LoadSnapshots()");

    // define action
    var _Action = new Action(() =>
    {
        // preview
        bool _Cancelled = false;
        if (DeleteSnapshotStarted != null)
        {
            CancelEventArgs _CancelArgs = new CancelEventArgs { };
            DeleteSnapshotStarted(this, _CancelArgs);
            if (_CancelArgs.Cancel)
            {
                _Cancelled = true;
                goto END;
            }
        }

        // execute
        Exception _Error = null;
        try
        {
            Proxy.CoreService.DeleteSnapshot(documentId);
            LoadSnapshots(null);
        }
        catch (Exception ex) { _Error = ex; }

    END:

        // complete
        if (DeleteSnapshotCompleted != null)
        {
            AsyncCompletedEventArgs _CompleteArgs = 
                new AsyncCompletedEventArgs(_Error, _Cancelled, null);
            DeleteSnapshotCompleted(this, _CompleteArgs);
        }

        // bubble error
        if (_Error != null)
            throw _Error;
    });

    // run it
    if (callback == null) { _Action(); }
    else
    {
        using (BackgroundWorker _Worker = new BackgroundWorker())
        {
            _Worker.DoWork += (s, arg) => { _Action(); };
            _Worker.RunWorkerCompleted += (s, arg) => { callback(arg.Error); };
            _Worker.RunWorkerAsync();
        }
    }
}

** I give - I'll avoid GOTO! :D**

Here's what seems best:

public event CancelEventHandler DeleteSnapshotStarted;
public event AsyncCompletedEventHandler DeleteSnapshotCompleted;
public void DeleteSnapshot(Guid documentId, Action<Exception> callback)
{
    if (!this.Snapshots.Where(x => x.DocumentId == documentId).Any())
        throw new Exception("Snapshot not found; ensure LoadSnapshots()");

    // define action
    var _Action = new Action(() =>
    {
        // preview
        CancelEventArgs _CancelArgs = new CancelEventArgs { };
        if (DeleteSnapshotStarted != null)
            DeleteSnapshotStarted(this, _CancelArgs);

        // execute
        Exception _Error = null;
        if (!_CancelArgs.Cancel) try
            {
                Proxy.CoreService.DeleteSnapshot(documentId);
                LoadSnapshots(null);
            }
            catch (Exception ex) { _Error = ex; }

        // complete
        if (DeleteSnapshotCompleted != null)
            DeleteSnapshotCompleted(this, 
              new AsyncCompletedEventArgs(null, _CancelArgs.Cancel, null));

        // bubble
        if (_Error != null)
            throw _Error;
    });

    // run it
    if (callback != null)
    {
        using (BackgroundWorker _Worker = new BackgroundWorker())
        {
            _Worker.DoWork += (s, arg) => { _Action(); };
            _Worker.RunWorkerCompleted += (s, arg) => 
                            { callback(arg.Error); };
            _Worker.RunWorkerAsync();
        }
    }
    else
        _Action();
}

Thanks everyone.

Upvotes: 0

Answers (11)

LBushkin

Reputation: 131676

Neal Stephenson thinks it's cute to name his labels 'Dengo'

In principle, it's worth avoiding non-local branching in code, for readability. In your case, it's possible to restructure the flow of control with a flag variable. See @NeilN and @minitech answers for the details.

In practice, it is sometimes (in rare cases :) useful to use goto to resolve complex flow of control scenarios where normal if/else/break/while/for structures would be more nested or convoluted than necessary.

The best "good" use of a goto (that I can think of right now) is to break out of a deeply nested set of loops without the overhead of additional conditional checks on each loop iteration. From a single level of nesting you could use break - but with many nested levels it becomes more painful. Here's an example:

// Column-ordered, first value search:
int valueFound = 0;
for (int i = 0; i < x; i++)
{
    for (int j = 0; j < y; j++)
    {
        if (array[j, i] < targetValue)
        {
            valueFound = array[j, i];
            goto Found;
        }
    }
}

Console.WriteLine("No value was found.");
return;

Found:
    Console.WriteLine("The number found was {0}.", valueFound);

Upvotes: 3

Slamlander

Reputation: 1

Because a switch ... break could have done the job cleaner.

You first declare CancleArgs as an enum { ... Canceled, ..., END}

switch (CancleArgs)
 {
    case Canceled: _Cancelled = true; break;
 ... other stuff
    case END:
      { 
      // complete          
      if (DeleteSnapshotCompleted != null)          
       {              
         AsyncCompletedEventArgs _CompleteArgs = new AsyncCompletedEventArgs(_Error, _Cancelled, null);              
         DeleteSnapshotCompleted(this, _CompleteArgs);
       }            
      // bubble error          
      if (_Error != null) throw _Error;  
      }
      break;
 }
// run it
   ...

No refactoring because it should have been written this way to begin with. ;)

Upvotes: -1

Patrik Svensson

Reputation: 13844

Try wrapping the deletion of the snapshot with this and remove the GOTO and the END label

if(!_Cancelled)
{
    Exception _Error = null;
        try
        {
            Proxy.CoreService.DeleteSnapshot(documentId);
            LoadSnapshots(null);
        }
        catch (Exception ex) { _Error = ex; }
}

Upvotes: 1

maxbeaudoin

Reputation: 6976

public event CancelEventHandler DeleteSnapshotStarted;
public event AsyncCompletedEventHandler DeleteSnapshotCompleted;
public void DeleteSnapshot(Guid documentId, Action<Exception> callback)
{
    if (!this.Snapshots.Where(x => x.DocumentId == documentId).Any())
        throw new Exception("Snapshot not found; ensure LoadSnapshots()");

    // define action
    var _Action = new Action(() =>
    {
        // preview
        bool _Cancelled = false;
        if (DeleteSnapshotStarted != null)
        {
            CancelEventArgs _CancelArgs = new CancelEventArgs { };
            DeleteSnapshotStarted(this, _CancelArgs);
            if (_CancelArgs.Cancel)
            {
                _Cancelled = true;
            }
        }

        if(!_Cancelled)
        {
            // execute
            Exception _Error = null;
            try
            {
                Proxy.CoreService.DeleteSnapshot(documentId);
                LoadSnapshots(null);
            }
            catch (Exception ex) { _Error = ex; }
        }

        // END:

        // complete
        if (DeleteSnapshotCompleted != null)
        {
            AsyncCompletedEventArgs _CompleteArgs = 
                new AsyncCompletedEventArgs(_Error, _Cancelled, null);
            DeleteSnapshotCompleted(this, _CompleteArgs);
        }

        // bubble error
        if (_Error != null)
            throw _Error;
    });

    // run it
    if (callback == null) { _Action(); }
    else
    {
        using (BackgroundWorker _Worker = new BackgroundWorker())
        {
            _Worker.DoWork += (s, arg) => { _Action(); };
            _Worker.RunWorkerCompleted += (s, arg) => { callback(arg.Error); };
            _Worker.RunWorkerAsync();
        }
    }
}

Upvotes: 0

quentin-starin

Reputation: 26638

There's no need for goto in your code. It only makes it more complicated. Here's an equivalent version without it.

public event CancelEventHandler DeleteSnapshotStarted;
public event AsyncCompletedEventHandler DeleteSnapshotCompleted;
public void DeleteSnapshot(Guid documentId, Action<Exception> callback)
{
    if (!this.Snapshots.Where(x => x.DocumentId == documentId).Any())
        throw new Exception("Snapshot not found; ensure LoadSnapshots()");

    // define action
    var _Action = new Action(() =>
    {
        // preview
        bool _Cancelled = false;
        if (DeleteSnapshotStarted != null)
        {
            CancelEventArgs _CancelArgs = new CancelEventArgs { };
            DeleteSnapshotStarted(this, _CancelArgs);
            if (_CancelArgs.Cancel)
            {
                _Cancelled = true;
            }
        }

        if (!_Cancelled) {
          // execute
          Exception _Error = null;
          try
          {
              Proxy.CoreService.DeleteSnapshot(documentId);
              LoadSnapshots(null);
          }
          catch (Exception ex) { _Error = ex; }
        }  

        // complete
        if (DeleteSnapshotCompleted != null)
        {
            AsyncCompletedEventArgs _CompleteArgs = 
                new AsyncCompletedEventArgs(_Error, _Cancelled, null);
            DeleteSnapshotCompleted(this, _CompleteArgs);
        }

        // bubble error
        if (_Error != null)
            throw _Error;
    });

    // run it
    if (callback == null) { _Action(); }
    else
    {
        using (BackgroundWorker _Worker = new BackgroundWorker())
        {
            _Worker.DoWork += (s, arg) => { _Action(); };
            _Worker.RunWorkerCompleted += (s, arg) => { callback(arg.Error); };
            _Worker.RunWorkerAsync();
        }
    }
}

Upvotes: 0

Neil N

Reputation: 25258

Change

        if (_CancelArgs.Cancel)
        {
            _Cancelled = true;
            goto END;
        }

to this:

_Cancelled = _CancelArgs.Cancel;

and END: to this:

if(!Cancelled)
{
   // complete...

Upvotes: 4

Hans Keﬆing

Reputation: 39284

Why not just an if (!_Cancelled) around those lines between the GOTO and the label?

Upvotes: 2

MGZero

Reputation: 5963

Just use your canceled variable in an if statement to see if you should skip the rest of the code.

Upvotes: 0

Ry-

Reputation: 224904

Yes, you even already have the flag variable:

    if (!this.Snapshots.Where(x => x.DocumentId == documentId).Any())
        throw new Exception("Snapshot not found; ensure LoadSnapshots()");

    // define action
    var _Action = new Action(() =>
    {
        // preview
        bool _Cancelled = false;
        if (DeleteSnapshotStarted != null)
        {
            CancelEventArgs _CancelArgs = new CancelEventArgs { };
            DeleteSnapshotStarted(this, _CancelArgs);
            if (_CancelArgs.Cancel)
            {
                _Cancelled = true;
                goto END;
            }
        }

        if(!_Cancelled) {
            // execute
            Exception _Error = null;
            try
            {
                Proxy.CoreService.DeleteSnapshot(documentId);
                LoadSnapshots(null);
            }
            catch (Exception ex) { _Error = ex; }
        }
    END:

        // complete
        if (DeleteSnapshotCompleted != null)
        {
            AsyncCompletedEventArgs _CompleteArgs = 
                new AsyncCompletedEventArgs(_Error, _Cancelled, null);
            DeleteSnapshotCompleted(this, _CompleteArgs);
        }

        // bubble error
        if (_Error != null)
            throw _Error;
    });

Upvotes: 8

James Michael Hare

Reputation: 38397

In general: It would be much clearer and more maintainable to instead refactor the code so that the goto is not necessary. It's a big method as it is and should be broken down a bit.

Occasionally goto is a good choice, but a lot of the time it tends to be used when a simple refactoring would suffice.

In Your Case: In your case it looks like from a lot of the other answers suggesting using the cancelled flags would solve your problem without the goto.

Upvotes: 1

Corey Ogburn

Reputation: 24717

From the looks of it, you can wrap the try/catch with if (!_Cancelled) { ... }. Currently the way you have it (from the code you've made available), you're not using _Cancelled anywhere. The new code would look like:

public event CancelEventHandler DeleteSnapshotStarted;
public event AsyncCompletedEventHandler DeleteSnapshotCompleted;
public void DeleteSnapshot(Guid documentId, Action<Exception> callback)
{
    if (!this.Snapshots.Where(x => x.DocumentId == documentId).Any())
        throw new Exception("Snapshot not found; ensure LoadSnapshots()");

    // define action
    var _Action = new Action(() =>
    {
        // preview
        bool _Cancelled = false;
        if (DeleteSnapshotStarted != null)
        {
            CancelEventArgs _CancelArgs = new CancelEventArgs { };
            DeleteSnapshotStarted(this, _CancelArgs);
            if (_CancelArgs.Cancel)
            {
                _Cancelled = true;
            }
        }

        if (!_Cancelled) {
            // execute
            Exception _Error = null;
            try
            {
                Proxy.CoreService.DeleteSnapshot(documentId);
                LoadSnapshots(null);
            }
            catch (Exception ex) { _Error = ex; }
        }

        // complete
        if (DeleteSnapshotCompleted != null)
        {
            AsyncCompletedEventArgs _CompleteArgs = 
                new AsyncCompletedEventArgs(_Error, _Cancelled, null);
            DeleteSnapshotCompleted(this, _CompleteArgs);
        }

        // bubble error
        if (_Error != null)
            throw _Error;
    });

    // run it
    if (callback == null) { _Action(); }
    else
    {
        using (BackgroundWorker _Worker = new BackgroundWorker())
        {
            _Worker.DoWork += (s, arg) => { _Action(); };
            _Worker.RunWorkerCompleted += (s, arg) => { callback(arg.Error); };
            _Worker.RunWorkerAsync();
        }
    }
}

Upvotes: 2

C#/Is GOTO right here?

Answers (11)

Related Questions