1
\$\begingroup\$

Recently I took a challenge on freecodecamp to solve the given problem: "Sum All Odd Fibonacci Numbers"

Here is my code:

<html>
<head></head>
<body>
Sum All Odd Fibonacci Numbers of number <input type='number' id='val' min="0"/>
<button onclick="getOutput()">get Result</button>
<span id="output"></span>
<script>
  function sumFibs(num) {
    if(num < 2) {
      return num;
    }
    var old = 0, now = 1, sum = 0;
    for(var i = 0; i < num; i++) {
      if(now <= num) {
        if(now % 2 !== 0) {
          sum += now; 
        }
        temp = now;
        now = now + old;
        old = temp;
      }
    }
    return sum;
  }
  function getOutput() {
    var val = document.getElementById('val').value,
    output = document.getElementById('output')
    if(isNaN(val)) {
      alert('not a number')
      return
    }
    output.innerHTML = sumFibs(val);
  }
</script>
</body>
</html>

\$\endgroup\$
3
  • \$\begingroup\$ I like it. Short and efficient. \$\endgroup\$
    – user1149
    Commented Oct 29, 2017 at 16:58
  • \$\begingroup\$ thanks @barryCarter, i want to understand how this can be further optimise \$\endgroup\$
    – Praveen Soni
    Commented Oct 31, 2017 at 2:47
  • \$\begingroup\$ I think you've nailed it. I would argue that any time spent making this more efficient would be wasted. \$\endgroup\$
    – user1149
    Commented Oct 31, 2017 at 17:24

1 Answer 1

Reset to default
3
\$\begingroup\$

This code is great! However, I have a few suggestions. I will put the suggestions in this code segment, marked with "CR".

function sumFibs(num) { // CR: improved formatting
  if (num < 2) {
    return num;
  }
  let old = 0; // CR: ES6 variables, split assignments
  let now = 1;
  let sum = 0;
  let temp; // CR: declared temp
  for (let i = 0; i < num; i++) {
    if (now <= num) {
      if (now % 2 !== 0) {
        sum += now; 
      }
      temp = now;
      now += old; // CR: augmented assignment
      old = temp;
    }
  }
  return sum;
}
  • Great job on solving the challenge with iteration!
  • Many IDEs put a space after for and if
  • Only declaring one variable per statement looks better, and can be less confusing in languages like C
  • let is scoped. If you use var with 2 for loops that declare i, it will be declared twice, but this doesn't happen with let. See this question on Stack Overflow.
\$\endgroup\$
2
  • \$\begingroup\$ Thank you for supplying an answer to this question that has been unanswered for some time. Could you please edit to expand upon what is great about the code? Also, in what ways does it have “improved spacing”- spaces after keywords? And for the sake of the OP and others can you explain the benefits of es6 variables? \$\endgroup\$
    – Sᴀᴍ Onᴇᴌᴀ ♦
    Commented Jul 30, 2022 at 4:01
  • 1
    \$\begingroup\$ @SᴀᴍOnᴇᴌᴀ Thanks for the advice! I'm new to code review, so I didn't really understand how it works. I'll edit the question. \$\endgroup\$
    – Eric Xue
    Commented Jul 30, 2022 at 18:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.