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

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

-
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. –  recursion.ninja Jan 31 '14 at 21:01

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...
checkRational(other);
return new Rational(addend.numer * denom + numer * other.denom, denom * other.denom);
}

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

}

-
@supercat without sync, there's no guarantee that both redN and redD would both be visible to the second thread at the same time... one may arrive before the other. It is broken without sync. –  rolfl Jan 31 '14 at 18:47
"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 '14 at 1:55
@DavidRicherby - How did I miss that ??? Thanks. Let me get my glasses.... –  rolfl Feb 1 '14 at 2:16
@rolfl: In the equals method, it should be ==, not = ? –  Martin R Feb 1 '14 at 9:00
@MartinR yes it should. thanks –  rolfl Feb 1 '14 at 12:23

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.

-
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 '14 at 17:57
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 '14 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 '14 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 '14 at 23:00

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

-

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


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){


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

-
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 '14 at 16:39
@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 '14 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

-