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 tha开发者_高级运维t 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.

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;
    });

Change

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

to this:

_Cancelled = _CancelArgs.Cancel;

and END: to this:

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

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);

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();
        }
    }
}

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

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.

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; }
}

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

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();
        }
    }
}

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();
        }
    }
}

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. ;)