Should “Action” delegate be refactored out into a new method?

Question

I have a following method

    private void SetProcessDocumentStatus(string status)
    {
        var setStatusWith = new Action<string>(
            statusValue => processDocumentStatusLabel.Text = statusValue);
        if (processDocumentStatusLabel.InvokeRequired)
            processDocumentStatusLabel.Invoke(
                (MethodInvoker)(() => setStatusWith(status)));
        else
            setStatusWith(status);
    }

From the above code, I am encapsulating action into setStatusWith. Should that action be refactored into another method as follows?

    private void SetProcessDocumentStatusWith(string status)
    {
        processDocumentStatusLabel.Text = status;
    }

    private void SetProcessDocumentStatus(string status)
    {
        if (processDocumentStatusLabel.InvokeRequired)
            processDocumentStatusLabel.Invoke(
                (MethodInvoker)(() => SetProcessDocumentStatusWith(status)));
        else
            SetProcessDocumentStatusWith(status);
    }

I am wondering if “Action” delegate should be used sparingly in code.

Answer 1

What you have seems perfectly clear to me. Moving the lambda to a separate function adds lines of code but no clarity.

I would probably write the first line as:

  Action<string> setStatusWith = 
    statusValue => processDocumentStatusLabel.Text = statusValue;

but I don't know which way is generally preferred.

Answer 2

There is nothing inherently wrong with using the Action delegate. The only reason I would say that you should have two separate methods here would be if you feel that you need to bypass the check to see if the InvokeRequired property is true and the call has to be marshalled. However, I see no clear reason to do that.

Personally, I believe that code that checks for InvokeRequired and then marshals a call to itself is a bad idea. I'm more a fan of the caller having to be aware of the environment that they are in (is it a UI thread or no) and then making the determination as to whether or not to marshal the call.

Answer 3

If anything, I would wrap the logic to perform the invoking in a function since that is logic that is going to be used many times (basically all functions that can be set async).

    private void SetProcessDocumentStatus(string status)
    {
        RunOnControl<string>(
            processDocumentStatusLabel, 
            statusValue => processDocumentStatusLabel.Text = statusValue,
            status);
    }

    private void RunOnControl<T>(Control control, Action<T> action, T param)
    {
        if (control.InvokeRequired)
            control.Invoke(action, param);
        else
            action(param);
    }

Answer 4

In the example, you make the action accept a parameter: Action<string> but then you only ever pass it the same parameter, the status. So it doesn't need to accept a parameter - maybe you are unaware of the ability of a lambda to capture variables?

private void SetProcessDocumentStatus(string status)
{
    Action setStatusWith = () => processDocumentStatusLabel.Text = status;

    if (processDocumentStatusLabel.InvokeRequired)
        processDocumentStatusLabel.Invoke(() => setStatusWith());
    else
        setStatusWith();
}

Or to adjust Alan Jackson's excellent answer:

private void SetProcessDocumentStatus(string status)
{
    processDocumentStatusLabel.Run(ctrl => ctrl.Text = status);
}

public static class ControlExtensions
{
    public static void Run<T>(this TControl control, Action<TControl> action)
        where TControl : Control
    {
        if (control.InvokeRequired)
            control.Invoke(() => action(control));
        else
            action(control);
    }
}

Here I've made the Action take the control as a parameter instead. Also I've made the helper method into an extension, which seems reasonable.