current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

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());
    }
}
share|improve this question

1 Answer 1

up vote 1 down vote

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.

share|improve this answer
1  
Commands have the advantage of making easily named methods: CanExecuteStartCommand and ExecuteStartCommand are pretty much as good as it gets =) –  Mat's Mug♦ Aug 27 at 3:54
    
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! –  DBHC Aug 28 at 7:23
    
@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). –  BCdotWEB Aug 28 at 7:39

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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