freeCodeCamp / wiki

freeCodeCamp's deprecated wiki articles
http://www.freecodecamp.com/wiki
281 stars 308 forks source link

Adds 'advanced' algorithm solution to challenge #1218

Closed ortonomy closed 8 years ago

ortonomy commented 8 years ago

closes #1217

alayek commented 8 years ago

This solution is correct, but I have a concern about this:

This uses splice(), which mutates the array. Side effects lead to bugs. Especially when you are calling a reduce on the very same array, and mutating the array on the fly.

Please use slice() instead of splice().

ortonomy commented 8 years ago

@alayek - this is an interesting point, but using splice was entirely the point. I want the 'used' numbers to be eliminated from the array without having to do complicated checks or use an array to keep track of the 'used' indices. I did intentionally mean to mutate the array because I was using reduce. I don't believe this to be a side effect, in this narrow case.

Secondarily to this point the first "basic" solution also modifies the source array on the fly, as evidenced by this code section:

for(i = 0; i < arr.length; i++) {
   //Looping from second element by setting first element  constant
   for(j = i + 1; j < arr.length; j++) {
     // Check whether the sum is equal to arg
     if(arr[i] + arr[j] == arg) {
       //Add the indices
       sum += i + j;
       //Set the indices to NaN so that they can't be used in next iteration
       arr[i] = arr[j] = NaN; // here you mutate the array in the very same way!
     }
   }
 }

arr[i] = arr[j] = NaN; // here you mutate the array in the very same way! If I remove it from this solution, then it should be removed from the basic code solution too?

alayek commented 8 years ago

Yes, please update your PR with that change in both those solutions. What I would recommend is creating a new array from the array arr passed as argument; and modify it as you see fit.

ortonomy commented 8 years ago

@alayek - thanks for the feedback. To confirm, I will not change the algorithm for the advanced solution, but changes to be made are:

  1. Put a copy of 'arr' arguments object into a new local var before manipulating it, in the existing basic solution
  2. Put a copy of 'arr' arguments object into a new local var before manipulating it, in the new advanced solution.
ortonomy commented 8 years ago

@alayek - made a new commit, pls chk. Do I need to make a new pull request?

ortonomy commented 8 years ago

@alayek - implemented local var using arr.slice() to make a copy. arr.from() is ES6, so won't use that here.

ortonomy commented 8 years ago

please squash the commits too.

alayek commented 8 years ago

LGTM :+1:

Let's :shipit: :package:

Rafase282 commented 8 years ago

Lets wait for the javascript declaration on the code first.

ortonomy commented 8 years ago

@Rafase282 - sorry, what does 'javascript declaration' mean?

Rafase282 commented 8 years ago

basically the comment I did on the code, do

```javascript

instead of just ```

ortonomy commented 8 years ago

@Rafase282 - sorry. Didn't see the comment. Adding.

ortonomy commented 8 years ago

@Rafase282 - declaration added.

ortonomy commented 8 years ago

First PR complete. 🎉

Rafase282 commented 8 years ago

I found something the run code at the bottom is an extra. I'll remove it. Thanks for the solution.

Thank you for your contribution!

ortonomy commented 8 years ago

Doh! Should have checked more. Hoping to make more contributions in the future!

ortonomy commented 8 years ago

@Rafase282 @alayek - out of curiosity, when is the code released to live? Do you have a schedule?

Rafase282 commented 8 years ago

It is already, just not on the wiki from the nav bar yet. it is even on the forum.