CODEWITHSUNDEEP
.netgenericsrefactoring
dance2die
dance2die

Reputation: 36925

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

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.

Upvotes: 1

Views: 248

Answers (4)

Daniel Earwicker
Daniel Earwicker

Reputation: 116714

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.

Upvotes: 2

Alan Jackson
Alan Jackson

Reputation: 6511

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

Upvotes: 1

casperOne
casperOne

Reputation: 74540

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.

Upvotes: 1

RossFabricant
RossFabricant

Reputation: 12492

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.

Upvotes: 5

Related Questions