10
\$\begingroup\$

After a couple of small programs in Python we are now asked to do some basic stuff in Java. I don't really know where to start to improve this and whether it's even necessary.

This is an exercise, so the resulting message for when the denominator is zero should not be taken too seriously.

import java.io.*;

/**
 * Class: GetRationalNumber
 * Takes two integers (numerator and denominator) and
 * return them as a decimal fraction.
 */
class GetRationalNumber {
    public static void main(String[] args) throws IOException {
        int p, q;

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

        System.out.println("Numerator: ");
        p = Integer.parseInt(userInput.readLine());

        System.out.println("Denominator: ");
        q = Integer.parseInt(userInput.readLine());

        if(q == 0) {
            System.out.println(
                "Divison by zero can cause serious trouble to the universe!"
            );
            System.exit(0);
        }

        System.out.println("Result:\n" + (double) p / (double) q);
    }
}
\$\endgroup\$
14
\$\begingroup\$
import java.io.*;

It's often a bad practice to import everything in a package. this will clutter your IDE's Ctrl + Space functionality, and makes it harder for other people to see exactly which things you are using from a package.


/**
 * Class: GetRationalNumber

I like the Javadoc, but it's totally overkill to specify the class name in the javadoc. The class name is already displayed in the javadoc functionality itself, you don't need to duplicate that when you write javadoc.


int p, q;

Make it a habit ASAP to avoid one-letter variable names like these. numerator and denominator would be much better.


Possible improvement: Catch NumberFormatException (or use a regex in advance to make sure that it is a number, but that might a bit over your head right now).


System.out.println(
    "Divison by zero can cause serious trouble to the universe!"
);

I would write this on one line:

System.out.println("Divison by zero can cause serious trouble to the universe!");

System.exit(0);

The zero here indicates the exit status, which can be used by scripts and other stuff. If a program terminates normally, that is exit status zero. Is a possible division by zero really a normal program termination? In my opinion it is not. I would do System.exit(1); or System.exit(42); or something.


Overall, there's nothing object-oriented about what you have done, but I guess you haven't gotten that far yet, huh? :) Your code is quite fine for being beginner code. Now take it to the next level :)

\$\endgroup\$
  • \$\begingroup\$ Ah, I tried to stick to the PEP8 80-chars-per-line limit for Python there. Thank you. Having a look into error handling. :) \$\endgroup\$ – kleinfreund Nov 24 '14 at 18:04
  • 1
    \$\begingroup\$ @JeroenVannevel I guess everyone is entitled to their own opinion. It may just be a matter of personal preference. In some cases, when it is a package of my own, I might also use .*, but I avoid using a wildcard import for things like java.util, java.stream, java.io... \$\endgroup\$ – Simon Forsberg Nov 24 '14 at 19:08
  • 2
    \$\begingroup\$ @kleinfreund Java is really verbose, 80 characters is not much! \$\endgroup\$ – Marc-Andre Nov 24 '14 at 20:04
  • 2
    \$\begingroup\$ @kleinfreund I would definitely not include the entire package for only 3 classes. I think most IDEs have a setting for a threshold for how many classes to import before using a wildcard import. \$\endgroup\$ – Simon Forsberg Nov 24 '14 at 21:25
  • 1
    \$\begingroup\$ @kleinfreund Personally, I only import * if I'm importing 6 or more classes from a package. 3 is not a lot. \$\endgroup\$ – raptortech97 Nov 26 '14 at 4:04
11
\$\begingroup\$

The task being performed is super simple, so I'm going to be picky.

Since the class has a main() to be executed, it should probably be a public class. Class names should be nouns, not verbs.

Variables should be declared as close as possible to their point of use. It would be better to write

int p = Integer.parseInt(userInput.readLine());

parseInt() can throw a NumberFormatException. It would be best to catch it and handle it. However, if you just want to let the program crash, it would still be a good idea to declare it explicitly in the method signature of main(), even though RuntimeExceptions aren't required to be declared.

I suggest putting a space after if, so that it looks less like a function call. In the case of a zero denominator, it would be better to exit with a non-zero status to indicate failure.

Note that with floating-point arithmetic, division by zero is not an error — it will successfully produce Infinity. Perhaps it is unnecessary to check for division by zero.

\$\endgroup\$
  • \$\begingroup\$ Thank you as well. Seeing your points and they make sense to me. \$\endgroup\$ – kleinfreund Nov 24 '14 at 18:06
7
\$\begingroup\$

These are all equivalent:

System.out.println("Result:\n" + (double) p / (double) q);
System.out.println("Result:\n" + p / (double) q);
System.out.println("Result:\n" + (double) p / q);

So I would drop one of the casts to make it shorter and simpler.


You could simplify the processing of input using a Scanner:

Scanner userInput = new Scanner(System.in);

System.out.println("Numerator: ");
int p = userInput.nextInt();

System.out.println("Denominator: ");
int q = userInput.nextInt();

About keeping variable references close together (the lines with int p; and p = ...) I'd like to quote a section from the wonderful Code Complete:

The code between references to a variable is a “window of vulnerability.” In the window, new code might be added, inadvertently altering the variable, or someone reading the code might forget the value the variable is supposed to contain. It’s always a good idea to localize references to variables by keeping them close together.

The idea of localizing references to a variable is pretty self-evident, but it’s an idea that lends itself to formal measurement. One method of measuring how close together the references to a variable are is to compute the “span” of a variable.


It might be interesting also to create a class to capture the notion of a rational number, for example:

class RationalNumber {
    private final int numerator;
    private final int denominator;

    RationalNumber(int numerator, int denominator) {
        if (denominator == 0) {
            throw new IllegalArgumentException("The denominator must be non-zero");
        }
        this.numerator = numerator;
        this.denominator = denominator;
    }

    @Override
    public String toString() {
        return Double.toString((double) numerator / denominator);
    }
}

Thanks to the overridden toString method, you can print with:

RationalNumber rationalNumber = new RationalNumber(p, q);
System.out.println("Result:\n" + rationalNumber);

or even:

System.out.println("Result:\n" + new RationalNumber(p, q));

Lastly, I really find the line breaks odd in new BufferedReader. (And System.out.println too, as Simon has already mentioned.)

\$\endgroup\$
1
\$\begingroup\$

I second janos in avoiding error messages like "Divison by zero can cause serious trouble to the universe!".

Though this may sound kind of funny at first glance:

a) it's simply not true. The universe is "Bigger than the biggest thing ever and then some. Much bigger than that in fact, really amazingly immense, a totally stunning size, real 'wow, that's big', time.". It doesn't even chuckle about a man-made error that occurred in using a mouse-made complementary science on a giant supercomputer that is to be destroyed anyway.

b) it's more convenient for the users of your program being told what can or even must be done to avoid an error rather than just being told what kind of error and why it occurred.

IMHO, the best is to supply both, in neutral or even polite language, as if you were talking to your program's users personally:

"A division by zero occurred. Please supply a denominator that is not zero."

c) using exclamation marks in messages is like shouting at your users. Nobody likes to be shout at. Even less from soulless, stupid machines.

P.S.: One of the greatest sites I've ever seen. It's quite old (1996-2000) but still highly topical regarding the principles. The original is offline for a long time. Thankfully mirrored since then: The Interface Hall of Shame by Isys Information Architects Inc.. Have fun like I have every time I visit it (again). (And, yes, one of those reader's comments is from me. :-)

\$\endgroup\$
0
\$\begingroup\$
public static void main(String[] args) throws IOException

You only throw an exception if you want any higher function to handle the same. Instead handle it in the main function itself with proper logs
< https://stackoverflow.com/questions/17629321/throwing-exception-in-main-method >

\$\endgroup\$

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.