1
\$\begingroup\$

This function derives a 'stars' rating from a product score that is retrieved from local storage. What would be the best way to refactor these else if statements?

    $scope.ratingStars = function(productID) {

        var bottomLine = $filter('filter')($localStorage.bottomlinesObject.bottomlines, {domain_key:''+productID})//jshint ignore:line

        if(bottomLine.length > 0) {

            if (bottomLine[0].product_score === 'undefined') {return 0;}//jshint ignore:line
            else if(bottomLine[0].product_score > 0 && bottomLine[0].product_score <=1.5)  {return 1;}// jshint ignore:line
            else if(bottomLine[0].product_score > 1.5 && bottomLine[0].product_score <=2) {return 2;}//jshint ignore:line
            else if(bottomLine[0].product_score > 2 && bottomLine[0].product_score <=2.5) {return 2.5;}//jshint ignore:line
            else if(bottomLine[0].product_score > 2.5 && bottomLine[0].product_score <=3.2) {return 3;}//jshint ignore:line
            else if(bottomLine[0].product_score > 3.2 && bottomLine[0].product_score <=3.7) {return 3.5;}//jshint ignore:line
            else if(bottomLine[0].product_score > 3.7 && bottomLine[0].product_score < 4.3) {return 4;}//jshint ignore:line
            else if(bottomLine[0].product_score >= 4.3 && bottomLine[0].product_score < 4.75) {return 4.5;}//jshint ignore:line
            else if(bottomLine[0].product_score >= 4.75 && bottomLine[0].product_score < 5) {return 5;}//jshint ignore:line
            else if(bottomLine[0].product_score >= 5) {return 5;}// jshint ignore:line

        }


        else {return 0;}


    };
\$\endgroup\$
  • 1
    \$\begingroup\$ Hi. Welcome to Code Review! Your title and problem statement are focused on how the code does things. But what does it actually do? What input does it take and what is the output supposed to be? Does the code actually run as is? Or does it need more context that you removed. \$\endgroup\$ – mdfst13 Jul 12 '16 at 0:37
  • \$\begingroup\$ Hmmm, this is written well enough (naming wise) that the functionality is obvious to at least me. I dont think this is worth a down vote. \$\endgroup\$ – konijn Jul 12 '16 at 14:39
1
\$\begingroup\$

Since the intervals aren't consistent, rounding wouldn't help, but you can drop the floor condition since you're using else-if.

    if(bottomLine.length > 0) {
        if (bottomLine[0].product_score === 'undefined') {return 0;}//jshint ignore:line
        else if(bottomLine[0].product_score <=1.5)  {return 1;}// jshint ignore:line
        else if(bottomLine[0].product_score <=2) {return 2;}//jshint ignore:line
        else if(bottomLine[0].product_score <=2.5) {return 2.5;}//jshint ignore:line
        else if(bottomLine[0].product_score <=3.2) {return 3;}//jshint ignore:line
        else if(bottomLine[0].product_score <=3.7) {return 3.5;}//jshint ignore:line
        else if(bottomLine[0].product_score < 4.3) {return 4;}//jshint ignore:line
        else if(bottomLine[0].product_score < 4.75) {return 4.5;}//jshint ignore:line
        else {return 5;}//jshint ignore:line
    }

And, since you're returning each time, you can drop the "else" part of the else if for that matter.

    if(bottomLine.length > 0) {
        if (bottomLine[0].product_score === 'undefined') {return 0;}//jshint ignore:line
        if(bottomLine[0].product_score <=1.5)  {return 1;}// jshint ignore:line
        if(bottomLine[0].product_score <=2) {return 2;}//jshint ignore:line
        if(bottomLine[0].product_score <=2.5) {return 2.5;}//jshint ignore:line
        if(bottomLine[0].product_score <=3.2) {return 3;}//jshint ignore:line
        if(bottomLine[0].product_score <=3.7) {return 3.5;}//jshint ignore:line
        if(bottomLine[0].product_score < 4.3) {return 4;}//jshint ignore:line
        if(bottomLine[0].product_score < 4.75) {return 4.5;}//jshint ignore:line
        return 5; //jshint ignore:line
    }

I think you should remove the redundant floor conditions, but I can see a case made either way with leaving the "else" in place.

\$\endgroup\$
1
\$\begingroup\$

In addition to Chris' answer, you could store the product_score to make each if statement shorter, i.e.

if(bottomLine.length > 0) {
    var score = bottomLine[0].product_score;

    if(score === 'undefined') { return 0; }
    if(score == 0) { return 0; }
    if(score <= 1.5) { return 1; }
    if(score <= 2) { return 2; }
    if(score <= 2.5) { return 2.5; }
    if(score <= 3.2) { return 3; }
    if(score <= 3.7) { return 3.5; }
    if(score <= 4.3) { return 4; }
    if(score <= 4.75) { return 4.5; }

    return 5;
}

return 0;

It's a shame there isn't an obvious formulaic method to arrive at the star value for a given score.

\$\endgroup\$
  • \$\begingroup\$ I think you need cover the case of score = 0 being 0 stars. \$\endgroup\$ – konijn Jul 12 '16 at 17:53
  • \$\begingroup\$ @konijn, OPs code shows any score in the range (0,1.5] is 1 star, and doesn't handle the explicit case of product_score = 0 but I added it in, and also the return 0 for the case when bottomLine.length is not >0 \$\endgroup\$ – Anthony Jul 12 '16 at 18:08
  • \$\begingroup\$ It is implicit by the final return 0;, and good call about the check on .length \$\endgroup\$ – konijn Jul 12 '16 at 18:09
1
\$\begingroup\$

Interesting question,

in addition the other reviews,

  • If your code is littered with // jshint ignore:line then you're doing it wrong, either dont use jshint or try to comply more

  • I would extract the logic that derives a starcount from a productscore into a separate function

  • I would probably challenge the business folks on the inconsistent <= and <

  • I would replace else {return 0;} with simply return 0;

  • I would replace

    if (bottomLine[0].product_score === 'undefined') {return 0;}
    

    with

    if (!bottomLine[0].product_score) //undefined or zero -> zero stars
      return 0;
    

    so that you dont need a return 0; at the end

\$\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.