HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaModerate

Project Euler "Even Fibonacci numbers" in Java 8

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
fibonaccinumbersjavaprojecteulereven

Problem

I'm looking for general advice on my code on the following problem:

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.

public class Problem2 extends Problem {
    @Override
    public void run() {
        result = FibonacciGenerator.finiteStream(i -> (i  (i % 2 == 0))
                .mapToInt(i -> i)
                .sum();
    }

    @Override
    public String getName() {
        return "Problem 2";
    }
}


public class FibonacciGenerator extends RestrictedGenerator {
    private int beforePrevious = 0;
    private int previous = 1;

    protected FibonacciGenerator(final Predicate predicate) {
        super(predicate);
    }

    @Override
    public boolean hasNext() {
        return predicate.test(previous);
    }

    @Override
    public Integer next() {
        int result = beforePrevious + previous;
        beforePrevious = previous;
        previous = result;
        return result;
    }

    public static Stream finiteStream(final Predicate predicate) {
        return RestrictedGenerator.toStream(new FibonacciGenerator(predicate), Spliterator.ORDERED);
    }

    public static Stream infiniteStream() {
        return RestrictedGenerator.toStream(new FibonacciGenerator(i -> true), Spliterator.ORDERED);
    }

    public static Stream finiteParallelStream(final Predicate predicate) {
        return RestrictedGenerator.toParallelStream(new FibonacciGenerator(predicate), Spliterator.ORDERED);
    }

    public static Stream infiniteParallelStream() {
        return RestrictedGenerator.toParallelStream(new FibonacciGenerator(i -> true), Spliterator.ORDERED);
    }
}


```
public abstract class RestrictedGenerator implements Iterator {
protected final Predicate predicate;

prot

Solution

So a fair amount of Java8 is new to me, including the Stream API, which is, in essence, why this review is useful to me too. Bear with me...

Readability

You say "my main concern is readability". Now that I have spent some time looking in to what you are doing, what you are using, and reading up on some documentation, the answer is: yes!

The code does look strange, but that is only because the language structures are new. You are using them right, conforming to what few 'standards' there are... and it is fine.

General

RestrictedGenerator

public abstract class RestrictedGenerator implements Iterator {


This class is essentially useless. It is a container for a Predicate, which is accessed through it's protected nature anyway. Also, what about it is 'restricted'? Why does it have that name?

The static methods on it could be anywhere too. They have no business being on this class specifically.

Basically the class is irrelevant, and makes the class structure more complicated than it needs to be. Moving the functionality to FibonacciGenerator is trivial.

Problem

I really like how you have set up a system for running your tests. This is a worthwhile investment. I have taken your system, and extended it quite a lot. I have made the performance tests a bit more structured. Also, I have rearranged the Problem class. This is a compliment for your code... not a criticism... it's good. I just want to use it differently from you. On the other hand, you may like what I have done, so have a look.

I did find some problems... you are doing long-division on the time values:

String.format("%,d", (elapsed / 1_000_000))


As your code gets faster (warmed up), you will want to convert that to a floating-point:

String.format("%.5f", (elapsed / 1_000_000.0))


Functionality

I cannot find anything wrong with the algorithm. It works as advertized, but there are some places where the code is excessive, redundant, or 'artificial'. The RestrictedGenerator was the worst offender.

Performance

Right, Performance is where I really get interested in problems... I have modified your code and run it through some benchmarks, then I have made alternate systems, and benchmarked them too.

The first thing I noticed is that you are reporting the performance time of the first run. It is common knowledge that Java only starts getting 'fast' when it is 'warmed up'. You want to know what sort of difference it makes? Well, you say your code runs in 56ms (your benchmark code), but, I say it runs in 0.00100ms. Yeah, the numbers are really small. The problem is not particularly challenging.

But, I thought, "I always complain about people using autoboxing instead of primitives... surely the Streams API has primitive processing options?", and, when I looked, it does! So, I converted your code to use IntStream and other Int* constructs, instead of Stream. This has helped with performance. There is an example of that code.

I also thought, what If I write it as I 'learn' Java8, and compare it to what I would have done in Java7.

So, I now have 4 implementations of the problem:

  • your version



  • your version converted to IntStream



  • my version in Java8



  • my version in Java7



Your Version

No reason to do anything here....

Your Version as IntStream

I have removed the code that's not relevant (including the RestrictedGenerator)

Problem2IntStream.java

public final class Problem2IntStream extends Problem {

    public Problem2IntStream() {
        super("Problem 2 - IntStream", 1000, 100);
    }

    @Override
    public Integer execute() {
        return FibonacciIntGenerator.finiteStream(i -> (i  (i % 2 == 0))
                .sum();
    }

}


FibonacciIntGenerator.java

public class FibonacciIntGenerator implements PrimitiveIterator.OfInt {
    private int beforePrevious = 0;
    private int previous = 1;
    private final IntPredicate predicate;

    protected FibonacciIntGenerator(final IntPredicate predicate) {
        this.predicate = predicate;
    }

    @Override
    public boolean hasNext() {
        return predicate.test(previous);
    }

    @Override
    public int nextInt() {
        int result = beforePrevious + previous;
        beforePrevious = previous;
        previous = result;
        return result;
    }

    public static IntStream finiteStream(final IntPredicate predicate) {
    return StreamSupport.intStream(Spliterators.spliteratorUnknownSize(new FibonacciIntGenerator(predicate), Spliterator.ORDERED), false);
    }

    public static IntStream infiniteStream() {
    return StreamSupport.intStream(Spliterators.spliteratorUnknownSize(new FibonacciIntGenerator(i -> true), Spliterator.ORDERED), false);
    }

}


My first Streams code ever

Note, this code works off a Spliterator instead of an Iterator... which, I think, removes a level of abstraction....

```
public final class Problem2Java8 extends Problem {

public Problem2Java8() {
super("Problem

Code Snippets

public abstract class RestrictedGenerator<T> implements Iterator<T> {
String.format("%,d", (elapsed / 1_000_000))
String.format("%.5f", (elapsed / 1_000_000.0))
public final class Problem2IntStream extends Problem<Integer> {

    public Problem2IntStream() {
        super("Problem 2 - IntStream", 1000, 100);
    }

    @Override
    public Integer execute() {
        return FibonacciIntGenerator.finiteStream(i -> (i <= 4_000_000))
                .filter(i -> (i % 2 == 0))
                .sum();
    }

}
public class FibonacciIntGenerator implements PrimitiveIterator.OfInt {
    private int beforePrevious = 0;
    private int previous = 1;
    private final IntPredicate predicate;

    protected FibonacciIntGenerator(final IntPredicate predicate) {
        this.predicate = predicate;
    }

    @Override
    public boolean hasNext() {
        return predicate.test(previous);
    }

    @Override
    public int nextInt() {
        int result = beforePrevious + previous;
        beforePrevious = previous;
        previous = result;
        return result;
    }

    public static IntStream finiteStream(final IntPredicate predicate) {
    return StreamSupport.intStream(Spliterators.spliteratorUnknownSize(new FibonacciIntGenerator(predicate), Spliterator.ORDERED), false);
    }

    public static IntStream infiniteStream() {
    return StreamSupport.intStream(Spliterators.spliteratorUnknownSize(new FibonacciIntGenerator(i -> true), Spliterator.ORDERED), false);
    }

}

Context

StackExchange Code Review Q#42473, answer score: 17

Revisions (0)

No revisions yet.