Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

Is there a better way than GOTO in this scenario?

Tags:

c#

Let's start this with a little honesty. I am not a fan of goto, but neither am I a fan of repeating my code over and over. Anyway, I hit this scenario. It's unusual, but not out of this world. I can't help but wonder if this is a good scenario for goto. Is there a better way?

public Task Process(CancellationTokenSource token)
{
    await SpinUpServiceAsync();
    foreach (var item in LongList())
    {
        if (token.IsCancellationRequested) goto cancel;
        await LongTask1(item);

        if (token.IsCancellationRequested) goto cancel;
        await LongTask2(item);

        if (token.IsCancellationRequested) goto cancel;
        await LongTask3(item);

        if (token.IsCancellationRequested) goto cancel;
        await LongTask4(item);

        if (token.IsCancellationRequested) goto cancel;
        await LongTask5(item);

        continue;
        cancel:
        {
            Log($"Cancelled during {item}");
            await SpinDownServiceAsync();
            return;
        }
    }
}

Here's the next best alternative I see:

public Task Process(CancellationTokenSource token)
{
    await SpinUpServiceAsync();
    foreach (var item in LongList())
    {
        if (token.IsCancellationRequested)
        {
            Log($"Cancelled during {item}");
            await SpinDownServiceAsync();
            return;
        }
        await LongTask1(item);

        if (token.IsCancellationRequested)
        {
            Log($"Cancelled during {item}");
            await SpinDownServiceAsync();
            return;
        }
        await LongTask2(item);

        if (token.IsCancellationRequested)
        {
            Log($"Cancelled during {item}");
            await SpinDownServiceAsync();
            return;
        }
        await LongTask3(item);

        if (token.IsCancellationRequested)
        {
            Log($"Cancelled during {item}");
            await SpinDownServiceAsync();
            return;
        }
        await LongTask4(item);

        if (token.IsCancellationRequested)
        {
            Log($"Cancelled during {item}");
            await SpinDownServiceAsync();
            return;
        }
        await LongTask5(item);
    }
}

But look at that needless repetition.

Thanks for your consideration.

like image 222
Jerry Nixon Avatar asked Aug 19 '16 21:08

Jerry Nixon


2 Answers

I don't have an exact type for LongTask1-5 but I'd do this.

public Task Process(CancellationTokenSource token)
{
    SOMETYPE[] tasks = new SOMETYPE[] {LongTask1, LongTask2, LongTask3, LongTask4, LongTask5};
    await SpinUpServiceAsync();
    foreach (var item in LongList())
    {

        foreach(var task in tasks){
            if (token.IsCancellationRequested) {
                Log($"Cancelled during {item}");
                await SpinDownServiceAsync();

                return;
            }
            await task(item);
        }



    }
} 

This approach puts the tasks in an array. If the requirements change, and more tasks are needed, then the array definition gets longer but the rest of the code stays the same. Good code uses loops for repetitive tasks, and NOT lots of copy-and-paste such as is in the original post.

like image 73
Xavier J Avatar answered Nov 06 '22 03:11

Xavier J


ok, it's my new version (it can be compiled)

prepare code (hacked)

class TypeOfItem { }

async Task LongTask1(TypeOfItem item) { }
async Task LongTask2(TypeOfItem item) { }
async Task LongTask3(TypeOfItem item) { }
async Task LongTask4(TypeOfItem item) { }
async Task LongTask5(TypeOfItem item) { }

async Task SpinUpServiceAsync() { }
async Task SpinDownServiceAsync() { }

IEnumerable<TypeOfItem> LongList() { yield break; }

void Log(string text) { }

and the solution:

private async Task<bool> CancelAt(TypeOfItem item)
{
  Log($"Cancelled during {item}");
  await SpinDownServiceAsync();
  return false; // cancel
}

public async Task<bool> Process(CancellationTokenSource token)
{
  await SpinUpServiceAsync();

  foreach (var item in LongList())
  {
    if (token.IsCancellationRequested) return await CancelAt(item);
    await LongTask1(item);

    if (token.IsCancellationRequested) return await CancelAt(item);
    await LongTask2(item);

    if (token.IsCancellationRequested) return await CancelAt(item);
    await LongTask3(item);

    if (token.IsCancellationRequested) return await CancelAt(item);
    await LongTask4(item);

    if (token.IsCancellationRequested) return await CancelAt(item);
    await LongTask5(item);
  }
  return true; // all done
}

@codenoire: good solution, but needed some fixes... :-)

public async Task Process(CancellationTokenSource token)
{
  var tasks = new Func<TypeOfItem, Task>[] { LongTask1, LongTask2, LongTask3, LongTask4, LongTask5 };
  await SpinUpServiceAsync();
  foreach (var item in LongList())
  {
    foreach (var task in tasks)
    {
      if (token.IsCancellationRequested)
      {
        Log($"Cancelled during {item}");
        await SpinDownServiceAsync();
        return;
      }
      await task(item);
    }
  }
}
like image 33
MaxKlaxx Avatar answered Nov 06 '22 03:11

MaxKlaxx