patternjavaModerate
Dynamic programming with Fibonacci
Viewed 0 times
withprogrammingfibonaccidynamic
Problem
I have written the following code using a dynamic programming technique. Can I use
ArrayList here? Please let me know if I can improve this code.package com.java.fib;
import java.math.BigInteger;
import java.util.HashMap;
public class Fibonaci {
public static void main(String[] args) {
System.out.println(" number ");
long startTime = System.currentTimeMillis();
HashMap memoized = new HashMap();
fibonanci(220, memoized);
System.out.println(" Total Time "
+ (System.currentTimeMillis() - startTime));
}
private static BigInteger fibonanci(int n, HashMap memoized) {
if (memoized.containsKey(n)) {
return memoized.get(n);
}
if (n <= 0) {
return BigInteger.ZERO;
}
if (n <= 2) {
return BigInteger.ONE;
} else {
BigInteger febonani = fibonanci(n - 1, memoized).add (fibonanci(n - 2, memoized));
System.out.println(" febonani " + febonani);
memoized.put(n, febonani);
return febonani;
}
}
}Solution
A few things:
Naming:
It's neither
Calculating:
quoting wikipedia:
By definition, the first two numbers in the Fibonacci sequence are \$1\$ and \$1\$, or \$0\$ and \$1\$, depending on the chosen starting point of the sequence, and each subsequent number is the sum of the previous two.
You on the other hand say:
The value of Fibonacci for \$n <= 0\$ is \$0\$, and for \$n <= 2\$ is \$1\$, and each subsequent number is the sum of the previous two.
Wikipedia also mentions "negative Fibonacci numbers". Your definition does not match the actual Fibonacci sequence definition :(
instead your fibonacci method should look like this, if you exactly follow the rules for only positive fibonacci numbers:
Printing is slow:
I see you benchmarking your code. Please keep in mind, that writing something to the Console is quite slow. Your measured execution time is not matching the calculation time.
You should remove the
Conditionals
Also you maybe have already realized, I removed the
This is because if your code reaches that area, all other branches have returned early, and the else is quite useless.
Naming:
It's neither
Fibonaci, nor febonani, nor fibonanci it's fibonacci. Please get your names to reflect what you're actually talking about and not some disfigured mutation of it :(memoized OTOH is a relatively nice name, I'd probably prefer memoizedFibonacciNumbers, but that's a thing of preferenceCalculating:
quoting wikipedia:
By definition, the first two numbers in the Fibonacci sequence are \$1\$ and \$1\$, or \$0\$ and \$1\$, depending on the chosen starting point of the sequence, and each subsequent number is the sum of the previous two.
You on the other hand say:
The value of Fibonacci for \$n <= 0\$ is \$0\$, and for \$n <= 2\$ is \$1\$, and each subsequent number is the sum of the previous two.
Wikipedia also mentions "negative Fibonacci numbers". Your definition does not match the actual Fibonacci sequence definition :(
instead your fibonacci method should look like this, if you exactly follow the rules for only positive fibonacci numbers:
private static BigInteger fibonacci(Integer n, HashMap memoized) {
if (n < 0) {
throw new IllegalArgumentException("We assume the positive Fibonacci sequence only");
}
if (memoized.containsKey(n)) {
return memoized.get(n);
}
if (n == 0) {
memoized.put(n, BigInteger.ZERO);
return memoized.get(n);
}
if (n == 1) {
memoized.put(n, BigInteger.ONE);
return memoized.get(n);
}
BigInteger fibonacci = fibonacci(n-1, memoized).add(fibonacci(n-2, memoized));
memoized.put(n, finbonacci);
return fibonacci;
}Printing is slow:
I see you benchmarking your code. Please keep in mind, that writing something to the Console is quite slow. Your measured execution time is not matching the calculation time.
You should remove the
System.out.println() in your fibonacci method.Conditionals
Also you maybe have already realized, I removed the
else in the fibonacci method. This is because if your code reaches that area, all other branches have returned early, and the else is quite useless.
Code Snippets
private static BigInteger fibonacci(Integer n, HashMap<Integer, BigInteger> memoized) {
if (n < 0) {
throw new IllegalArgumentException("We assume the positive Fibonacci sequence only");
}
if (memoized.containsKey(n)) {
return memoized.get(n);
}
if (n == 0) {
memoized.put(n, BigInteger.ZERO);
return memoized.get(n);
}
if (n == 1) {
memoized.put(n, BigInteger.ONE);
return memoized.get(n);
}
BigInteger fibonacci = fibonacci(n-1, memoized).add(fibonacci(n-2, memoized));
memoized.put(n, finbonacci);
return fibonacci;
}Context
StackExchange Code Review Q#57646, answer score: 17
Revisions (0)
No revisions yet.