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

How do I simplify this method? Is handling an exception in this method a good idea?

[WebMethod]
public string GetMDSToken(string xuid)
{
    var tokenTask = _mapTokenService.GetMappedSSOTokenAsync(xuid);
    var tokenTimestampTask = _SSOFrontendService.GetSSOTokenTimestampAsync();

    Task.WhenAll(tokenTask, tokenTimestampTask);

    if (tokenTimestampTask.Result.Status != ResultStatus.OK)
        return _serializer.Serizalize(tokenTimestampTask.Result);

    if (tokenTask.Result == null)
    {
        var response = _handledResponseFactory.CreateErrorResponce(ResultStatus.Business, NotLogginErrorMessage);
        return _serializer.Serizalize(response);
    }

    try
    {
        var mappedToken = tokenTask.Result;
        if (mappedToken.SSOTokenTimestamp != tokenTimestampTask.Result.Content)
            UpdateSSOToken(mappedToken);

        var MDSTokenTask = _MDSFrontendService.GetToken(mappedToken.SSOToken);

        Task.WhenAll(MDSTokenTask);
        return _serializer.Serizalize(MDSTokenTask.Result);
    }
    catch (Exception ex)
    {
        var response = _handledResponseFactory.CreateExceptionResponce(ex);
        return _serializer.Serizalize(response);
    }
}
share|improve this question
add comment

1 Answer

up vote 2 down vote

I can't speak for the language you a using specifically, but I have always been lead to believe that declaring variables within if and try statements is bad form. Can't say I've ever seen one in a catch statement, but I believe the same principle might apply.

You seem to be wrapping a lot in that try block. Do you expect that

var mappedToken = tokenTask.Result;

could actually throw an exception? If not, I would personally recommend removing it from that code block.

To be clear, how much code to put in a try block would be a matter of opinion, but are you obeying the single responsibility principle? I would posit that the code would be simplified if the reader knew what is expected to throw an exception at a glance. Your try block is responsible for 6 different tasks by my count. Should the reader assume that all of those tasks are capable of faulting, and that the resulting exception should be the same regardless of which part failed and which part was successful? If UpdateSSOToken were to execute but Task.WhenAll failed are we in the same boat we would be in if UpdateSSOToken had failed? Something to consider.

As for is it a good idea to have the exception at all-

The method you choose depends on how often you expect the event to occur.

To help you determine that you should consider the following about the WhenAll method of Tasks-

If any of the supplied tasks completes in a faulted state, the returned task will also complete in a Faulted state

This answer cites some useful information on exception with tasks, including

An Async Primer by Bill Wagner

which goes into detail about the await pattern. I would suggest at least reading the primer if none of the other links, I believe it directly addresses your question.

share|improve this answer
2  
In C#, variables should be declared as close as possible to their usage, and ideally be as short-lived as possible; declaring variables within an if or a try block is ok. However you are correct, tokenTask.Result is already accessed outside of the try block, hence mappedToken should be declared and assigned outside the try block. –  Mat's Mug Jun 3 at 22:28
    
+1 @Mat'sMug Thanks for that tidbit, I did not realize that this only applied to JavaScript. Now I have to re-optimize my .NET classes to reflect this. Better late than never I guess... –  blaze4218 Jun 4 at 16:02
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.