Freecodecamp: Bug: Avoid Mutations and Side Effects Using Functional Programming needs to validate the global variable is used in the incrementer function

[RandellDawson]

While reviewing a PR (#39363) that fixes a separate issue on the Avoid Mutations and Side Effects Using Functional Programming challenge, I noticed the following test does not validate that the value returned from the incrementer function returns a value that is one larger than the fixedValue global variable's value.

  - text: Your <code>incrementer</code> function should return a value that is one larger than the <code>fixedValue</code> value.
    testString: const newValue = incrementer(); assert(newValue === 5);

A user can simply write the following and pass the challenge without even using the fixedValue variable in the code.

var fixedValue = 4;

function incrementer () {
  // Only change code below this line
   return 5;
  // Only change code above this line
}

I suggest adding a third test, so that we set the value of fixedValue to a different value before calling the function. However, since the user could technically write const fixedValue = 4; instead of var fixedValue = 4;, one way of approaching this would be to use an IIFE: like the following:

  - text: Your <code>incrementer</code> function return a value based on the global `fixedValue` variable value.
    testString: |
      (function() {
       const fixedValue = 10;
       const newValue = incrementer();
       assert(fixedValue === 10 && newValue === 11);
      })()

Before adding a help wanted label, I am looking for feedback from others about this approach to fixing the challenge.

[nhcarrigan]

I think this is a good fix. The only other alternative I could think of would be a regex to test that the incrementer function references the fixedValue variable, but this method would catch more edge cases.

[RandellDawson]

The only other alternative I could think of would be a regex to test that the incrementer function references the fixedValue variable,

When we can avoid using regex, we avoid regex.

[nhcarrigan]

I am 100% on board with avoiding regex. 🙂

I can go ahead and prepare a fix for this if you'd like, after the other PR gets merged or closed.

[RandellDawson]

I would like to open this one up for first timers, since we already have a "fix". Let's see if anyone else comments about the suggestion before adding the label though.

[Sky020]

This looks like a decent approach. However, would it not suffice to edit the first test to this, instead of creating a new test?

[RandellDawson]

This looks like a decent approach. However, would it not suffice to edit the first test to this, instead of creating a new test?

@Sky020 They are testing similar but different things. The first one is testing if they changed the global variable and the third test is indirectly checking that but also validating they did not just use return 5 in the incrementer function.

[Sky020]

@RandellDawson , true, but do we care if they alter the variable (manually)? As in, can we make do with two tests total, because your suggestion already checks to see if the variable has been manipulated by the function?

[RandellDawson]

I just want it to be clear to the user why the test is not passing. Is it because they changed the global variable or that they did not use the global variable in the value returned? I could be wrong, but I think adding the 3rd test makes it clearer what has happened to the user.

[RandellDawson]

So we are in agreement about adding the 3rd test? If so, I will add the first timer label to it. I am also fine waiting for others to reply.

[Sky020]

So we are in agreement about adding the 3rd test?

Yes, I agree that the clarity will be beneficial.

[Mayank1795]

@RandellDawson @Sky020 The call to incrementor function inside the IIFE does not refer to the fixedValue variable created inside IIFE. That's why the assertion will give an error.

[RandellDawson]

@Mayank1795 I should have tested the code before suggesting it. The problem is closure. The original fixedValue variable is declared in the same scope that the incrementer function is. This means when fixedValue is referenced inside it, the function will always use the globally declared fixedValue. We will have to come up with another way to do this.

[Mayank1795]

@RandellDawson We can do something like this. Change the global variable to some other value for testing and later assign the original value. I will update the PR.

```var fixedValue = 4;

function incrementer (){
return fixedValue+1;
}

(function() {
fixedValue = 10;
const newValue = incrementer();
console.log(fixedValue, newValue); // prints 10, 11
fixedValue = 4;
})();

console.log(fixedValue); // prints 4
```

[arrohit258]

Is this issue still open? I will like to work on it.

[nhcarrigan]

Heya @Rohit-Arora-0508

Thank you for your interest in contributing! There is currently an active PR that will close this issue, and it looks like that PR is on track for approval. We typically accept the first comprehensive PR that fixes an issue - as such, I would recommend looking for other issues labelled help wanted (or first timers only, if this is your first contribution) and check for ones that do not yet have a linked PR. 🙂

[arrohit258]

Ok Thanks 🙂🙂

On Tue, Sep 1, 2020, 20:32 Nicholas Carrigan [e-mail]
wrote:

Heya @Rohit-Arora-0508 https://github.com/Rohit-Arora-0508

Thank you for your interest in contributing! There is currently an active
PR that will close this issue, and it looks like that PR is on track for
approval. We typically accept the first comprehensive PR that fixes an
issue - as such, I would recommend looking for other issues labelled help
wanted (or first timers only, if this is your first contribution) and
check for ones that do not yet have a linked PR. 🙂

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/freeCodeCamp/freeCodeCamp/issues/39364#issuecomment-684920379,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/APCTK7AX2LI6NCXRCQORZWLSDUEJVANCNFSM4PTZPOGQ
.