Rational number arithmetic

I was assigned to make a Rational class to perform functions on fractions. After awhile I got it to work, but now I'm wondering how to simplify and clean up my code. Basically how to improve and use good Java practices so I can write better Java in the future.

```
public class Rational {

private int numer, denom;

//constructors
public Rational(){
int num = 1;
int den = 2;
reduce();
}
public Rational(int num, int den){
numer = num;
denom = den;
reduce();
}
public Rational(Rational x){
numer = x.numer;
denom = x.denom;
reduce();
}

//setters
public void setNumer(int num){
numer = num;
reduce();
}
public void setDenom(int den){
denom = den;
reduce();
}
public void setRational(int num, int den){
numer = num;
denom = den;
reduce();
}

//getters
public int getNumer(){
return numer;
}
public int getDenom(){
return denom;
}

//Copy method
public void copyFrom(Rational x){
numer = x.numer;
denom = x.denom;
reduce();
}

//Equals method
public boolean equals(Rational x){
if (numer / denom == x.numer / x.denom){
return(true);
}
else {
return(false);
}
}

//Compare to method
public int compareTo(Rational x){
if (numer / denom == x.numer / x.denom){
return (0);
}
else if (numer / denom numer){
numer = (x.numer multx) - (numer mult);
}
else {
numer = (numer mult) - (x.numer multx);
}
reduce();
}

//Multiplication
public void times(Rational x){
numer = numer * x.numer;
denom = denom * x.denom;
reduce();
}

//Division

Right, there's two parts to this review:

• Is what you are doing 'good'?

• Is there a better way?

The answer to the second question is 'yes', but we'll have to explore your current code to understand why....

Current code, is it good?

Your code, for the most part, is neat, well-named, and generally understandable. Great. But, there are some significant problems too:

-
your 'default' constructor creates the Rational 1/2 .... why? What's special about that....? Remove it.

-
It is better to have one 'core' constructor, and then have the other constructors call it.... :

public Rational(int num, int den){
    numer = num;
    denom = den;
    reduce();
}

public Rational(Rational x){
    this(x.numer, x.demon);
}

-
The equals(...) and hashCode() contract. If you override one of equals() or hashCode() you should always also override the other.

-
your equals() method is oddly broken because it should take an Object as an argument. You have a situation where you may compare two rationals in a context where Java will call the equals(Rational) in one situation, but in a different situation (if Java does not know that the value really is a Rational) it may call equals(Object)... and that may return false. This will lead to some interesting confusion..... hmmm.

-
you implement the method compareTo(Rational) but your class does not implement the Comparable interface. If you implement the Comparable interface you can do things like put your values in an array and then sort them using the native Java mechanisms, etc.

-
your public interface methods should have comprehensive names, even if your variables do not: setNumer(int) and setDemon(int) should be setNumerator(int) and setDenominator(int) respectively. The same is true for the get* versions. Just because you were a bit lazy and used the auto-generated getter/setter mechanism in your IDE to create these methods, does not mean they are good names.

-
you are not validating your denominator for division-by-zero.

-
if your numerator is 0 you should have a special case for it.

-
you should check for nulls for all input Rationals

OK, that's enough about the style.... let's go through the bugs... ;-)

-
you do integer division a lot, and you are getting wrong answers (note that this equals() method is broken for other reasons too):

public boolean equals(Rational x){
    if (numer / denom == x.numer / x.denom){
        return(true);
    } else {
        return(false);
    }
}

This is broken, because, consider two Rationals 1/1 and 99/50 ... when you do your math, you are doing integer division, so both of them have the result 1, and 1 == 1 so your code will declare that 1 == 1.5, which is only off by nearly 100%....

This same type of logic is used in the compareTo() method.

Since you reduce() your Rational after every operation, your equals method could be as simple as return numer == x.numer && denom == x.denom;

OK, that's enough about the general issues....

The better way.

Storing your Rational numbers is an important part of many programs. The underlying problem with your class is that it is Mutable. .... you can change the value of a Rational. This is unfortunate for a few reasons:

• you cannot use it as a Key in a java.util.Map because, if the value changes, it will break the internal hashing in the map.

• similarly, you cannot store these rationals in a java.util.Set().

• If you sort your data, the order will change if you change a Rational's value.

• The instances are not thread safe.

• serializing the instances is a problem.

This problem is well understood, and all the Java number-type classes are Immutable (Integer, Long, Double, BigInteger, BigDecimal, ......).

BigDecimal is a really good example of what you should be doing here. Have a look at it's Javadoc. Notice how all the mathematical methods do not modify the BigDecimal, but return a new BigDecimal with the right value.

Putting all of this together, your class should really look something like:

```
public final Rational implements Comparable {

private static final int getGCD(int a, int b) {
.....
return divisor;
}

private static final Rational checkRational(rat) {
if (rational == null) {
throw new NullPoiterException("Must supply a non-null Rational value");
}
return rat;
}

private final int numer, denom;

public Rational(int numerator, denominator) {
if (denominator == 0) {
throw new IllegalArgumentException("Denominator cannot be 0");
}
// reduce the value at construct time....
int div = numerator == 0 ? 1 : getGCD(numerator, denominator);
// div is guaranteed to be a divisor, so integer division is safe
this.numer = numerator/div;
this.denom = numerator == 0 ? 1 : denominator/div;
}

public Rational(Rational other) {
this(other.numer, other.denom

StackExchange Code Review Q#40554, answer score: 25