Struggling with function factory & closure

Question

After a long time I played around with plain JS again. And I'm struggling in fixing a closure problem.

Task: I'm using a function factory to set params for a returned function which I then want to call.

Problem: As far as I understand the params are passed by reference and when the execution context is popped off the stack, the variables lead to undefined memory space. How can I make this work?

    function changeColorConstructor(greenLimit, redLimit) {
        var greenLimit = greenLimit;
        var redLimit = redLimit;
        console.log('1. Green Limit in Constructor shell: ' + greenLimit);
        
        return function(metric) {
          var color;
    
          console.log('3. Green Limit in returned function: ' + greenLimit);
    
          switch(metric) {
            case metric >= greenLimit:
              console.log('Green');
              color = '#56C621'; // green
              break;
            case metric < greenLimit && metric > redLimit:
              console.log('Yellow');
              color = '#F2E607'; // yellow
              break;
            case metric <= redLimit:
              console.log('Red');
              color = '#C64C20'; // red
              break;
            default:
              color = '#C0C0C0'
              break;
          }
          return color;
        }
    
      }
    
      var indexColor = changeColorConstructor(5000, 10000);
      console.log('2. Returned function: ' + indexColor);
    
      console.log('4. Output from production function: ' + indexColor(5000));

"*the variables lead to undefined memory space*" - No, that's not how JavaScript works. Describe your actual problem, not what you think the problem is. — melpomene, Mar 09 '18 at 15:04
Why are you creating local variables with the same name as the ones passed in parameters? — Nisarg Shah, Mar 09 '18 at 15:04
I think you're missing the /new/ keyword when assigning the function/object to indexColor --- var indexColor = new changeColorConstructor(5000, 10000); --- Otherwise, indexColor is a reference to the function rather than it's own thing. — Doug, Mar 09 '18 at 15:05
@Doug No, `indexColor` will be the function returned by `changeColorConstructor`, which is fine (and what the OP is trying to do). It's a function generator. — Dave Newton, Mar 09 '18 at 15:08
Thanks for all your input. Indeed the switch statement was off. Good personal learning: Keeping an open mind and don't prematurely diagnose the issue. — Juri, Mar 09 '18 at 15:27
Don't edit your question with the solution. If you want to answer your own question, post it as an answer. — melpomene, Mar 09 '18 at 15:30
@Juri Please consider the idea that this isn't a good use of the switch statement for reasons I list in my comment on the answer, and as discussed in the accepted answer of the linked question. — Dave Newton, Mar 09 '18 at 16:05

score 2 · Accepted Answer · answered Mar 09 '18 at 15:21

As far as I can see, your problem is not with closures or function factories, but with switch:

switch(metric) {
    case metric >= greenLimit:

This code compares metric to the result of metric >= greenLimit, i.e. it works like if (metric == (metric >= greenLimit)).

metric >= greenLimit is either true or false, so comparing this to metric doesn't make much sense.

In the snippet below I've replaced your switch statement by the if / else cascade you intended, and I've removed the nonsensical var greenLimit = greenLimit; assignments.

function changeColorConstructor(greenLimit, redLimit) {
    console.log('1. Green Limit in Constructor shell: ' + greenLimit);

    return function (metric) {
        var color;

        console.log('3. Green Limit in returned function: ' + greenLimit);

        if (metric >= greenLimit) {
            console.log('Green');
            color = '#56C621'; // green
        } else if (metric < greenLimit && metric > redLimit) {
            console.log('Yellow');
            color = '#F2E607'; // yellow
        } else if (metric <= redLimit) {
            console.log('Red');
            color = '#C64C20'; // red
        } else {
            color = '#C0C0C0'
        }
        return color;
    };
}

var indexColor = changeColorConstructor(5000, 10000);
console.log('2. Returned function: ' + indexColor);

console.log('4. Output from production function: ' + indexColor(5000));

score -1 · Answer 2 · answered Mar 09 '18 at 15:20

I was able to get 'Green' to print out when I used this switch-case trick found at: Switch on ranges of integers in JavaScript

Basically, set the switch to test against to a boolean 'true' to compare ranges in a case.

    function changeColorConstructor(greenLimit, redLimit) {
        var greenLimit = greenLimit;
        var redLimit = redLimit;
        console.log('1. Green Limit in Constructor shell: ' + greenLimit);
        
        return function(metric) {
          var color;
    
          console.log('3. Green Limit in returned function: ' + greenLimit);
    
          switch(true) {
            case metric >= greenLimit:
              console.log('Green');
              color = '#56C621'; // green
              break;
            case metric < greenLimit && metric > redLimit:
              console.log('Yellow');
              color = '#F2E607'; // yellow
              break;
            case metric <= redLimit:
              console.log('Red');
              color = '#C64C20'; // red
              break;
            default:
              color = '#C0C0C0'
              break;
          }
          return color;
        }
    
      }
    
      var indexColor = changeColorConstructor(5000, 10000);
      console.log('2. Returned function: ' + indexColor);
    
      console.log('4. Output from production function: ' + indexColor(5000));

The problem with this solution is that it's brittle, and allows people to *very* easily think they could re-order the cases, change values without regard to execution order, etc. This is not a good use for a `switch` as discussed in several comments at the linked question. — Dave Newton, Mar 09 '18 at 15:29
@DaveNewton I agree -- it was neat to see a work around to the range challenge in switch-cases, but I like melpomene 's solution by converting it to a series of if-else statements. — Doug, Mar 09 '18 at 16:18
you are right, in this case the solution with an if/else is better than hacking the switch statement. — Juri, Mar 12 '18 at 13:06

Struggling with function factory & closure

2 Answers2