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 &lt; x.numer / x.denom){
        return (-1);
        }
        else{
        return (1);
            }    
        }

//Find greatest common divisor
        static int gcd(int x, int y){
        int r;
        while (y != 0) {
        r = x % y;
        x = y;
        y = r;
            }
        return x;
        }

//Rational Addition            
        public void plus(Rational x){
        int greatdenom = x.denom * denom;       
        int multx = greatdenom / x.denom;
        int mult = greatdenom / denom;
        denom = x.denom * denom;
        numer = (x.numer * multx) + (numer * mult);
        reduce();
        }

//Rational Subtraction
        public void minus(Rational x){
        int greatdenom = x.denom * denom;       
        int multx = greatdenom / x.denom;
        int mult = greatdenom / denom;
        denom = x.denom * denom;
        if (x.numer &gt; 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        
        public void divBy(Rational x){
        numer = numer / x.numer;
        denom = denom / x.denom;
        reduce();
        }

//Fraction simplifier        
        private void reduce(){
        int divisor;
        divisor = Rational.gcd(numer, denom);
        numer = numer / divisor;
        denom = denom / divisor;
        }

    @Override
        public String toString(){
        if (denom == 1){
        return numer + "";
        }
        else{
        return numer + " / " + denom;
        }       
    }
}
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 &amp;&amp; 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&lt;Rational,...&gt; 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&lt;Rational&gt;().
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&lt;Rational&gt; {

    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);
    }

    @Override
    public int hashCode() {
        // somewhat convenient hashcode ... it's a bitwise trick...
        // could be improved...
        return numer * 31 ^ denom * 31;
    }

    @Override
    public boolean equals(Object other) {
        return other instanceof Rational
              &amp;&amp; ((Rational)other).numer == numer
              &amp;&amp; ((Rational)other).denom == denom;
    }

    @Override
    public int compareTo(Rational other) {
        checkRational(other);
        Rational difference = subtract(other);
        if (difference.numer &gt; 0) {
            return 1;
        }
        if (difference.numer &lt; 0) {
            return -1;
        }
        return 0;
    }

    // return a new instance instead of updating ourselves...
    public Rational add(Rational addend) {
        checkRational(other);
        return new Rational(addend.numer * denom + numer * other.denom, denom * other.denom);
    }


    ....... Lots more .....

}