Beginner radian/degree converter program

Question

I'm a bit of a beginner when it comes to Java, and I created this small code to help me grasp a few small subjects. It allows its user to convert a radical to a degree measure and a degree measure to a radical. This is my final code:

import java.io.BufferedReader; 
import java.io.IOException; 
import java.io.InputStreamReader;
import java.util.ArrayList; 

public class MainClass {

    public static void dtr (BufferedReader dRead) throws IOException {

        ArrayList<Integer> divisors = new ArrayList<Integer>();
        ArrayList<Integer> gcd = new ArrayList<Integer>();


        int d = Integer.parseInt(dRead.readLine());
        for (int pd = 1; pd <= d; pd++) {
            if (d % pd == 0) {
                divisors.add(pd);               
            }
        }

        for (int index = 0; index < divisors.size(); ++index) {
            if (180.0 % divisors.get(index) == 0) {
                gcd.add(divisors.get(index));
            }

        }

        int dem = (180 / gcd.get(gcd.size() - 1));
        if ((d / (gcd.get(gcd.size() - 1))) == 1) {
            System.out.print("Radical: pi / " + dem);
        } else {
            System.out.print("Radical: " + (d / (gcd.get(gcd.size() - 1))) + "pi / " + dem);
        }

    }

    public static void rtd (BufferedReader rnRead, BufferedReader rdRead) throws IOException {

        int rn = Integer.parseInt(rnRead.readLine());
        int rd = Integer.parseInt(rdRead.readLine());

        int dividend = rn * 180;

        System.out.print("Degrees: " + (dividend / rd));

    }

    public static void main(String[] args) throws IOException {

        System.out.println("Do you want to convert from radicals to degrees, or degrees to radicals?");
        System.out.print("Use \"R\" for radials to degrees, and \"D\" for degrees to radicals: ");
        BufferedReader userInput = new BufferedReader(new InputStreamReader(System.in));
        String userChoice = userInput.readLine();

        if (userChoice.contains("D") || userChoice.contains("d")) {

            System.out.print("Degrees: ");
            BufferedReader dRead = new BufferedReader(new InputStreamReader(System.in));
            dtr(dRead);

        } else if (userChoice.contains("R") || userChoice.contains("r")) {

            System.out.println("Radical numerator (omit pi): ");
            BufferedReader rnRead = new BufferedReader(new InputStreamReader(System.in));
            System.out.println("Radical denominator: ");
            BufferedReader rdRead = new BufferedReader(new InputStreamReader(System.in));
            rtd(rnRead, rdRead);

        } else {

            System.out.println("Invalid response. Please restart the program.");

        }

    }

}

I know how bad some of this code is, but I just want to know if there is any way I could have optimized what this does, because I'd like to apply those concepts to everything that I'll make in the future.

Answer 1

Both Martin & David have made good points, and I won't repeat them here.

But David said "It is also common to reuse the Reader and not create a new one every time you need to perform a read operation." This doesn't stress the point enough. It is wrong to keep creating BufferedReader objects on the same input stream. A BufferedReader is designed to reduce the overhead of continuously asking the operating system for more characters from the input stream; it does this by reading "as much as it reasonable can". If you are running your program at the console, and entering responses by hand, this isn't much of a problem; the console will be waiting for you to enter more input. But your teacher may have an automated system for running every students' solution, where the input is provided by redirecting standard input from a file. Eg:

testdata1.txt:

R
2
3

And running each students' solution like: java MainClass < testdata1.txt, so when the first BufferedReader is opened ...

    BufferedReader userInput = new BufferedReader(new InputStreamReader(System.in));
    String userChoice = userInput.readLine();

the BufferedReader could read the entire file (6 to 9 characters, depending on end-of-line), scan for and return the first line "R\n", while leaving "2\n3\n" ready to be returned by the next calls to userInput.readLine(). But instead,

        BufferedReader rnRead = new BufferedReader(new InputStreamReader(System.in));

is created, and when it tries to read from System.in, it will discover that it is at the end-of-file! Oops! rnRead can't return what userInput has already read and has internally cached.

To summarize: It is not just common to reuse the Reader; it is necessary to avoid incorrect operation.

---

While Martin has suggested a much better GCD algorithm, let's concentrate on simply improving your implementation.

In this code, you use gcd.get(gcd.size() - 1) often:

    int dem = (180 / gcd.get(gcd.size() - 1));
    if ((d / (gcd.get(gcd.size() - 1))) == 1) {
        System.out.print("Radical: pi / " + dem);
    } else {
        System.out.print("Radical: " + (d / (gcd.get(gcd.size() - 1))) + "pi / " + dem);
    }

It would be clearer to retrieve that value once, store it in a local variable, and just refer to that local variable:

    int last_gcd = gcd.get(gcd.size() - 1);
    int dem = 180 / last_gcd;
    if (d / last_gcd == 1) {
        System.out.print("Radical: pi / " + dem);
    } else {
        System.out.print("Radical: " + (d / last_gcd) + "pi / " + dem);
    }

But if we are only using the last value of the gcd array, why are we storing the values in an ArrayList? Why not skip the ArrayList, and just remember the last value?

    int last_gcd = 1;
    for (int index = 0; index < divisors.size(); ++index) {
        if (180 % divisors.get(index) == 0) {
            last_gcd = divisors.get(index);
        }
    }

