Global variable conflicts with algorithm.

[krishnakumar360]

Challenge Factorialize a Number has an issue.
User Agent is: Mozilla/5.0 (Windows NT 5.1; rv:47.0) Gecko/20100101 Firefox/47.0.
Please describe how to reproduce this issue, and include links to screenshots if possible.

My code:

var count=1;
function factorialize(num) {
  for(var i=1; i<=num;i++){

    count= count * i;

  }
  return count;

}

factorialize(0);

[raisedadead]

@krishnakumar360 : Thanks for reporting this.

Your code is correct but using the var count=1 in the global scope is causing the test cases to fail.
Moving it to the function's scope should let you pass the challenge.

function factorialize(num) {
  var count = 1;
  for(var i=1; i<=num; i++) {
   count = count*i;
...

The reason being that we run multiple test cases in on the code, using a global var like this persists the last value and hence the test cases fail.

Nevertheless this is valid code and we should be addressing this.

[BKinahan]

@raisedadead Is it truly valid code? Functions should be reusable in many cases, and users should learn about side effects caused by operating on global variables.

[kirushna-kumar]

@raisedadead I concur with @BKinahan. Maybe we can have two calls to factorialize() in the challenge to explicitly portray the effects of using global variables in functions, if used.

[BKinahan]

Note that almost all of the algorithm challenges do not work if global variables are used, the only exception that comes to mind being Counting Cards which sets up a global variable for the user to illustrate a case when it is useful to track a value between function calls (and there it is reset between sets of function calls in each test).

Any changes to be made here regarding global variables or how the user should be made aware of them as @flipsyde606 suggests would need to justify a change to most of the other similar challenges.

[raisedadead]

Maybe we can have two calls to factorialize() in the challenge to explicitly portray the effects of using global variables in functions, if used.

@flipsyde606 @BKinahan thanks a lot for the inputs.

While you guys are correct, this challenge may not be the correct place to illustrate that thought, and as @BKinahan has already noted, this being true for almost all challenges we would have to add a case to all challenges which doesn't make sense.

So, we should be handling the global variable declarations in the code runner (just like we check for loops) and possibly throw an error on the console? (with the exceptions of course for challenges that truly require global vars)

/cc @FreeCodeCamp/issue-moderators

[Bouncey]

Do we have a way forward with this?

[BKinahan]

I don't think there's an easy solution to this.

It doesn't seem feasible to detect a global variable declaration and throw an error as @raisedadead suggested since valid code such as this would cause a false positive:

var count;
function factorialize(num) {
  count = 1;
  for(var i=1; i<=num;i++){
    count= count * i;
  }
  return count;
}

(As long as the variable is assigned an initial value with the function before use as an operand, it works correctly regardless of global declaration, with or without initialization. I can't think of a way to detect that.)

The best we can do in that regard is probably to ensure that there is sufficient reinforcement of the concepts of scope and side-effects early in the algorithm section of the course, and continue not passing code that is not reusable due to improperly handled variables.

In terms of something practical that can be applied across all the challenges, it may be worthwhile to consider changes to what output users see when running tests.

Currently a user who includes a function call at the end of the code editor will see the output for that call. For example, factorialize(5) will correctly output 120 and display it, and yet the test for the input 5 will be marked as incorrect because of the other calls before it. But the user doesn't see these calls or their effects, so they can only conclude that there is a problem with the tests, especially as they try the other inputs and confirm that their code works correctly. (Notably, if the user does not include a function call in the editor, the output varies depending on any other code in the editor, eg blank for the example above or 1 if var count; is replaced bycount = 1; ).

So the only way a user with a problematic global variable in their code can stumble upon the source of the problem is by including multiple function calls in their editor (a similar process to what @flipsyde606 mentions above) and seeing the incorrect output.

All of that is to say, it may be useful to alter the output display to include input-output pairs to illustrate what results the tests are actually giving when they are run as a full set, for example:
(with correct code):

factorialize(5): 120
factorialize(10): 3628800
factorialize(20): 2432902008176640000
factorialize(0): 1

(and with a problematic global variable):

factorialize(5): 120
factorialize(10): 435456000
factorialize(20): 1.0594217768725671e+27
factorialize(0): 1.0594217768725671e+27

(though these may be off by a factor of 120; I believe there's another call to factorialize(5) to make sure it's a number, which in itself may be a reason to run that test after all the others...)

A user would immediately see that these numbers are way off, even if they don't immediately connect the dots about the cause, and will probably (hopefully) ask about their code in a help chat rather than assuming a bug in the challenge and reporting an issue.

There are problems with this approach as a general solution. Some challenges have a large number of tests, or long outputs, which would not be easily displayed in the output section. Still, it may be a useful addition to certain challenge tails, particularly if they have a recurring confusion about global variable use and a few custom output lines could illustrate the problem clearly.

Hopefully this was readable and useful 😄

[raisedadead]

@BKinahan thanks again for looking into this, and whoa that was a huge post. However I think we can try a hack for this, I'll have to double check and get back on this but to the best of my knowledge the final code that is asserted for each test case runs within an eval method.

So it is possible (theoretically speaking) to append //# sourceURL=functionNameXX where XX is an incremental no added to the function name in discussion, at the top of the seed code.

This will enforce a challenge execution in separate execution context for each eval (chrome for instance will do this in separate VMs)

Maybe hackish but should do the trick.

[raisedadead]

And if this doesn't then we can wrap the entire seed code within a named container functions.

[Locheed]

Just spend some time figuring out why my test fails and it was this problem with a global variable.

Just a thought, how about adding a note to descriptions, that "Using global variables in this test will cause the test to fail." I think it would help in this kinda situation, but doesn't give too many tips for an exercise.

[wjhurley]

Should this issue be closed?

[raisedadead]

Should this issue be closed?

Nope we need a way to detect global variables, stop the execution of tests and show a warning to users why the code can't be run.

or

Execute the tests in some isolated mechanism , that avoids this conflict.

[systimotic]

Hi all! It looks like this issue is a duplicate of #9063, so I'll close this in favour of that. If I got this wrong, please reopen!