Reusing too much code, but I cannot figure out how to refactor this correctly [closed]

Posted on

Problem

I have some code that run a series of functions in order and then outputs the results of each. When I execute this code, I am reusing a ton of it, where only the function itself in the code is changing. MethodThatChanges in the below example, is a method that returns void.

Task[] task1 = new Task[10];
for (int i = 0; i < 10; i++)
{
    task1[i] = Task.Factory.StartNew(() =>
    {
        MethodThatChanges1(i);
    });
}
Task.WaitAll(task1);

Task[] task2 = new Task[10];
for (int i = 0; i < 10; i++)
{
    task2[i] = Task.Factory.StartNew(() =>
    {
        MethodThatChanges2(i);
    });
}
Task.WaitAll(task2);

etc........

I would like to write a function instead that contains all of this code and passes in as a parameter, the method that each section calls. Something like this, obviously this doesn’t work as is…

void ExecuteTask(void method)
{
    Task[] task = new Task[10];
    for (int i = 0; i < 10; i++)
    {
        task[i] = Task.Factory.StartNew(() =>
        {
            method(i);
        });
    }
    Task.WaitAll(task);
}

Then I could replace all of the copied code with:

ExecuteTask(method1);
ExecuteTask(method2);
etc...

I’ve looked into Action and Func delegates, but I do not understand exactly how that work. Any help would be appreciated.

Solution

You were on the right track with Action delegates. There is another problem however: the loop variable i is captured inside the lambda expression. Since it is the same variable in each Task, when you do i++ in the for-loop, other Tasks will use the new value as well.

(This is a very weird problem, more on this on Eric Lippert’s blog: Closing over the loop variable considered harmful) It can be solved by simply creating a copy of i.

void ExecuteTask(Action<int> method) {
    Task[] task = new Task[10];
    for (int i = 0; i < 10; i++)
    {
        int copy = i;
        task[i] = Task.Factory.StartNew(() => method(copy));
    }
    Task.WaitAll(task);   
}

ExecuteTask(method1);
ExecuteTask(method2);

An Action variable would be what you’re looking for as it appears as though your method doesn’t return anything. (If it returns something, use a Func.) So create another method that will do the following:

void ActionMethod(Task[] tasks, Action<int> method) {
    for (int i = 0; i < tasks.Length; i++) {
        tasks[i] = Task.Factory.StartNew(() => method(i));
    }
}

Then you can call it from your other methods like so:

Task[] tasks1 = new Task[10];
Task[] tasks2 = new Task[10];

ActionMethod(tasks1, MethodThatChanges1);
ActionMethod(tasks2, MethodThatChanges2);

Leave a Reply

Your email address will not be published. Required fields are marked *