2
\$\begingroup\$

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);
}
\$\endgroup\$
1
  • 2
    \$\begingroup\$ It's good form to give a little back ground about what your code does. Welcome to CodeReview! =) \$\endgroup\$
    – RubberDuck
    Jun 3, 2014 at 22:05

2 Answers 2

1
\$\begingroup\$

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).

\$\endgroup\$
0
\$\begingroup\$

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

\$\endgroup\$
1
  • \$\begingroup\$ Why is there a specific reason ? And how would you handle the exception? \$\endgroup\$
    – Marc-Andre
    Jun 24, 2014 at 13:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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