BONUS: This just fixed a bug. If you entered 0 for the number of degrees, the divisors would be empty, and the gcd array would also be empty, so gcd.get(gcd.size() - 1) would blow up with an IndexOutOfBoundsException. Now, last_gcd just stays with the initial value of 1.

I'm not liking the two occurrences of divisors.get(index). We could probably fetch the value once, store it in a local variable (say divisor), then use the local variable in the if(...) test and the assignment. Since looping over an array or Collection is so common, Java has a short-cut syntax for this:

    int last_gcd = 1;
    for (int divisor : divisors) {
        if (180 % divisor == 0) {
            last_gcd = divisor;
        }
    }

But wait! We are generating the list of divisors, and then using each value in the list exactly once. Like above, why not skip the list, and just use each value as it is generated?

    int last_gcd = 1;
    for (int pd = 1; pd <= d; pd++) {
        if (d % pd == 0) {
            if (180 % pd == 0) {
                last_gcd = pd;
            }
        }
    }

Since we've gotten rid of the divisors list and gcd list, we can name the variable gcd instead of last_gcd (which makes more sense). Combining the two if statements above into one, we arrive at:

    int gcd = 1;
    for (int pd = 1; pd <= d; pd++) {
        if (d % pd == 0  &&  180 % pd == 0) {
            gcd = pd;
        }
    }

    int dem = 180 / gcd;
    if (d / gcd == 1) {
        System.out.print("Radical: pi / " + dem);
    } else {
        System.out.print("Radical: " + (d / gcd) + "pi / " + dem);
    }

Again, see Martin's suggestion for a better GCD(m,n) computation.

---

Separation of concerns. Your rtd and dtr functions should simply convert between radicals and degrees; they should not be doing any input/output. This allows you to test you functions, without trying to create an InputStream for the functions to read data from, or capturing System.out as an StringWriter so the output can be compared with expected output.

For instance, your dtr() function could become:

public static String dtr(int d) {
    int gcd = 1;
    for (int pd = 1; pd <= d; pd++) {
        if (d % pd == 0  &&  180 % pd == 0) {
            gcd = pd;
        }
    }

    int dem = 180 / gcd;
    if (d / gcd == 1) {
        return "pi / " + dem;
    } else {
        return (d / gcd) + "pi / " + dem;
    }
}

The main program would be responsible for reading the value of d, calling dtr(d), and printing the results:

    if (userChoice.contains("D") || userChoice.contains("d")) {  // But see David's improvement

        System.out.print("Degrees: ");
        int d = Integer.parseInteger( userInput.readLine() );
        System.out.println("Radical: " + dtr(d));
    }

---

See also: Scanner.nextInt() as an alternative to Integer.parseInt(userInput.readLine())

Answer 2

Nowadays is common to omit the generic type on the right hand side of an assignment so instead of

 ArrayList<Integer> divisors = new ArrayList<Integer>(); 

you can write

 ArrayList<Integer> divisors = new ArrayList<>(); 

Since you are not using any ArrayList specific functionality it is cleaner to declare divisors as List (same goes for gcd)

 List<Integer> divisors = new ArrayList<>();

The call gcd.get(gcd.size() deserves a separate method with a reasonable name.

Regarding BufferedReader userInput ... while in this example it is harmless to not close IO resources, it is good practice to do so in general. Check out "try with resources statement": https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html.

It is also common to reuse the Reader and not create a new one every time you need to perform a read operation. With this in mind you will have something like this:

public static void main(String[] args) throws IOException {
    System.out.println("Do you want to convert from radicals to degrees, or degrees to radicals?");
    System.out.print("Use \"R\" for radials to degrees, and \"D\" for degrees to radicals: ");
    try (BufferedReader userInput = new BufferedReader(new InputStreamReader(System.in))) {
        String userChoice = userInput.readLine();

        if (userChoice.contains("D") || userChoice.contains("d")) {

            System.out.print("Degrees: ");
            dtr(userInput);

        } else if (userChoice.contains("R") || userChoice.contains("r")) {

            System.out.println("Radical numerator (omit pi): ");
            System.out.println("Radical denominator: ");
            rtd(userInput);

        } else {

            System.out.println("Invalid response. Please restart the program.");
        }
    }
}

Note that now rtd requires only one parameter. Because you print before reading user input, the user gets to see

 Radical numerator (omit pi): 
 Radical denominator: 

before she has an opportunity to provide any input. Those statements should move into rtd interleaved with reading user input.

 if (userChoice.contains("D") || userChoice.contains("d"))

can be simplified like so:

if (userChoice.toLowerCase().contains("d")) {     

Answer 3

The Euclidean algorithm is a well-known and efficient method to compute the greatest common divisor of two integers.

Compared to your approach, the Euclidean algorithm is faster, less code, and does not require additional storage.

Using for example

// https://rosettacode.org/wiki/Greatest_common_divisor#Iterative_Euclid.27s_Algorithm
public static int gcd(int a, int b) {
    while (b > 0) {
        int c = a % b;
        a = b;
        b = c;
    }
    return a;
}

reducing the fraction simplifies to

int degrees = Integer.parseInt(dRead.readLine());
int commonDivisor = gcd(degrees, 180);
int numerator = degrees / commonDivisor;
int denominator = 180 / commonDivisor;