Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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;
        }       
    }
}
share|improve this question
4  
Default constructor should be 0/1 or non-existent. All constructors should prohibit 0 as the denominator value. This ensures that Rational forms an field. –  awashburn Jan 31 at 21:01

5 Answers 5

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 .....

}
share|improve this answer
    
What would you think of the idea of including reducedNumerator and reducedDenominator as fields, with the specification that those fields may only be written once, the first time a rational number is used in a fashion that requires reduction? When adding many fractions together, reducing each intermediate results will not only take time--it will in many cases increase the cost of the next addition. –  supercat Jan 31 at 18:05
    
For example, adding 1/30 to 20/720 will be easier than adding it to 1/36, since in the former case a single division will reveal that multiplying the first fraction by 24/24 will allow it to be added to the second, whereas the latter would require much more work. –  supercat Jan 31 at 18:11
2  
"your 'default' constructor creates the Rational 1/2" No it doesn't! It declares local variables num and den, assigns the values 1 and 2 to them and discards them, leaving the class's fields numer and denom uninitialized. –  David Richerby Feb 1 at 1:55
2  
@DavidRicherby - How did I miss that ??? Thanks. Let me get my glasses.... –  rolfl Feb 1 at 2:16
2  
@rolfl: In the equals method, it should be ==, not = ? –  Martin R Feb 1 at 9:00

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.

share|improve this answer
2  
Hey Hunter, and welcome to CodeReview.... Your logic and reasoning, as expected is right.... The excessive reducing is a good point , but even better is your negative fraction observations... That's a flaw I missed in my assessment. Learned something. The integer division is broken in the source. In computers, (-3/-5) == 0 (his computer math is buggy). The circular logic is because this is an exercise for the asker... but you would be right for a non-trivial implementation. –  rolfl Jan 31 at 17:57
1  
The language (Java, in this case) does have a concept of rational numbers (single-precision floating point float and double-precision double are the primitive types), however a computer's storage of rational numbers is imprecise; consider how to write out 1/3: you get a repeating decimal 0.333... The computer has the same problem (with different rationals, since they are base-2 instead of base-10), and in addition can't correctly store particularly small (~10^-324 for double) or large (~10^307) numbers... nor numbers with digits near them. 1+10^-325 == 1 as far as Java is concerned. –  Brian S Jan 31 at 22:52
    
The other issue is that the OP is using integer division, rather than floating-point division. 3 / 4 is 0.75 mathematically, but Java will tell you it's 0, since the integer will have all the decimal places simply chopped off. 3.0 / 4, on the other hand (or (double)3 / 4), will correctly evaluate to 0.75. That said, because of the imprecision of the floating-point format combined with base-2 data storage creating unexpected rounding errors, comparing the equality of rational numbers rarely works. Equality on the numerator and denominator is best. –  Brian S Jan 31 at 22:54
    
(Also: numeratorA * denominatorB won't give precision problems, since they're integers rather than rationals, so your #4 is a fine answer. It's possible for them to overflow or underflow, but that would require one of them to be outside the range [-2147483648, 2147483647].) –  Brian S Jan 31 at 23:00
    
10 votes! Congratulations for your [badge:nice-answer]! Welcome to CR, hope you stick around! :) –  Mat's Mug Feb 1 at 0:26

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);} 
share|improve this answer

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().

share|improve this answer
7  
Great comment! However, I do not agree with the function name : gcd is a perfectly fine function/method name for a module/class dealing with fraction. From a quick search : Java.math.BigInteger has it, the Python Fraction class has it, Perl 6 has it as a built-in... Also, having "get" in the name of a Math function is a bit awkward : cos() would be prefered over getCosinus() and so should gcd() over getGreatestCommonDivisior(). Again, I agree on everything else and find your comments relevant. –  Josay Jan 31 at 16:39
1  
@Josay +1, style guidelines are fine but sometimes you need to make some compromises to make the code readable (especially since cos, gcd, etc... are fairly standard). If someone doesn't know what gcd is he sure as hell isn't going to know what greatest common divisor is. Stop the madness! –  Thomas Feb 1 at 14:59

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
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.