Rational number arithmetic

Question

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

Answer 1

Right, there's two parts to this review:

1. Is what you are doing 'good'?
2. 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:

1. you cannot use it as a Key in a java.util.Map<Rational,...> because, if the value changes, it will break the internal hashing in the map.
2. similarly, you cannot store these rationals in a java.util.Set<Rational>().
3. If you sort your data, the order will change if you change a Rational's value.
4. The instances are not thread safe.
5. 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<Rational> {

    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
              && ((Rational)other).numer == numer
              && ((Rational)other).denom == denom;
    }

    @Override
    public int compareTo(Rational other) {
        checkRational(other);
        Rational difference = subtract(other);
        if (difference.numer > 0) {
            return 1;
        }
        if (difference.numer < 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 .....

}

Answer 2

I am a mathematician who hasn't written a real line of code in a decade, and not a computer scientist, so please take what I say with huge grains of salt! Just thought you might think my perspective is interesting.

1. Why are you reducing so often? It seems computationally expensive. Of course you will want to reduce when the user asks e.g. what is my fraction, but in the meantime you may wish just to store and compute with un-reduced fractions.

2. It seems like your algorithm will handle negative fractions poorly, because when you run the Euclidean algorithm to reduce, it is random whether your numerator or denominator will wind up with the negative sign (unless I am confused). This won't cause mathematical errors, but it seems like sometimes you will tell the user his fraction is -3/5 and other times that it is 3/-5.

3. In fact, does your code know that (-3/-5) = (3/5) ? (Well, it does, because you have defined equality this way, but will it sometimes tell the user that their fraction is -3/-5 and other times tell the user it is 3/5 ?)

4. You define things by using the built-in division operation on integers. This seems a little circular mathematically (but may well be the right way to do things from a CS perspective) -- how do you define what 3 divided by 5 is if you don't already know what a rational number is? For instance, for your boolean equals, it seems more natural to use cross-multiplication and say

   return (numer * x.denom) == (denom * x.numer)
   

   as this is the standard definition of equality of rational numbers (and doesn't require things to be reduced, and appeals only to a statement working entirely in the class Int). Same complaint for when you compare fractions.

Answer 3

You can make all the other constructors call the 2-variable constructor. For example:

public Rational(int num, int den){
 numer = num;
 denom = den;
 reduce();
}
public Rational(){this(1,2);}
public Rational(Rational r){this(r.numer,r.denom);} 

Answer 4

Your indentation style is a mess, fix that first.

Java normally uses a modified K&R style, like this:

function void style() {
    if (true) {
        // Code
    } else {
        // Code
    }
}

---

You should totally use JavaDoc!

---

private int numer, denom;

Do not declare variables on the same line for clarity purposes. It's easy to overlook the declaration of a variable if they are declared on the same line.

---

public boolean equals(Rational x){

You should only overwrite equals() with a typed variant. Also, overriding equals() and doing it correctly is hard.

---

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

This can be shortened:

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

---

public int compareTo(Rational x){

You should implement the Comparable interface if you create such a method.

---

All of your methods that accept a Rational do not handle the case if it is null.

---

@Override
    public String toString(){
    if (denom == 1){
    return numer + "";
    }
    else{
    return numer + " / " + denom;
    }

Never, ever for what ever reason is int + "" a valid way to cast an integer (or anything for that matter) to a String!

@Override
public String toString() {
    if (denom == 1) {
        return Integer.toString(numer);
    } else {
        return Integer.toString(numer) + " / " + Integer.ToString(denom);
    }
}

Or use String.format().

Answer 5

Your are not handling overflow, and rational numbers can very easily get overflow, even when they don't get close to infinity. This code (assuming you have immutable rationals) approximates sqrt(2):

  Rational r = two;
  for (int i=0; i < 5; ++i) {
    r = r.plus(two.divide(r)).divide(two);
    System.out.println(r);
  }

Even just 5 iterations and your ints get out of range. If your ints overflow, at the very least you should write (extremely complex) code to get an approximate answer, or (much better) use BigInteger for the numerator and denominator.

With BigInteger, after five iterations you get

 886731088897/627013566048