1
\$\begingroup\$

We have a JavaScript function declaration as such:

function itemIncludesTotal(item){
    var includeItem = false;
    if (item.lineItem.hasOwnProperty("details")
      && item.lineItem.details.hasOwnProperty("total")
      && item.lineItem.details["total"]
      && !item.lineItem.restricted
    ){
      includeItem = true;
    }
    return includeItem;
  };

Where the function's argument (item) would be:

var item = {lineItem: {details: {total: "$5.00"}, restricted: true}};

The function call and return values would be:

itemIncludesTotal(item); => TRUE || FALSE

Our team is in a discussion regarding its structure:

  1. Should it be left alone?
  2. Should it be refactored?
  3. How would you refactor this function?
  4. Why did you refactor the way you did?
  5. What if anything do you personally find pleasing about the function?
  6. What if anything do you personally find unpleasing about the function?
  7. Does this function match current industry practices?
  8. What industry practices, if any, does this function lack?
\$\endgroup\$
3
\$\begingroup\$

I'd implement trivial things trivially if the data is as plain and simple as shown in the question.

function includesTotal(item) {
    return item.lineItem.details
        && item.lineItem.details.total
        && !item.lineItem.restricted ? true : false;
}

The last ? true : false is simply to make it obvious that the return value is boolean.

For a generic function I'd check the path fully though:

function includesTotal(item) {
    return item
        && item.lineItem
        && item.lineItem.details
        && item.lineItem.details.total
        && !item.lineItem.restricted ? true : false;
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ wOxxOm, could you also explain why you removed the variable "includeItem". Do you feel that by removing the variable the function is any less understandable? \$\endgroup\$ – SoEzPz Oct 20 '16 at 15:25
  • 1
    \$\begingroup\$ Extra intermediate (not mandatory) variables are used to clarify the purpose of a non-obvious expression. In this case the function name itself is a clarifying variable, especially in ES6: let includesTotal = (item) => .....;. Adding this variable in such trivial case obfuscates the code by implying a non-existent significance of the intermediate result. It also vaguely reminds me of x86 assembly and some language I've used in the '90s that didn't have return inside functions/procedures. \$\endgroup\$ – wOxxOm Oct 20 '16 at 15:44
  • \$\begingroup\$ If I could ask your experience one more question, what do you mean by "especially in ES6"? \$\endgroup\$ – SoEzPz Oct 20 '16 at 15:47
  • \$\begingroup\$ I mean in case of simple expressions, the ES2015/ES6 arrow function declaration syntax is basically an assignment operator by itself. \$\endgroup\$ – wOxxOm Oct 20 '16 at 15:55

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.