Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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?

share|improve this question

migrated from programmers.stackexchange.com Jan 13 at 19:22

This question came from our site for professional programmers interested in conceptual questions about software development.

1  
Sigh! The young people today's don't care (or even seem to know ) about clock cycles. Test first for the most frequently expected result (hopefully, success) and save a few nano-seconds ;-) – Mawg Jan 14 at 14:18
    
Thanks for reminder ;) – Tom Halley Jan 14 at 14:58
up vote 9 down vote accepted

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.

share|improve this answer

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.

share|improve this answer
    
You could also use a single statement. I guess that you use two so that you can examine the statusCode when debugging (?) – Mawg Jan 14 at 14:16
1  
And, more importantly, to have no duplication – Bruno Schäpper Jan 15 at 13:12
    
He he - I commented above that no one worries about clock cycles any more - and now I am not concerned about code size :-) – Mawg Jan 15 at 13:15

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

share|improve this answer

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.

share|improve this answer

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.