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 2 down vote favorite

How can I improve this code, without to return async Task?

[WebMethod]
public string Login(string login, string password, string deviceId, string appVersion, string xuid)
{            
    var authTask = _SSOFrontendService.GetSSOTokenAsync(login, password, deviceId, appVersion);
    var tokenTimestampTask = _SSOFrontendService.GetSSOTokenTimestampAsync();

    Task.WhenAll(authTask, tokenTimestampTask);
    var authResult = authTask.Result;

    if (authResult.Status == ResultStatus.OK)
    {
        var token = new MappedToken(xuid, login, password, deviceId, appVersion, authResult.Content, tokenTimestampTask.Result.Content);
        var mapTokenTask = _mapTokenService.MapAsync(token);

        Task.WhenAll(_mapTokenService.MapAsync(token));
        return _serializer.Serizalize(mapTokenTask.Result);
    }
    return _serializer.Serizalize(authResult);
}
share|improve this question
2  
It's good form to give a little back ground about what your code does. Welcome to CodeReview! =) –  ckuhn203 Jun 3 at 22:05
add comment

2 Answers

up vote 0 down vote

First major thing while using TPL, use exception handling. Its very important to catch the AggregateException and handle the threads behaviour.

share|improve this answer
    
Why is there a specific reason ? And how would you handle the exception? –  Marc-Andre Jun 24 at 13:38
add comment
up vote 0 down vote

How can I improve this code, without to return async Task?

Doing this doesn't make much sense. If the whole method is going to be synchronous, then you're not gaining much by using asynchronous methods in it, because the synchronous method still needs to block a thread.

Though there might be a way to make async-await work with ASMX.

Task.WhenAll(authTask, tokenTimestampTask);

This doesn't do anything at all. WhenAll() returns a Task that represents the two Tasks combined, so that you can await them. But you're not doing anything with that Task.

You could instead use WaitAll(), but I don't see any reason for that, using Result makes sure that the Task is already completed before returning the value (it does this by blocking the thread if the Task is not completed yet, just like Wait() and WaitAll()).

Task.WhenAll(_mapTokenService.MapAsync(token));

I can see three things wrong with this line:

  1. The WhenAll() issue mentioned above.
  2. You have only a single Task, so you don't need WhenAll() or WaitAll().
  3. You're calling MapAsync() for the second time unnecessarily.
if (authResult.Status == ResultStatus.OK)
{
   …
}
return _serializer.Serizalize(authResult);

It's often best to deal with the failure state immediately by inverting the if:

if (authResult.Status != ResultStatus.OK)
{
    return _serializer.Serizalize(authResult);
}

…

This way reduces the level of nesting for the main part of the code and also improves its flow (you're dealing with the failure in a single place).

share|improve this answer
add comment

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.