3
\$\begingroup\$

I have written an algorithm that generates a Fibonacci series to the \$Nth\$ number. The code below works fine, but as a beginner I know I must be writing pretty ugly code.

For example I think it is probably not good practice to declare and initialize the variables where I have, but I am confused about what would be the best place.

All comments and advice on code style etc. are welcome.

package algorithms;

import java.util.Scanner;

/**
* This class will generate a list of fibonacci numbers
* @author Richard
*
*/
public class FibonacciGenerator {

/**
 * This method will generate a list of fibonacci numbers
 * @param args
 */
public static void main(String[] args) {
    //declare vars and set initial values
    int number = 0, previousNumber = 0, twoNumbersAgo = 1;
    Scanner input = new Scanner(System.in);

    //prompt user for nth number
    System.out.println("Enter the n'th number for your Fibonacci series: ");

    //set loop counter equal to given nth number
    int n = input.nextInt();

    //loop through as many times as n
    for (int counter=1;counter<=n;counter++){

        //setting each new number in list equal to the sum of the previous two
        number = (previousNumber) + (twoNumbersAgo);

        //print out number
        System.out.print(number+" ");

        //change vars to be next two numbers
        twoNumbersAgo = previousNumber;
        previousNumber = number;    
    }
    //close resources
    input.close();
}
}
\$\endgroup\$
1
\$\begingroup\$

Advice 1

One very useful principle is that each method should solve only one problem and not more. In this context, I suggest that you roll a static method that simply computes a desired Fibonacci sequence and returns it as an array/list. Then, another method can call the previous and output the numbers.

Advice 2

Instead of closing a Scanner explicitly, you could use a try with resources. Any Autocloseable can do and will be closed implicitly.

try(Scanner scanner = new Scanner(System.in)) {
    // Use scanner here.
}
// Scanner automatically closed here.

The above will close Scanner when exiting the block, or in the case of exception.

Advice 3

Fibonacci sequence grows exponentially. For that reason you might want to use BigInteger.

Summa summarum

An alternative implementation may look like this:

import java.math.BigInteger;
import java.util.Arrays;
import java.util.Scanner;

public class FibonacciGenerator {

    public static BigInteger[] getFibonacciSequencePrefix(int n) {
        BigInteger[] result = new BigInteger[n];

        BigInteger a = BigInteger.ONE;
        BigInteger b = BigInteger.ONE;

        for (int i = 0; i < n; ++i) {
            result[i] = a;
            BigInteger tmp = a;
            a = b;
            b = b.add(tmp);
        }

        return result;
    }

    public static void main(String[] args) {
        try (Scanner scanner = new Scanner(System.in)) {
            int n = scanner.nextInt();
            System.out.println(Arrays.toString(getFibonacciSequencePrefix(n)));
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Just note that the original program prints the first N Fibonacci numbers, it does not just print the N'th number. – Judging from the first sentence in the question, that seems to be intentional. \$\endgroup\$ – Martin R Oct 22 '17 at 15:41
  • \$\begingroup\$ @MartinR Oops, you are right. Edited my answer. \$\endgroup\$ – coderodde Oct 22 '17 at 16:13
  • \$\begingroup\$ I didn’t get the try part. Can you elaborate on it a little? \$\endgroup\$ – piepi Oct 22 '17 at 22:32
  • \$\begingroup\$ @piepi Done! If there is more content I could add, let me know! \$\endgroup\$ – coderodde Oct 23 '17 at 11:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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