Implementation of a Fraction class in Java

Question

I am writing a bunch of Math related classes:BigInteger,Fraction,matrix,vector,polynomial,set etc.All of these classes will use each other.The classes will either work in decimal format or in Rational format at a time(eg as FractionalMatrix and DecimalMatrix) so I thought it will be best to implement the Fraction class first.

My primary concern is that this is going to be a big project.So, code readability and management is very important.Any suggestions on making the code better are appreciated.

Here is the Fraction class:

public final class Fraction {

private int numerator;

private int denominator;

public Fraction(int numerator,int denominator,boolean wantToReduce) {
    if (denominator == 0) {
        throw new IllegalArgumentException("The denominator is zero.");
    }
    if(numerator==0){
        this.numerator = 0;
        this.denominator = 1;
    }
    else{
    this.numerator = numerator;
    this.denominator = denominator;
    }
    if(denominator<0){
        this.numerator = -1*this.numerator;
        this.denominator = -1*this.denominator;
    }
    if(wantToReduce==true)
        this.reduce();
}
public Fraction(int num) {
    this.numerator = num;
    this.denominator = 1;
}

@Override
public String toString() {
    if(denominator!=1)
    return numerator+"/"+denominator;
    else
        return numerator+"";
}

@Override
public boolean equals(Object obj) {
    if(!(obj instanceof Fraction))
        return false;
    Fraction f = ((Fraction)obj);
    int gcd= Math2.gcd(numerator,denominator);
    f.reduce();
    if(this.numerator/gcd==f.numerator && this.denominator/gcd == f.denominator)
        return true;
    else
        return false;
}

public Fraction reduce(){
    int gcd = Math2.gcd(numerator,denominator);
    numerator = numerator/gcd;
    denominator = denominator/gcd;
    return this;
}

//Cannot decide weather to make static methods or not so I randomly picked one. 
public static Fraction add(Fraction f1,Fraction f2,boolean w){
    return new Fraction(f1.numerator*f2.denominator+f2.numerator*f1.denominator,f1.denominator*f2.denominator,w);
}
public static Fraction sub(Fraction f1,Fraction f2,boolean w){
    return new Fraction(f1.numerator*f2.denominator-f2.numerator*f1.denominator,f1.denominator*f2.denominator,w);
}
public static Fraction mul(Fraction f1,Fraction f2,boolean w){
    return new Fraction(f1.numerator*f2.numerator,f1.denominator*f2.denominator,w);
}
public static Fraction div(Fraction f1,Fraction f2,boolean w){
    return new Fraction(f1.numerator*f2.denominator,f1.denominator*f2.numerator,w);
}


}

Here is the Math2.gcd method, I have used The Euclidean Algorithm for finding the GCD.

 public static int gcd(int a,int b){
    if(a<0)
        a=-1*a;
    if(b<0)
        b=-1*b;
    int t;
    while(b!=0){
        t=b;
        b = a%b;
        a = t;          
    }
    return a;
}

Answer 1

I'm not going to repeat the basics which Ronan has covered fairly comprehensively. I'd just add: why use -1*a when you could use -a?

However, I have some design questions:

Reduction

I have a Rational class which I have used for about 15 years. It always reduces, and I've never found this to be a problem. Why would you want unreduced fractions?

In addition, consider that being reduced or not is not really an property of the number: it's a property of how the number is displayed in text. If you have use cases, would they be covered equally well or better by a toString method which takes a desired denominator?

Among the advantages of using reduced numbers are i) it simplifies equality testing; ii) it reduces the risk of overflow (or improves memory usage and speed, if using BigIntegers for numerator and denominator).

Mutability

Whether you keep unreduced numbers or not, make them immutable. In fact, I would recommend making every class in your CAS immutable, and using the builder pattern if you think this makes it too hard to instantiate matrices.

int vs BigInteger

Why are the numerator and denominator stored as ints? They will easily overflow, especially given that adding two rationals requires multiplying numerators and denominators. If you do stick with ints to keep memory usage low then you should cast to longs in the arithmetic methods and then reduce before casting back to ints, but I would recommend using BigInteger.

A final note

Well done for considering the special cases around zero and negative numbers in the constructor.

Answer 2

General notes about formatting

Your indentation isn't very good. Also, it's usually a good idea to have space between comparator/equal sign and their operand (compute(a) != 0 instead of compute(a)!=0).

You also want to have a space after commas to make the parameters more readable.

Any decent IDE have a code formatter that can solve all this issues ;)

A few proposals

• I'd make another constructor taking two int value and delegating to your first constructor :

  public Fraction(final int numerator, final int denominator) { this(numerator, denominator, true); }

• Fraction should probably be made Serializable. Is it also voluntary that Fraction doesn't implement Comparable ?

• It may also be useful to have an immutable constant for Zero instead of always creating a new object for this very common value.

  public static final Fraction ZERO = new Fraction(0);

• You should add a method that allows you to turn your Fraction into an int (or into your BigInteger class) as well as another one to turn it into a double (or another object of your making) allowing you to cast your fraction to other type.

Step by step analysis

public final class Fraction {

private int numerator;

private int denominator;

Your object is mutable (mainly due to the reduce method). I'm not sure you want it to be immutable (but it's a good idea considering the code) but you should at least make it thread-safe. To turn it into an immutable object, you should make numerator and denominator final and rework a bit your reduce method as well as your main constructor.

@Override
public String toString() {
    // ...

This one is good, I'd write it as...

@Override
public String toString() {
    return numerator + ((denominator != 1) ? "/" + denominator : "");
}

but that's nit-picking. If you turn your object into an immutable one, it may be worth to cache the toString result ;)

@Override
public boolean equals(Object obj) {
    // ...

Formatting aside, there are two things that bothers me :

f.reduce();

You are modifying the argument in an equals method !!! ALERT !! :) If you make the Fraction method immutable this will be solved. ;)

