1
\$\begingroup\$

I am new to async and was wondering if any improvement can be made to the solution I put together from scattered information on the web.

The following (working) code shows how a button press runs logic and if there are any errors they will appear in the returned task.Exception.Handle variable which I can check in my ContinueWith code. If it's not null, I open the error dialog window for the user. By passing in FromCurrentSynchronizationContext, I get to open the window on the UI thread and avoid issues with UI thread access.

Any improvements to make it more readable or suggest a different patter for this would be much welcomed.

public void StartButton()
{
    //prevent multiple button presses with IsRunning switch
    if (!IsRunning)
    {
        IsRunning = true;

        Threading.Task.Factory.StartNew(() =>
        {
            ProgramLogic();
            IsFabRunning = false;
        })
        .ContinueWith(p =>
        {
            if (p.Exception != null)
                p.Exception.Handle(x =>
                {
                    logger.Error(p.Exception);
                    windowManager.ShowDialog(new ForceCloseViewModel(p.Exception.Message), null, null);
                    IsFabRunning = false;
                    return true;
                });
        }, Threading.TaskScheduler.FromCurrentSynchronizationContext());
    }
}
\$\endgroup\$
2
\$\begingroup\$

This is a good fit for async await:

public async void RunProgramLogic()
{
    if (!IsRunning)
    {
        IsRunning = true;
        try
        {
            await Task.Run(() => ProgramLogic())
                      .ConfigureAwait(true); // schedules back to the synchronization context the method was called from.
        }
        catch (Exception e)
        {
            logger.Error(e); // Log or LogError feels like better names
            windowManager.ShowDialog(new ForceCloseViewModel(e.Message), null, null);
        }
        finally
        {
            IsRunning = false;
        }
    }
}

Regarding async await As you can see using async and await removes quite a bit of noise from the code. In general async void is smelly and should only be used for eventhavndlers. In your case it feels like a decent fit since you handle the exceptions. Async void means it cannot be awaited.

Regarding command: While I agree that this looks like a fit for a RelayCommand it is not trivial due to the async.

Here is a good read on commands for async scenarios.

Most RelayCommand implementations handle raising CanExecuteChanged poorly by calling CommandManager.InvalidateRequerySuggested which is very inefficient.

Here is a nice implementation.

Regarding scheduling Always be explicit with .ConfigureAwait(true|false) when awaiting. The default is true which means that the code continues on the synchronizationcontext it came from. This is a strange default in my opinion and ConfigureAwait is a poorly named method. In your case I think the logger and windowManager should be thread agnostic enabling .ConfigureAwait(false) Suggested implementation:

public class WindowManager
{
    public void ShowDialog(object viewModel, ...)
    {
        if (Dispatcher.CurrentDispatcher.CheckAccess())
        {
            ShowDialogCore(viewModel, ...);
        }
        else
        {
            Dispatcher.CurrentDispatcher.BeginInvoke(new Action(() => ShowDialogCore(viewModel, ...)));
        }
    }

    private void ShowDialogCore(object viewModel, ...)
    {
        ...
    }
}

The upside with implementing it like this is that the caller will not need to remember to call it on the UI thread. It is the job of the windowmanager to schedule if needed. Hence fixing many potential bugs in one place.

Regarding the pattern Using a flag like this is fine and will work well if you refactor to relaycommand. A try-finally block makes it clear in my opinion.

\$\endgroup\$
  • \$\begingroup\$ I was under the impression that, by default, continuations sare made on the context that awaited the awaitable. not the that the awaitable is executing on. \$\endgroup\$ – Gusdor Oct 26 '15 at 12:57
  • \$\begingroup\$ The default is ConfigureAwait(true) which does what you say and is a poor default. I prefer being explicit about it always. \$\endgroup\$ – Johan Larsson Oct 26 '15 at 15:38
1
\$\begingroup\$

This problem:

public void StartButton()
{
    //prevent multiple button presses with IsRunning switch
    if (!IsRunning)
    {
        IsRunning = true;

... can be solved far more elegantly by implementing MVVM and using commands, both of which you should be doing anyway. I'd write an example using your code, but you've not given us much to work with (and I don't have the time right now anyway).

Also, StartButton() is a really bad name for a method.

\$\endgroup\$
  • 1
    \$\begingroup\$ Commands have the advantage of making easily named methods: CanExecuteStartCommand and ExecuteStartCommand are pretty much as good as it gets =) \$\endgroup\$ – Mathieu Guindon Aug 27 '15 at 3:54
  • \$\begingroup\$ Thanks for the replies. I changed some names to make it more clear as a code snippet but i thought i would check why its a bad name in your opinion as i normally do suffix functions names linked to UI elements with the UI element type. FYI the actual name is "FabIssueOnSelectionButton". This is a bit of a divergence as i was more interested in improving the async part but its good to get feedback! \$\endgroup\$ – DBHC Aug 28 '15 at 7:23
  • \$\begingroup\$ @DBHC I would assume that something called "xxxxButton" is the name of a Button. Microsoft's Method Naming Guidelines say: "Use verbs or verb phrases to name methods." IMO the code-behind in WPF should be very small, and UI elements should bind to actions via Commands. Avoid closely linking your UI and Tasks etc, if you need to report results to the user while tasks are executing, consider a "ReportingService" or something alike (example). \$\endgroup\$ – BCdotWEB Aug 28 '15 at 7:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.