Skip to main content
Code Review

Return to Answer

Remove invalid curly.
Source Link
Brythan
Brythan
  • 7k
  • 3
  • 21
  • 37
    ArrayList<Long> sequence = new ArrayList<Long>() {

As a general rule, you want to put the interface on the left side, not the implementation. This makes it easier to change the code later with a different implementation of the same interface.

    public static List<Long> sequence = new ArrayList<>() {;

Also, newer versions of Java do not require you to specify the type in the <> on the right side. The compiler is smart enough to figure it out.

I would also have made sequence a class variable. In fact, it might make sense to have a FibonacciSequence class that is separate from the Euler project altogether. That would allow you to create a more general implementation that you can reuse for other problems. This would also simplify main which has more of the programming logic than necessary.

public static void main(String[] args) {
    final long START = System.nanoTime();

    System.out.print("Result: " + sumStoredSequence(4000000L, 1, 3) 
        + ".\nTime used for calculation in nanoseconds: " 
        + (System.nanoTime() - START) + "."
    );
}

This puts all the problem logic in the function, leaving just display and timing logic in main. All main knows is that the maximum value is four million, the starting index is 1, and the update interval is 3.

Note: I'm not arguing against the optimizations in other answers. My point is that there are improvements that can be made in the current implementation, aside from the algorithmic ones.

    ArrayList<Long> sequence = new ArrayList<Long>() {

As a general rule, you want to put the interface on the left side, not the implementation. This makes it easier to change the code later with a different implementation of the same interface.

    public static List<Long> sequence = new ArrayList<>() {

Also, newer versions of Java do not require you to specify the type in the <> on the right side. The compiler is smart enough to figure it out.

I would also have made sequence a class variable. In fact, it might make sense to have a FibonacciSequence class that is separate from the Euler project altogether. That would allow you to create a more general implementation that you can reuse for other problems. This would also simplify main which has more of the programming logic than necessary.

public static void main(String[] args) {
    final long START = System.nanoTime();

    System.out.print("Result: " + sumStoredSequence(4000000L, 1, 3) 
        + ".\nTime used for calculation in nanoseconds: " 
        + (System.nanoTime() - START) + "."
    );
}

This puts all the problem logic in the function, leaving just display and timing logic in main. All main knows is that the maximum value is four million, the starting index is 1, and the update interval is 3.

Note: I'm not arguing against the optimizations in other answers. My point is that there are improvements that can be made in the current implementation, aside from the algorithmic ones.

    ArrayList<Long> sequence = new ArrayList<Long>() {

As a general rule, you want to put the interface on the left side, not the implementation. This makes it easier to change the code later with a different implementation of the same interface.

    public static List<Long> sequence = new ArrayList<>();

Also, newer versions of Java do not require you to specify the type in the <> on the right side. The compiler is smart enough to figure it out.

I would also have made sequence a class variable. In fact, it might make sense to have a FibonacciSequence class that is separate from the Euler project altogether. That would allow you to create a more general implementation that you can reuse for other problems. This would also simplify main which has more of the programming logic than necessary.

public static void main(String[] args) {
    final long START = System.nanoTime();

    System.out.print("Result: " + sumStoredSequence(4000000L, 1, 3) 
        + ".\nTime used for calculation in nanoseconds: " 
        + (System.nanoTime() - START) + "."
    );
}

This puts all the problem logic in the function, leaving just display and timing logic in main. All main knows is that the maximum value is four million, the starting index is 1, and the update interval is 3.

Note: I'm not arguing against the optimizations in other answers. My point is that there are improvements that can be made in the current implementation, aside from the algorithmic ones.

Source Link
Brythan
Brythan
  • 7k
  • 3
  • 21
  • 37

    ArrayList<Long> sequence = new ArrayList<Long>() {

As a general rule, you want to put the interface on the left side, not the implementation. This makes it easier to change the code later with a different implementation of the same interface.

    public static List<Long> sequence = new ArrayList<>() {

Also, newer versions of Java do not require you to specify the type in the <> on the right side. The compiler is smart enough to figure it out.

I would also have made sequence a class variable. In fact, it might make sense to have a FibonacciSequence class that is separate from the Euler project altogether. That would allow you to create a more general implementation that you can reuse for other problems. This would also simplify main which has more of the programming logic than necessary.

public static void main(String[] args) {
    final long START = System.nanoTime();

    System.out.print("Result: " + sumStoredSequence(4000000L, 1, 3) 
        + ".\nTime used for calculation in nanoseconds: " 
        + (System.nanoTime() - START) + "."
    );
}

This puts all the problem logic in the function, leaving just display and timing logic in main. All main knows is that the maximum value is four million, the starting index is 1, and the update interval is 3.

Note: I'm not arguing against the optimizations in other answers. My point is that there are improvements that can be made in the current implementation, aside from the algorithmic ones.