views:

442

answers:

10

Being new to this I really am trying to learn how to keep code as simple as possible, whilst doing the job it's supposed to.

The question I have done is from Project Euler, it says

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, ...

Find the sum of all the even-valued terms in the sequence which do not exceed four million.

Here is my code below. I was wondering what the best way of simplifying this would be, for a start removing all of the .get(list.length()-1 )..... stuff would be a good start if possible but I don't really know how to?

Thanks

public long fibb()
{
    ArrayList<Integer> list = new ArrayList<Integer>();


    list.add(1);
    list.add(2);

    while((list.get(list.size() - 1) +  (list.get(list.size() - 2)) < 4000000)){  
        list.add((list.get(list.size()-1)) + (list.get(list.size() - 2)));
    }     

    long value = 0;
    for(int i = 0; i < list.size(); i++){
        if(list.get(i) % 2 == 0){
            value += list.get(i);
        }    
    }
    return value;
}
+10  A: 

You don't need a list, you only need two last items. Here is some pseudocode, I leave translating it to your language to you.

f0=1 #pre-last number
f1=1 #last number
while(...) {
    t = f0 + f1
    if (t%2 == 0) # replaces your second loop 
         sum += t 
    f0 = f1
    f1 = t
}

Next, you can observe that the numbers are always in sequence:

odd, odd, even, odd, odd, even [...]

and optimize further, if you need to

unbeli
+1 for such a clearly written answer!
glowcoder
the answer is absolutely not clearly written... never ever use a variable that is only one char long...
Janusz
Like 'i' as frequently used in for loops? Better call it 'loop_index_increment_counter' then.
Chris Dennett
@Janusz always follow the rules you read, blindly ;)
unbeli
@Janusz: I would hate to see your code that deals with arrays. If `for (int i = 0; i < ...` isn't a self-documenting and globally standard idiom, I don't know what is.
Mark Peters
i would make an exception for i sometimes but at some times you can use something that shows what you are counting like currentShop. With a good ide you have code completion and the person reading it especially in listed loops has it much easier.
Janusz
A: 

My advice would be this (I'm not going to provide an answer as it's always best to come to the conclusions yourself)

Try to think of why you need to store the values of the sequence? Your program is going to store 4000000 integers in a big array, is there really a need for this? You could just use 2 items and zip through (using the Fibonacci calculation) adding the even numbers to a Total variable.

djhworld
the array size only ends up at about 34 in length
simion
The point is storing all the members are not necessary, because of the nature of the Fibonacci series.
futureelite7
My apologies for the confusion, I misread your code - but futureelite7 is right - my point was to suggest not storing the results in an array. Imagine if you wanted to generate 1 million Fibonacci numbers and think about how much memory that would take up in your program!
djhworld
+3  A: 

You can get rid of the List altogether. Just keep the last two fibonacci numbers in two variables and calculate the next one in a loop (similar to your first loop).

Then keep a running total of all numbers that match the condition (i.e. the are even and less than million) in the same loop.

Joachim Sauer
+1 for getting rid of the second loop.
Chris Thornton
+3  A: 

First of all, before attempting to rewrite any code, it's useful to have a unit test. Even the most obvious change can break something unexpected.

Second, it's important to be very careful. You'll often see in code that X calls to the same functions can be replaced by one call, assignment to variable, and use of that variable.

However, you have to be careful doing that. For instance, in your current code, you cannot really replace list.size() because the size changes all the time between calls in the same iteration.

What mostly makes your code difficult to understand is that you refer to list indices while you're updating the lists. You need to save indices to variables, split into multiple lines, and perhaps add some comments to make it more readable.

Uri
Who on Earth downvoted this answer, and why?
Quick Joe Smith
Downvote countered. I have no idea why it was downvoted (twice!). Even the unit test, while frightening to a learner, is a great idea and a Fibonacci generator is a prime candidate for one.
Mark Peters
A: 

You would do a great deal of simplifying if you don't safe all the values in a list. You could check if they are even "at the run"

for example like that:

int old, current;
old = 1;
current = 1;

long value = 0;

while(current < 4000000) {
 if(current % 2  == 0) value += current;

 int tmp = old + current;
 old = current;
 current = tmp;
}

return value;
lerad
A: 

Well, for one thing, a member of the Fibonacci sequence is generated by the two previous values before it. Why not just keep two numbers in the buffer, and test if the number is even. If it is even, add it to your sum and stop when you reach four million.

code:

public int fibEvenSum(int a, int b) {

int sum = 0;
int c = a + b;

while (c <= 4000000) {

if (c % 2 == 0) {
sum += c;
}

//Shift the sequence by 1
a = b;
b = c;

c = a + b;

} //repeat

return sum;
}
futureelite7
+29  A: 

The other responders have all given great answers. I want to show you how refactoring works in action, not just for this specific problem knowing things about Fibonacci numbers, but as an iterative process that carefully whittles down code to the bare minimum. Refactoring lets us start with working but complicated code and steadily pare it down one step at a time. Let me show you all of the in between steps you could make while working your way towards the final solution.

Note: I've changed your initial starting values to 1 and 1 instead of 1 and 2. Strictly speaking, the Fibonacci sequence begins with two 1's, as in 1, 1, 2, 3, 5...

Step 1 - Reverse the list

For starters, to get rid of the repetitive list.size() - x expressions you could add the numbers in reverse order. Then finding the two most recent numbers is simpler.

public long fibb()
{
    ArrayList<Integer> list = new ArrayList<Integer>();

    list.add(1);
    list.add(1);

    while (list.get(0) + list.get(1) < 4000000) {
        // Use list.add(0, ...) to add entries to the *front*.
        list.add(0, list.get(0) + list.get(1));
    }     

    long value = 0;

    for (int i = 0; i < list.size(); i++) {
        if (list.get(i) % 2 == 0) {
            value += list.get(i);
        }    
    }

    return value;
}

