current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 8 down vote favorite
1

I'm attempting to find a fibonacci series where the last number is no greater than a given maximum. Given the start digits 1, 2

Here's my working code.

var fib = [1,2];
var max = 4000000;
var i = 2;
var loop = true

while (loop) {
    if (fib[i-1] + fib[i-2] > max) {
        loop = false;
    } else {
        fib.push(fib[i-1] + fib[i-2]);
        i++;
    }
}

console.log(fib)
share|improve this question
up vote 9 down vote accepted

Your loop can be terminated in a few different ways. You're using a boolean loop variable, but there are cleaner ways.

One is to simply move the if condition to the while condition (Malachi's answer beat me to this):

while (fib[i-1] + fib[i-2] <= max) {
  i = fib.push(fib[i-1] + fib[i-2]);
}

push helpfully returns the array's new length, so you don't need the i++.

Another is to keep the if branching, but use it to break the loop instead:

while (true) {
  var next = fib[i-1] + fib[i-2];
  if (next > max) {
    break; // exit the loop
  }
  i = fib.push(next);
}

Here we avoid calculating the new fibonacci number twice, but still: the previous code block is probably still the cleanest.

You also initialize i to 2, which is a bit redundant: It's the length of the fib array. Rather than manually initializing the array, and manually recording its length, I'd just do var i = fib.length.

Lastly, it might be good to consider "bad" max values. I.e. what if the maximum is set to zero? Or 1? Or -2324359? For zero you could return an empty array; for 1 you'd need to return a shorter array than what you start with, and for the negative max you could throw an error or return null. Not saying you need to any of this, but it's always to good to consider how you handle such cases.

share|improve this answer
1  
push returns new length - well that's handy thanks. – Luke 17 hours ago
up vote 4 down vote

I think that you can remove the loop variable by moving that equation into the while conditional statement, like this:

var fib = [1,2];
var max = 4000000;
var i = 2;

while (fib[i-1] + fib[i-2] <= max) {
    fib.push(fib[i-1] + fib[i-2]);
    i++;
}

console.log(fib)

I just inverted the condition.

share|improve this answer
1  
Upvote for the loop simplification, but the other answer use i = push() which I liked and had a slightly clearer explanation. – Luke 16 hours ago
up vote 0 down vote

In the interest of providing an alternate from the while loops already given:

var f = (a, b, c, m) => (b + c >= m) ? a : f(a.concat([b + c]), c, b + c, m);
var fib = f([1, 1], 1, 1, 10);

Less pretty version for golfs sake (43 chars)

a=[];for(b=c=1;c<=10&&a.push(b,c);c+=b+=c)a
share|improve this answer
    
Woops, thought this was the CodeGolf SE :) – John Jones 6 hours ago

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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