patternjavascriptModerate
Choosing a CSS class based on an HTTP status code
Viewed 0 times
classcssstatushttpbasedcodechoosing
Problem
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:
And I wanted it written as so:
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?
He wanted the function written as so:
function getCssClass(options) {
var statusCodeCssClass = '';
var statusCode = parseInt(options.statuscode, 10);
if ((statusCode >= 100 && 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 = 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?
Solution
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
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:
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
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 = 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.Code Snippets
function getCssClass(options) {
var statusCode = parseInt(options.statuscode, 10);
return (statusCode >= 100 && statusCode < 200) || statusCode >= 400) ? 'text-danger' : '';
}Context
StackExchange Code Review Q#116696, answer score: 10
Revisions (0)
No revisions yet.