Step 2 - Switch to a linked list

Let's switch the ArrayList to a LinkedList. Inserting at the beginning of an array is inefficient, whereas its a quick operation on a linked list.

Along those lines, we'll need to get rid of the get() calls in the second loop. Looking up entries by index is slow using linked lists. To do this I've changed the second loop to use for (variable: container) syntax.

public long fibb()
{
    // Changed to use a linked list for faster insertions.
    List<Integer> list = new LinkedList<Integer>();

    list.add(1);
    list.add(1);

    // Using get() is normally a bad idea on linked lists, but we can get away
    // with get(0) and get(1) since those indexes are small.
    while (list.get(0) + list.get(1) < 4000000) {
        list.add(0, list.get(0) + list.get(1));
    }     

    long value = 0;

    // Altered loop to avoid expensive get(i) calls.
    for (Integer n: list) {
        if (n % 2 == 0) {
            value += n;
        }    
    }

    return value;
}

Step 3 - Combine the loops

The next optimization is to combine the two loops. Instead of generating all of the numbers first and then checking for even ones later, you could check for even numbers as you generate them.

public long fibb()
{
    List<Integer> list = new LinkedList<Integer>();
    long value = 0;

    list.add(1);
    list.add(1);

    while (list.get(0) + list.get(1) < 4000000) {
        int next = list.get(0) + list.get(1);

        list.add(0, next);

        if (next % 2 == 0) {
            value += next;
        }    
    }     

    return value;
}

Step 4 - Eliminate the list

Now you might notice that you never refer to numbers beyond index 1. Numbers at positions 2 and beyond are never used again. This hints that you don't even need to keep a list of all the numbers any more. Since you are checking for the even numbers as they are generated, you can now throw away all but the two most recent numbers.

Also, as a minor detail, let's rename value to total.

public long fibb()
{
    int a = 1, b = 1;
    long total = 0;

    while (a + b < 4000000) {
        // Calculate the next number.
        int c = a + b;

        // Check if it's even.
        if (c % 2 == 0) {
            total += c;
        }

        // Shift the values.
        a = b;
        b = c;
    }     

    return total;
}
John Kugelman
this is not refactoring, by definition of refactoring ;)
unbeli
Even though it looks cleaner, reversing the list is a horrible idea. The performance of an array list degrades immensely when you do all your adding to the start of the list. At least correct it to use a linked list if you're going to suggest that.
Mark Peters
+1 for comprehensive answer
djhworld
+1; more if I could. This is a great demonstration of how to go about producing cleaner code. Yes, @Mark Peters, reversing the list may be inefficient - but look what happened at the end! It went away. It's OK to make your code a little worse on the way to being better. It's all in the follow through.
Carl Manaster
now thats what i call sexy code! (do i need to get out more?!) I think i really need to practise the process of refactoring my code tbh, thats for a great tutorial. I want to mark this as correct, but the other one has more votes. I hope that isn't frowned upon?
simion
@Mark - Excellent point. I've added a step to switch to a linked list. It's not strictly necessary since eventually the list is removed entirely, but the point here is the process not the end result, anyways.
John Kugelman
@Carl: You're right if you consider the whole set of changes as an atomic change. But I read them as trying to say that each is incrementally better than the last. So I just wanted to relay that, if you were not going to make it to Step 3/4, danger lay with staying at Step 1. @John: You've got my +1 now. Great post!
Mark Peters
+1; fantastically clear demonstration of incremental code improvement.
eggsyntax
+1  A: 

I would drop the list. You're doing two iterations here when all you really need to do is iterate over the sequence and keep a tally variable as you go.

  • So, iterate over the Fibonacci sequence using some sort of for/while loop.
  • Add it to the tally variable if it is even.

To do a simple iteration over the sequence, you only need to record the two most recent values (f(n) and f(n-1)) and (depending on your language) a temporary variable for when you update those values. Usually something along the lines of:

temp = current;
current = current + previous; // temp holds old value of current so the next line works.
previous = temp;

The simples approach is a basic for loop. Or you could roll your own class that implements the Iterator<E> interface if you wanted to be all OO about it. Then your code could be as simple as:

Fib fib = new Fib();
for(long n : fib) {
    if(n % 2 == 0) t += n;
}

That reminds me, I must get back into Euler again some day.

Quick Joe Smith
+1  A: 

Everything looks prettier in Python!

def fibb():
    ret = 2 # to take into account the first 2
    fib = [1, 2]

    while True:
        latest = fib[-1] + fib[-2]
        if latest >= 4000000: return ret

        fib.append(latest)
        if latest % 2 == 0: ret += latest

Note: This was more a programming exercise than anything else. I realise it's probably not practical to move off to Python.

Edit, here's the more memory efficient, no-list approach:

def fibb():
    ret = 0
    f0, f1 = (1, 1)

    while True:
        f0, f1 = (f1, f0 + f1)
        if f1 >= 4000000: return ret
        if f1 % 2 == 0: ret += f1
Oli
A: 

Like @John Kugelman's solution, but more efficient. This will loop only 1/3 of the times and each loop is shorter with less branches and no even test required. This takes advantage of the fact that every third fib value is even and just calculates the values in between to reset the a and b value.

public static long fibb() {
    int a = 1, b = 1;
    long total = 0;
    while (true) {
        int c = a + b;
        if (c >= 4000000) return total;
        total += c;
        a = b + c;
        b = c + a;
    }
}
Peter Lawrey