if(this.numerator/gcd==f.numerator && this.denominator/gcd == f.denominator)
    return true;
else
    return false;

This can be simplified to :

return this.numerator/gcd == f.numerator && this.denominator/gcd == f.denominator;

Also, 99.9% of the time you want to have your own hashCode implementation when you modify the equals method so you should consider overriding it. For example :

public int hashCode() {
    final int prime = 919;
    return prime * numerator + denominator;
}

For the add,sub... methods I'd have made them object method myself but that's really up to you in the end ;) w is a poor name for the boolean parameter but, aside from that, it's ok.

Your gcd method is good (formatting aside) but you should consider making a short javadoc for that not-so-easy-to-understand method.

[EDIT] : it's better when hashCode give the same hashCode for two equals objects, as such you should only use reduced number for your hashCode calculation, the implementation given in my code is thus error-prone ;)

if you make your object immutable, you'll encounter the following problem

x = new Fraction(4, 2, false);
x.equals(x); // return false

which sadly breaks one of the most basic contract from equals (reflexivity), you should then only use equals with reduced number

Answer 3

indentation

If your actual indentation doesn't look like this but you manualy put 4 spaces here to turn it into a code block: Just paste the code, select it all then press the {} button at the top of the editor to have it auto indent for you.

If this is your actual indentation I advise you to use a (free) IDE (most popular ones are IntelliJ and Eclipse but others exist too) and have it correct the indentation for you.

always {}

You should always put braces after each if/else/for/... as posted here.

boolean == true

if(wantsToReduce == true) is exactly the same as if(wantsToReduce). if() expects a boolean value. wantsToReduce is already a boolean value. Combining 2 boolean values with == returns a new boolean value. Might as well just use your original boolean directly.

reduce

I would argue that it's cleaner in general to always reduce when creating a new fraction. Since you yourself already think that 2/3 is equal to 4/6 (because you want to reduce inside the equals). Then it makes perfect sense to always work with the reduced version.

immutable

After the reduce change we might want to make the entire class immutable (since immutable classes are generally easier to get "right").

To do this you should put the reduce functionality into the constructor and make the numerator and denominator final. The new constructor then looks like this:

public Fraction(int numerator, int denominator) {
    if (denominator == 0) {
        throw new IllegalArgumentException("The denominator is zero.");
    }
    if (numerator == 0) {
        this.numerator = 0;
        this.denominator = 1;
    } else {
        if (denominator < 0) {
            numerator = -numerator;
            denominator = -denominator;
        }
        int gcd = Math2.gcd(numerator, denominator);
        this.numerator = numerator / gcd;
        this.denominator = denominator / gcd;
    }
}

equals and hashcode

If you implement equals you should always implement hashcode as well. Now that we use an always reduced version of Fraction this becomes trivial. Even better to have your IDE generate them for you:

@Override
public boolean equals(Object o) {
    if (this == o) {
        return true;
    }
    if (o == null || getClass() != o.getClass()) {
        return false;
    }

    Fraction fraction = (Fraction) o;

    if (numerator != fraction.numerator) {
        return false;
    }
    if (denominator != fraction.denominator) {
        return false;
    }

    return true;
}

@Override
public int hashCode() {
    int result = numerator;
    result = 31 * result + denominator;
    return result;
}

static vs instance method

I'd say it depends on how many other Number like classes you want the methods to be compatible with. If you want to only add fractions together then it would make more sense to have the add/sub/... methods non-static. It reads easier to have frac1.add(frac2).mul(frac3) instead of mul(add(frac1,frac2),frac3)).

On the other hand if you also want to support adding/multiplying/... with int for example you can use method overloading like the java devs did in their Math class. I would also put them into your Math2 class instead of in the Fraction class.

I would go for the static methods inside your Math2 class option, but some purist feel strong against Utility classes. (Note that your Math2 class should also have a private final constructor to prevent people from pointlessly subclassing it).

Answer 4

The proposed changes:

• The positives and negatives in the fraction are irrelevant and do not matter. the numerator and denominator are not publicly exposed. Hence remove positives and negatives conversion in constructor.

• i went for a more immutable approach. i'd much rather send queries twice then store anything in a variable for repeated use, unless performance becomes an issue. because then you have to manage state. so whenever you add or subtract - you are dealing with a completely new fraction object!

• Remove the reduce() method from the constructor. If the client wants a reduced fraction, then they can simply call it themselves. Also for the same reason - remove the reduce parameter from the constructor. It can simply be called as a public method by the client if required.

• Change the API so that you can send messages to fraction objects and then can respond in kind - i.e. you can change fraction objects together to perform complex calculations - it's much harder to do this if you use static functions. furthermore.

• Rename methods to fuller names - easier understandability

• Add hash code method given we are comparing fractions.

• I went for a polymorphic approach - if you wanted to add different types of fractions and want them treated differently - you can easily do it without modifying anything (apart from the factory method). i.e. it is open for extension but largely closed from modification.

• Static methods vs object methods? which reads easier? I prefer the instance methods. look how much easier they read. half add a quarter mutliply by half and reduce the total. simple! it's harder to read as a static method.

Fraction half = Fraction.Factory(1, 2);
Quarter quarter = Fraction.Factory(1, 4);
half.add(quarter).multiply(half).Reduce();
Fraction.add(half, quarter, true)...etc???

The code:

• btw it's in c# - who can really tell the difference anyways. link here because SO formatting is just horrible. it was coming out a complete mess.

Edit

• @Imus is right - positives and negatives do matter when comparing equality. I made a mistake. It's probably easier to treat this in the constructor via a method - to preserve readability. i haven't made those changes yet.