5
\$\begingroup\$

I have a background in Linguistics and my journey as an aspiring developer has just begun.

I was hoping you could give me a feedback on the solution I have come up with for the second challenge of Project Euler, and maybe explain me why many of the other solutions I have come across were wrapped inside a single function.

Each new term in the Fibonacci sequence is generated by adding the previous two terms. By starting with 1 and 2, the first 10 terms will be:

1, 2, 3, 5, 8, 13, 21, 34, 55, 89, ...

By considering the terms in the Fibonacci sequence whose values do not exceed four million, find the sum of the even-valued terms.

let fibSequence = [0, 1];
let result = 0;

function fibonacci(array) {
  return array[array.length - 1] + array[array.length - 2];
}

while(fibSequence[fibSequence.length - 1] < 4e+6) {
  fibSequence.push(fibonacci(fibSequence));
}

fibSequence.forEach(function (number) {
  if(number % 2 === 0) {
    result += number;
  }
});

console.log(result);
\$\endgroup\$
  • 3
    \$\begingroup\$ Welcome to Code Review! I hope you get some great answers. \$\endgroup\$ – Phrancis Jul 8 '18 at 0:19
3
\$\begingroup\$

Welcome to coding! You've put together a solution that works, but as you get more experienced in both programming, and Project Euler you will discover that there's almost always a better way.... (tm)

On the programming side, you need to realize that you don't need to remember previously computed values in the Fibonacci sequence (no need for the array), and in the Project Euler space, you need to realize that every 3rd value is even. Another project euler optimization is that the sequence can be thought to start from 0, 1, ...., not from 1, 2, ..... If you start at 0, 1, ... you get the sequence 0, 1, 1, 2, 3, 5, 8, ..... (this helps because the first value 0 can be treated as being even....)

Finally, it is almost always a good idea to have a single function that represents the task at hand. You have created a global variable result to store your computation, but really that should be encapsulated in a function of its own. This makes the logic obvious, and also reusable.

Putting these two aspects together, it becomes really a case of simplifying the process down to:

function evenFibSum(limit) {
  let current = 0;
  let next = 1;
  let sum = 0;
  while (current < limit) {
    // add in our current even value (started at 0)
    sum += current;
    // advance to the next even fib number
    // we already have next, so use tmp for the value after next
    const tmp = current + next;
    // the new current is the sum of the previous 2 values
    current = next + tmp;
    // the new next is the new current plus the previous value.
    next = tmp + current;
  }
  return sum;
}

With the above code, you can easily have:

console.log(evenFibSum(4e+6));

Note how with the above code, there's no global values, there's no slow array (adding new members to the array is time consuming and takes space), and also note that the function is reusable with multiple different input values.

\$\endgroup\$
  • \$\begingroup\$ Thank you for taking the time to point me in the right direction. I hope you can help me understand one last thing. Now that you mention it, I can see how wrapping the solution into a single function helps make it more clear and reusable, but would it have been inherently wrong not doing this? \$\endgroup\$ – user172963 Jul 8 '18 at 12:44
  • 1
    \$\begingroup\$ Technically, no, it would not be wrong, but, by making it a function it becomes easy to test, easy to call, and easy to reuse. Those are things that you learn, with experience, make a big difference in "quality-of-life" as a programmer. In the long run, it makes your life easier. \$\endgroup\$ – rolfl Jul 8 '18 at 22:43
0
\$\begingroup\$

Your code looks great! A couple suggestions, though:

  1. I would recommend either:
    A. Putting the code you put in the forEach directly in the while loop.
    B. Changing the forEach to a combination of filter and sum.
  2. As it seems you're using modern JS, you should probably use an arrow function instead of an anonymous function (or whatever they're called).
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy