2
\$\begingroup\$

I need to create method which handle HttpResposeMessage. Method should throw an exception if error exist, client will handle rest. You can see my code bellow. This code is working fine and doing what I want, but still it looks quite ugly to me.

Do you have some ideas for nicer implementation?

  [Authorize]
  public abstract class BaseController : Controller
    {
        protected readonly ILogger logger;
        protected readonly IHostingEnvironment env;
        private readonly IStringLocalizer localizer;

        public BaseController(ILogger logger, IHostingEnvironment env, IStringLocalizer localizer)
        {
            this.logger = logger;
            this.env = env;
            this.localizer = localizer;
        }

       protected void HandleResponseError(HttpResponseMessage response, ILogger logger)
        {
        if (!response.IsSuccessStatusCode)
        {
            var message = String.Empty;
            if (response.StatusCode.Equals(HttpStatusCode.Forbidden))
            {
                message = this.localizer["Forbidden"] + ": " + this.localizer["You have not the appropriate rights to access this page."];
            }
            else
            {
                try
                {
                    message = response.Content.ReadAsStringAsync().Result;
                    if (string.IsNullOrWhiteSpace(message))
                    {
                        message = message = this.localizer["Communication error with server"];
                    }
                }
                catch
                {
                    message = this.localizer["Communication error with server"];
                }
            }
            logger.LogError($"Client error message {message} !");
            throw new ApplicationException(message);
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ I agree with @BCdotWEB that it's strange that localizer is a field but the logger not. You should post the entire code. There seem to be much more wrong with it. \$\endgroup\$ – t3chb0t Oct 12 '16 at 8:18
  • \$\begingroup\$ Both are fields, see my edit. \$\endgroup\$ – Raskolnikov Oct 12 '16 at 8:45
4
\$\begingroup\$

Considering that there is no else to if (!response.IsSuccessStatusCode), you should do:

if (response.IsSuccessStatusCode)
{
   return;
}

This saves on indentation and makes your method easier to read.


I'd use string.Format for things like this:

message = this.localizer["Forbidden"] + ": " + this.localizer["You have not the appropriate rights to access this page."];

Why is localizer a property on the class this method belongs to, but logger is not? And why do you need to write this.localizer?


In the end there's not much code to review, and perhaps you should consider posting the whole class.

\$\endgroup\$
  • \$\begingroup\$ Thank you for help. Both localizer and logger are properties on class. Just logger does not have ''this" i my method. \$\endgroup\$ – Raskolnikov Oct 12 '16 at 8:30
4
\$\begingroup\$

I see this method does currently two things: it creates the error message and it throws an exception. This is too much and should be separated.

You should extract a method that creates the error message and call it by the handler.

protected string CreateErrorMessage(HttpResponseMessage response)
{
    if (response.StatusCode.Equals(HttpStatusCode.Forbidden))
    {
        return localizer["Forbidden"] + ": " + localizer["You have not the appropriate rights to access this page."];
    }

    try
    {
        var message = response.Content.ReadAsStringAsync().Result;
        if (string.IsNullOrWhiteSpace(message))
        {
            return localizer["Communication error with server"];
        }
    }
    catch
    {
        return localizer["Communication error with server"];
    }
}

and

protected void HandleResponseError(HttpResponseMessage response, ILogger logger)
{
    if (response.IsSuccessStatusCode)
    {
        return;
    }

    var message = CreateErrorMessage(response);        
    logger.LogError($"Client error message {message} !");
    throw new ApplicationException(message);
}
  • The code is usually easier to understand with less nesting so I changed the first condition to return if there was no error.
  • You use string interpolation but not everywhere - try to be consistent when it makes sense - here it does.

"Communication error with server"

This and other keys should be constants that could have much shorter names.

static class Translation
{
    const string AccessDenied = "...";
    const string CommunicationError = "...";
}

Usage:

return localizer[Translation.CommunicationError];

This is less error prone as you don't have to type the same text over and over again.

\$\endgroup\$
3
\$\begingroup\$

In the constructor two things can be improved. One and most important is the validation of arguments, so no null dependencies get injected, preventing a NullReferenceException down the line. The other, and less important, it's typically accepted as a good practice that constructors of abstract classes should not be public, but private instead, as they cannot be instantiated directly anyway. I would rewrite it as this:

protected BaseController(ILogger logger, IHostingEnvironment env, IStringLocalizer localizer)
{
    if(logger == null) throw new ArgumentNullException(nameof(logger));
    if(env == null) throw new ArgumentNullException(nameof(env));
    if(localizer == null) throw new ArgumentNullException(nameof(localizer));

    this.logger = logger;
    this.env = env;
    this.localizer = localizer;
}

In the HandleResponseError method, why there is a logger parameter? This is exactly the same that it's injected in the constructor, why are there two of them? It's not clear from the code. Possible alternatives would be to remove it and just use the class variable or if there is a legitimate need to have another, different logger for just this method, rename the parameter to something more specific regarding that, and use this parameter in the logging at the end.

As with the constructor, arguments should be validated and proper exceptions thrown as soon as possible:

if(response == null) throw new ArgumentNullException(nameof(response));
if(logger == null) throw new ArgumentNullException(nameof(logger));

As a final note, I would not use ApplicationException in case of failure, because it's considered useless and because generally this leads to catching general exceptions. Instead, I would define my own exception and try to catch that if possible. Even if that's not useful right now, it might provide some future-proof in case a special handling is needed with this particular case, which would be quite difficult to do if you use a general exception class.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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