Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I'm writing a program that will perform a long-running computation, gradually improving a solution. On top of that, there's a simple UI that basically allows the user to stop the computation (letting the worker finish a step it's working on) and shows the best solution. Here's a simplified version of what I have, I'm using async/await for the first time. Is there anything you would improve, or any other comments?

Worker code

public class SearchEngine
{
    //...
    public async Task<IProblemSolution> Search(CancellationTokenSource cts)
    {
        await Task.Factory.StartNew(() => {
            BestSolution = Problem.RandomSolution();
            while (!cts.IsCancellationRequested)
            {
                IProblemSolution s = Problem.RandomSolution(); // for simplification 
                if (s.ObjectiveValue() < BestSolution.ObjectiveValue())
                    BestSolution = s;
            }
        });
        return BestSolution;
    }
}

UI

    private async void StartButton_Click(object sender, RoutedEventArgs e)
    {
        if (CancelTokenSrc != null)
            return;

        CancelTokenSrc = new CancellationTokenSource();
        Engine = new SearchEngine(new SampleProblem());

        StopButton.IsEnabled = true;
        StartButton.IsEnabled = false;
        Task<IProblemSolution> t = Engine.Search(CancelTokenSrc);
        await t;

        ResultLabel.Content = t.Result.ObjectiveValue().ToString();
        StopButton.IsEnabled = false;
        StartButton.IsEnabled = true;
        CancelTokenSrc = null;
    }

    private void StopButton_Click(object sender, RoutedEventArgs e)
    {
        if (CancelTokenSrc != null)
            CancelTokenSrc.Cancel();
    }
share|improve this question

Task-based Asynchronous Pattern (MSDN)

public async Task<IProblemSolution> Search(CancellationTokenSource cts)

As I explain in this answer, one would rightfully expect the signature of that method to read like this:

public async Task<IProblemSolution> SearchAsync(CancellationTokenSource cts)

The pattern for async methods is quite simple: use the async keyword, return Task or Task<T>, and put an "Async" suffix to the method's name.


I haven't written much async code (read: never wrote a line of C# that leverages features of .net 4.5), but from previous reviews of such code I think this:

    Task<IProblemSolution> t = Engine.Search(CancelTokenSrc);
    await t;

    ResultLabel.Content = t.Result.ObjectiveValue().ToString();

Could be written like this:

    var searchResult =  await Engine.SearchAsync(CancelTokenSrc);
    ResultLabel.Content = searchResult.ObjectiveValue().ToString();
    //...

My understanding is that await somehow deals with Task.Result and so the type of searchResult would be IProblemSolution - I might be wrong here though.

Other than that, I find you could give var some lovin', and change this:

IProblemSolution s = Problem.RandomSolution();

To this:

var s = Problem.RandomSolution();

Lastly, I might be biased because I use ReSharper and with that tool IntelliSense/auto-complete is much "smarter", but I don't like names like cts - I'd rather call it cancelTokenSource and then I can type ctsTab and poof I'm referring to a readable identifier with a pronounceable name.

share|improve this answer
    
“I don't think the Click handler for your StartButton is called asynchronously, you can drop the async keyword in that signature.” That's not how async works. It's required if you use await, which this method does. And since it's an event handler, async void is the right signature. – svick Apr 4 '14 at 20:09
    
But you're right about the part of using await instead of Result. – svick Apr 4 '14 at 20:11
    
@svick right, just found out - I'll edit to correct. Thanks! – Mat's Mug Apr 4 '14 at 20:13

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.