Multiple 'using' statements in method implementation

Question

I'm learning WebAPI framework for ASP.NET and C#. Is the following way of handling incoming data correct?

    [HttpPost]
    [ExpiredCookieCheck]
    public IHttpActionResult Auth([FromBody]AuthenticationData authenticationData)
    {
        try
        {
            if (authenticationData == null)
                using (var responseManager = new ResponseManager())
                    return responseManager.PrepareMessage(
                        HttpStatusCode.Unauthorized,
                        "No authentication model was provided in request body.",
                        null,
                        ResponseManager.ResponseType.InterfacePlainText
                    );

            using (var authenticationHandler = new AuthenticationHandler())
            {
                var validationData = authenticationHandler.CheckAuthenticationDataFromWeb(authenticationData);

                if (!validationData.Key)
                    using (var responseManager = new ResponseManager())
                        return responseManager.PrepareMessage(
                            HttpStatusCode.Unauthorized,
                            "Incorrect credentials for authentication.",
                            null,
                            ResponseManager.ResponseType.InterfacePlainText
                        );

                using (var cookieManager = new CookieManager())
                using (var responseManager = new ResponseManager())
                {
                    var authenticationCookie = cookieManager.CreateAuthenticationCookieHeader(Request.RequestUri.Host);
                    cookieManager.SaveAuthenticationCookie(validationData.Value.Id, authenticationCookie);

                    return responseManager.PrepareMessage(
                        HttpStatusCode.OK,
                        null,
                        authenticationCookie,
                        ResponseManager.ResponseType.InterfacePlainText
                    );
                }
            }
        }
        catch (Exception exception)
        {
            using (var responseManager = new ResponseManager())
                return responseManager.PrepareMessage(
                    HttpStatusCode.Unauthorized,
                    exception.ToString(),
                    null,
                    ResponseManager.ResponseType.InterfacePlainText
                );
        }
    }

I'm thinking that I'm using something not correctly. I dislike my way of defining using statements. Maybe it's miles better to define at the beginning of the method all required using statements and then only call internal methods of objects from those using statements.

Also, is it correct to use try/catch block of code in Web API controller method? I'm thinking that it could be incorrect to handle some possible errors in Web API method, couldn't it?

Answer 1

There is nothing inherently wrong with having multiple using statements. It keeps the lifetime of objects to the minimum which is not a bad thing to do.

Another point is that I'd possibly re-factor the error raising: You always prepare the error message with the same parameters except for the actual message. This could easily be encapsulated in a little helper function. So the refactored code could look like this:

private Response GetUnauthorizedResponse(string message)
{
    using (var responseManager = new ResponseManager())
    {
        return responseManager.PrepareMessage(
            HttpStatusCode.Unauthorized,
            message,
            null,
            ResponseManager.ResponseType.InterfacePlainText
        );
    }
}

[HttpPost]
[ExpiredCookieCheck]
public IHttpActionResult Auth([FromBody]AuthenticationData authenticationData)
{
    try
    {
        if (authenticationData == null)
        {
            return GetUnauthorizedResponse("No authentication model was provided in request body.");
        }

        using (var authenticationHandler = new AuthenticationHandler())
        {
            var validationData = authenticationHandler.CheckAuthenticationDataFromWeb(authenticationData);
            if (!validationData.Key)
            {
                return GetUnauthorizedResponse("Incorrect credentials for authentication.");
            }

            using (var cookieManager = new CookieManager())
            using (var responseManager = new ResponseManager())
            {
                var authenticationCookie = cookieManager.CreateAuthenticationCookieHeader(Request.RequestUri.Host);
                cookieManager.SaveAuthenticationCookie(validationData.Value.Id, authenticationCookie);

                return responseManager.PrepareMessage(
                    HttpStatusCode.OK,
                    null,
                    authenticationCookie,
                    ResponseManager.ResponseType.InterfacePlainText
                );
            }
        }
    }
    catch (Exception exception)
    {
        return GetUnauthorizedResponse(exception.ToString());
    }
}

This removed some distracting clutter and lets the reader focus better on the actual functionality of the method.

Answer 2

As a habit, I prefer to use using statements for only the granular pieces where they are applicable rather than wrapping the entire code file. The reason for this is that though you are calling dispose when the using block ends, a managed object (for example, database connections) will generally be placed in a pool for later use. In this way, you don't actually repeat the expensive object creation. On the flip side, something like file IO is less expensive, so even though you won't be returning the stream object to a pool, you're still being wise to keep the scope of a using block as small as possible. The larger your using block becomes, the higher the chance is that you'll either have an unaccounted for problem and use more resources than you intended or provide scope to pieces of code that do not need the resource access.

There is nothing wrong with your try-catch, but it would be beneficial to look through the Exception Types that can be thrown in all the methods you use in your catch block. Anything thrown here, after all, is not handled. Perhaps you want it to fail silently, or perhaps you might have a try catch within the catch, and if an exception is thrown, you send a more robust type of error message. For instance, I maintain an application that writes errors to a database. It's reasonable to think that it could lose database access, so I also have a simpler log method that dumps into a text file if the original error logging fails.

