Choosing a CSS class based on an HTTP status code

Question

I was having a discussion with my coworker about a function that returns either an empty string, or a string of a css class. The function is passed an object which has an HTTP status code, and then depending on that statusCode, it returns a value. We couldn't agree which is cleaner.

He wanted the function written as so:

function getCssClass(options) {
  var statusCodeCssClass = '';
  var statusCode = parseInt(options.statuscode, 10);

  if ((statusCode >= 100 && statusCode < 200) || statusCode >= 400) {
    statusCodeCssClass = 'text-danger';
  }
  return statusCodeCssClass;
};

And I wanted it written as so:

function getCssClass(options) {
  var statusCode = parseInt(options.statuscode, 10);
  if ((statusCode >= 100 && statusCode < 200) || statusCode >= 400) {
    return 'text-danger';
  }
  return '';
};

The main difference is that I don't think the initial empty variable should be there, and he does. His argument is that the variable makes it easier to read, and my argument is that the variable obfuscates the function.

I know it's splitting hairs, but I was unable to find an answer in the Clean Code book or on Google about unnecessary variable declarations.

Can someone tell me why one is better than the other?

Answer 1

One very important principle of modern software design is that mutability should be minimised. The first case needlessly introduces mutability by potentially modifying the value of statusCodeCssClass. Thus I'd argue it's bad practice to write code like this, regardless of whether it's easier to read (which is a completely subjective claim as both are simple and easy-to-read).

However, the second approach ignores the fact that JavaScript supports an idiomatic solution to to a "test a value and return one of two results based on that test" problem: the ternary operator. This can be used in this situation to yield a third solution that makes better use of language features:

function getCssClass(options) {
    var statusCode = parseInt(options.statuscode, 10);
    return (statusCode >= 100 && statusCode < 200) || statusCode >= 400) ? 'text-danger' : '';
}

However, the code could be further improved by respecting the single responsibility principle. The function should only have one responsibility. Currently, it has two: it extracts the status code from options and it returns a CSS value based on that code. By splitting out obtaining the status code into another function, this one becomes more focused.

Answer 2

That's a matter of style entirely.. But I'd go with this:

function getCssClass(options) {
    var statusCode = parseInt(options.statuscode, 10);
    return statusCode >= 200 && statusCode < 300 ? '' : 'text-danger';
}

This checks for 2xx response codes, which communicates the intent ("was everything ok") better.

Answer 3

First of all, the function name is too generic. Instead of getCssClass, you should use getStatusCodeCssClass or something similar, describing that it calculates the CSS class based on status code.

Second, why is this function responsible for getting and parsing the status code? It should be passed directly.

Third, what do the conditions mean? The conditions probably represent error conditions. Why not make that clearer by calling a function isErrorStatusCode?

Taking these into account, you'd end up with:

function isErrorStatusCode(statusCode) {
    return (statusCode >= 100 && statusCode < 200) || (statusCode >= 400);
}

function getStatusCodeCssClass(statusCode) {
    return isErrorStatusCode(statusCode) ? 'text-danger' : '';
}

function getStatusCode(options) {
    return parseInt(options.statuscode, 10);
}

Then, call it with getStatusCodeCssClass(getStatusCode(options)).

Answer 4

I disagree when others say it's a matter of style. Imo the first function only adds more clutter to the code without providing any value. It is completely clear what the second function does - definitely the second one.