Answer 3

Please use Brackets

It isn't a necessity to make the code compile, but it is standard usage in C#

so this

        if (authenticationData == null)
            using (var responseManager = new ResponseManager())
                return responseManager.PrepareMessage(
                    HttpStatusCode.Unauthorized,
                    "No authentication model was provided in request body.",
                    null,
                    ResponseManager.ResponseType.InterfacePlainText
                );

becomes this:

if (authenticationData == null)
{    
    using (var responseManager = new ResponseManager())
    {
        return responseManager.PrepareMessage(
            HttpStatusCode.Unauthorized,
            "No authentication model was provided in request body.",
            null,
            ResponseManager.ResponseType.InterfacePlainText
        );
    }
}

and is instantly more readable.

it comes in handy, especially when you get to this chunk of your code

        using (var authenticationHandler = new AuthenticationHandler())
        {
            var validationData = authenticationHandler.CheckAuthenticationDataFromWeb(authenticationData);

            if (!validationData.Key)
                using (var responseManager = new ResponseManager())
                    return responseManager.PrepareMessage(
                        HttpStatusCode.Unauthorized,
                        "Incorrect credentials for authentication.",
                        null,
                        ResponseManager.ResponseType.InterfacePlainText
                    );

            using (var cookieManager = new CookieManager())
            using (var responseManager = new ResponseManager())
            {
                var authenticationCookie = cookieManager.CreateAuthenticationCookieHeader(Request.RequestUri.Host);
                cookieManager.SaveAuthenticationCookie(validationData.Value.Id, authenticationCookie);

                return responseManager.PrepareMessage(
                    HttpStatusCode.OK,
                    null,
                    authenticationCookie,
                    ResponseManager.ResponseType.InterfacePlainText
                );
            }
        }

my mind wanted to say you weren't using brackets on the using statement because I saw the if statement was also unbracketed, but you have bracketed the encompassing using statement, but not the one inside the if statement, which should be bracketed as should the if statement.

it should look like this

using (var authenticationHandler = new AuthenticationHandler())
{
    var validationData = authenticationHandler.CheckAuthenticationDataFromWeb(authenticationData);

    if (!validationData.Key)
    {
        using (var responseManager = new ResponseManager())
        {
            return responseManager.PrepareMessage(
                HttpStatusCode.Unauthorized,
                "Incorrect credentials for authentication.",
                null,
                ResponseManager.ResponseType.InterfacePlainText
            );
        }
    }

    using (var cookieManager = new CookieManager())
    using (var responseManager = new ResponseManager())
    {
        var authenticationCookie = cookieManager.CreateAuthenticationCookieHeader(Request.RequestUri.Host);
        cookieManager.SaveAuthenticationCookie(validationData.Value.Id, authenticationCookie);

        return responseManager.PrepareMessage(
            HttpStatusCode.OK,
            null,
            authenticationCookie,
            ResponseManager.ResponseType.InterfacePlainText
        );
    }
}

Answer 4

What does the using do? It is a terse way to call dispose on the object at the end of a given scope. So first, make sure you actually need to call dispose on the object at all, because it's completely possible the GC will take care of the object automatically. If you're concerned about garbage collection due to the async (server-side) nature of the request, don't worry about that. The garbage collector is smart and you don't have to manage the lifetime of objects like this in async contexts.

What's the purpose of your manager classes? I hope they are separate objects used to facilitate writing unit tests in isolation.

If they are, then you should also be testing your controllers and injecting the managers into your controllers instead of instantiating them inside the methods. That will solve your concerns about object lifetime right there--managing the lifetime will move from the controller to the component you wire up to inject the controllers.

If your manager objects are generally amalgamations of methods that hold subroutines, then I would move away from the using statements entirely because chances are you don't need to worry about writing code to control their disposal. In fact, I would question the need for some of these objects to stand as individual objects. For example I'm not sure you need a separate class to manufacture custom responses--it may be just as much code to call the object as it would be to inline the response logic in your controller and you may introduce unwanted coupling in your code. You haven't shared your manager classes, though, so I can only speculate.

As for the exception handling, you should either set up exception filters, or set up a global exception handler, or both. Per-controller method try/catch should not be necessary, generally speaking. You should either have the try/catch happening in your manager classes or further upstream, or completely outside of the manager classes/controller.

Answer 5

For objects that implement IDisposable, a using block is the best way to assure that you call Dispose on them when you are done, and the scoping prevents accidental use after disposal. In that way, your code is fine.

I wonder if those classes (AuthenticationHandler, CookieManager, ResponseManager) really need to implement that interface, or if that is a misunderstanding or design error on the part of the author of those classes. I would take another look at them and see if they can be simplified, and that simplification could propagate to your code.