12
\$\begingroup\$

I have been challenged by @nhgrif to write a Fraction class to better my understanding of OOP. I have found an interesting challenge here (I apologize that the source is not mentioned, I was not able to find it either).

Problem statement: Create a class named Fraction having two integer data members named for a fraction's numerator and denominator. The class' default constructor should provide both data members with default values of 1 if no explicit initialization is provided. The constructor must also prohibit a 0 denominator value. Include member functions for displaying an object's data values and mathematical functions capable of adding, subtracting, multiplying, and dividing two Fraction objects.

I have written this on Ideone.com hence explaining why everything is in one file. I have made a slight change to the default values given no parameters, as I have been told a 0/1 default fraction makes more sense than 1/1.

I have also added a simple but potentially useful method to cast a fraction into its decimal value.

I named my fraction variables using a pattern like fractionN_D where N is numerator and D is denominator. It seemed short enough to be practical (as compared to fractionThreeFourths) while still carrying meaning. Any suggestion on that is of course welcome.

 class Fraction {
    private final int numerator;
    private final int denominator;

    Fraction() {
        numerator = 0;
        denominator = 1;
    }

    Fraction (int numerator, int denominator) {
        this.numerator = numerator;
        if (denominator == 0) {
            throw new IllegalArgumentException("Denominator cannot be zero.");
        } else {
            this.denominator = denominator;
        }
    }

    public String getFraction() {
        return numerator + "/" + denominator;
    }
    public double getFractionDecimal() {
        return ((double) numerator) / denominator;
    }

    // Mathematical functions

    public String addFraction(Fraction otherFraction) {
        // Sum formula: a/b + c/d = (ad + cb)/bd
        int a = this.numerator;
        int b = this.denominator;
        int c = otherFraction.numerator;
        int d = otherFraction.denominator;
        return ((a*d) + (c*b)) + "/" + (b*d);
    }

    public String subtractFraction(Fraction otherFraction) {
        // Subtraction formula: a/b - c/d = (ad - cb)/bd
        int a = this.numerator;
        int b = this.denominator;
        int c = otherFraction.numerator;
        int d = otherFraction.denominator;
        return ((a*d) - (c*b)) + "/" + (b*d);
    }

    public String multiplyByFraction(Fraction otherFraction) {
        // Multiplication formula: a/b * c/d = ac/bd
        int a = this.numerator;
        int b = this.denominator;
        int c = otherFraction.numerator;
        int d = otherFraction.denominator;
        return (a*c) + "/" + (b*d);
    }

    public String divideByFraction(Fraction otherFraction) {
        // Division formula: (a/b) / (c/d) = ad/bc
        int a = this.numerator;
        int b = this.denominator;
        int c = otherFraction.numerator;
        int d = otherFraction.denominator;
        return (a*d) + "/" + (b*c);
    }
}

class TestFraction {
    public static void main(String[] args) {
        Fraction fraction3_4 = new Fraction(3,4);
        Fraction fraction2_3 = new Fraction(2,3);

        System.out.println("F1: " + fraction3_4.getFraction() + " Dec: " + fraction3_4.getFractionDecimal());
        System.out.println("F2: " + fraction2_3.getFraction() + " Dec: " + fraction2_3.getFractionDecimal() + "\n");
        System.out.println("F1 + F2: " + fraction3_4.addFraction(fraction2_3));
        System.out.println("F2 + F1: " + fraction2_3.addFraction(fraction3_4) + "\n");
        System.out.println("F1 - F2: " + fraction3_4.subtractFraction(fraction2_3));
        System.out.println("F2 - F1: " + fraction2_3.subtractFraction(fraction3_4) + "\n");
        System.out.println("F1 * F2: " + fraction3_4.multiplyByFraction(fraction2_3));
        System.out.println("F2 * F1: " + fraction2_3.multiplyByFraction(fraction3_4) + "\n");
        System.out.println("F1 / F2: " + fraction3_4.divideByFraction(fraction2_3));
        System.out.println("F2 / F1: " + fraction2_3.divideByFraction(fraction3_4));
    }
}

The output of the above program is as follows:

F1: 3/4 Dec: 0.75
F2: 2/3 Dec: 0.6666666666666666

F1 + F2: 17/12
F2 + F1: 17/12

F1 - F2: 1/12
F2 - F1: -1/12

F1 * F2: 6/12
F2 * F1: 6/12

F1 / F2: 9/8
F2 / F1: 8/9
\$\endgroup\$
  • 4
    \$\begingroup\$ The case for using 1/1 as default is that this is the identity value for multiplication/division. It also avoids one vector by which a fraction produces divide by zero errors as a component of other divisions. \$\endgroup\$ – ben rudgers Mar 5 '15 at 2:42
12
\$\begingroup\$

Public

class Fraction {

I get it that you're writing this in ideone so you cannot do this, but for the record, that class should be public class

Fraction() {

This constructor should be public

Fraction (int numerator, int denominator) {

This constructor should also be public

Constructor calls constructor

As your Fraction() constructor is essentially initializing it with specific values you can call one constructor from another

public Fraction() {
    this(0, 1);
}

Throw or else

if (denominator == 0) {
    throw new IllegalArgumentException("Denominator cannot be zero.");
} else {

If you throw an exception inside the if, there's no need to write else explicitly. Just put the code that is currently in the else after the if-statement. Like this:

public Fraction (int numerator, int denominator) {
    this.numerator = numerator;
    if (denominator == 0) {
        throw new IllegalArgumentException("Denominator cannot be zero.");
    }
    this.denominator = denominator;
}

(I do however agree about @Jeroen's answer about putting the denominator check first)

Returning something useful

public String addFraction(Fraction otherFraction) {

What do you get when you add a Fraction with another Fraction? (Mathematically speaking)

You get... A STRING!

No you don't. You get a Fraction

These mathematical methods you have written here should really return a new Fraction instance.

And you don't need to have Fraction in the name. Simply add, subtract, multiply, divide is enough.

MOAR METHODS!

I would recommend adding a toString method, in your case the implementation is simple:

@Override
public String toString() {
    return getFraction();
}

I would also like to see a normalize method that returns a fraction that has been reduced as much as possible, for example:

  • 18/4 --> 9/2
  • 24/4 --> 6/1
  • 4/8 --> 1/2

I am also missing public int getNumerator() and public int getDenominator(), which would make your Fraction class much more useful.

\$\endgroup\$
  • \$\begingroup\$ Funny you mention getNominator() etc. I actually had those in originally but removed them because I didn't think they'd be useful. Thanks for the great review! \$\endgroup\$ – Phrancis Mar 5 '15 at 1:05
  • 1
    \$\begingroup\$ A gcd() function (Greatest Common Denominator) would be a great help with normalising the fraction. They are very simple to write! \$\endgroup\$ – CJ Dennis Mar 5 '15 at 13:55
  • 2
    \$\begingroup\$ Why would getNumerator() and getDenominator() be useful? 1/2 and 2/4 and 16/32 are the same fraction, not different ones. \$\endgroup\$ – Michael Shaw Mar 5 '15 at 18:53
  • 1
    \$\begingroup\$ @SimonAndréForsberg: Adding one to the denominator isn't an operation on fractions. 0.5 is a fraction. Is adding 1 to the denominator going to give 1/3? 5/11? And there is a way to do this sort of thing with the constructor already, just use a for loop with new Fraction(1, i);. From an OO perspective, you're coupling the interface with its implementation. I also think asking "why not?" to justify adding code is a bad idea; YAGNI and all that. \$\endgroup\$ – Michael Shaw Mar 5 '15 at 22:52
  • 1
    \$\begingroup\$ @SimonAndréForsberg: Not making the numerator and denominator naked public variables is also a restriction. Restrictions aren't necessarily bad. I would consider adding dead code that gains nothing to be harm, even though in this case it isn't a whole lot. I'd really like to argue that the implementation could change, but for this specific example that seems unlikely. In general, implementations can and do change, so while it might not cause much harm in this case, exposing the implementation is not the good OO the OP asked the question to learn about. \$\endgroup\$ – Michael Shaw Mar 6 '15 at 5:41
6
\$\begingroup\$

It's good that you made your class immutable. I'd make the class and its constructors public as well.

If you offer a default constructor at all, you should chain them:

public Fraction() {
    this(0, 1);
}

… but consider offering constants ZERO and ONE instead.

Instead of IllegalArgumentException, consider throwing ArithmeticException — that's what happens when you try to divide by zero.

The getFraction() and getFractionDecimal() methods are confusingly named. I'd call them toString() and toDouble(), respectively.

Your arithmetic functions could have shorter names, e.g. add(Fraction addend). They should each return another Fraction, not a String. (You could always call toString() on the result, but parsing a String back into a Fraction is relatively hard.)

The results should be in reduced form. Arguably, that should be the case for all Fractions, and enforced in the constructor, so that (new Fraction(2, 4)).toString() produces "1/2".

Note that dividing by another fraction has the possibility of dividing by zero.

\$\endgroup\$
6
\$\begingroup\$

This is in addition to Simon's remarks.


It's good practice to validate arguments as the first course of action in the body. Therefore I would suggest this instead:

Fraction (int numerator, int denominator) {
    if (denominator == 0) {
        throw new IllegalArgumentException("Denominator cannot be zero.");
    }

    this.denominator = denominator;
    this.numerator = numerator;       
}

Instead of getFraction() and getFractionDecimal() I would use toString() for the string representation and getDecimal() for the decimal one.

\$\endgroup\$
  • \$\begingroup\$ Your second point: getFraction() reads like it would return a Fraction (public Fraction getFraction() { return this; }) \$\endgroup\$ – Cole Johnson Mar 5 '15 at 19:50
  • \$\begingroup\$ Yes, I noticed it later when other answers mentioned that but I forgot to adjust the naming here. I've done this now, thanks. \$\endgroup\$ – Jeroen Vannevel Mar 5 '15 at 19:52
5
\$\begingroup\$

My suggestion would be to store a fraction rather than output a string, and have a fraction output method (toString or such) that would return the string.

Also, a method that ensures you have the lowest possible denominator would be good, as looking at your code I think you would potentially be missing that - getting 12/24 looks possible, while you should get 1/2 for that result. My math brain is rusty right now so I can't say what the most effective and efficient method for this would be.

Also, if a fraction's denominator becomes 1 (say result is 12/12, which simplifies to 1/1) it should return only a whole number.

And finally, when you have greater than 1 total (11/10 for instance) it should output 1 1/10 or have the option to do so somehow - your choice though, and a step beyond the scope of the challenge I think.

I think with those changes you would be well on your way to a functional Fraction class. I do like it though, looks nice and clean at least.

\$\endgroup\$
  • \$\begingroup\$ I really like the suggestion of outputting 1 1/10 \$\endgroup\$ – Simon Forsberg Mar 5 '15 at 1:34
5
\$\begingroup\$
public String divideByFraction(Fraction otherFraction) {
    // Division formula: (a/b) / (c/d) = ad/bc
    int a = this.numerator;
    int b = this.denominator;
    int c = otherFraction.numerator;
    int d = otherFraction.denominator;
    return (a*d) + "/" + (b*c);
}

Taking in mind the comments made by Simon, 200_success, and others, we need to fix this particular method up a bit.

If this is equal to zero (zero numerator) or otherFraction is equal to zero (zero numerator), the result will be some number over 0. If we were actually returning a Fraction, then we'd be constructing one in here, and the constructor (and thus, this method) will throw an IllegalArgumentException. Java probably has a DivisionByZero except, and this is actually what we want to use, so let's check for zero numerators/divisors. If this has a numerator of zero, we can just quickly return a zero fraction. If otherFraction has a numerator of zero, we need to throw a division by zero exception.

So, something like this:

public Fraction divide(Fraction otherFraction) {
    if (otherFraction.numerator == 0) {
        throw new IllegalArgumentException("Division by zero.")
    }

    int numerator = this.numerator * otherFraction.denominator;
    int denominator = this.denominator * otherFraction.denominator;

    return new this(numerator, denominator);
}

And fundamentally, we have to ask ourselves, should the math functions return a new Fraction or mutate the current one?

You could make the argument for something like this:

public void divide(Fraction divisor) {
    if (divisor.numerator == 0) {
        throw new IllegalArgumentException("Division by zero.")
    }

    this.numerator *= divisor.denominator;
    this.denominator *= divisor.numerator;
    this.reduce();
}

static public Fraction divide(Fraction dividend, Fraction divisor) {
    int numerator = dividend.numerator * divisor.denominator;
    int denominator = dividen.denominator * divisor.numerator;
    return new this(numerator, denominator);
}

So, now usage would look like this:

Fraction threeFifths = new Fraction(3,5);
Fraction oneHalf = new Fraction(1,2);
threeFifths.divide(oneHalf); // three fifths is now 6/5
Fraction twelveFifths = Fraction.divide(threeFifths,oneHalf);

What's missing?

Here's a list of methods that are missing that could be added...

public float toFloat()
public int toInt()

... etc, these may or may not be particularly useful.

More constructors...

public Fraction(int numerator) {
    return this(numerator,1);
}

public Fraction(double d) {
    // this IS possible...
}

Some factory methods:

static public Fraction Zero() {
    return this(0,1);
}

static public Fraction One() {
    return this(1,1);
}

Math methods for other types:

public void divide(int divisor) {
    this.divide(new Fraction(divisor));
}

...etc

\$\endgroup\$
  • \$\begingroup\$ 0/0 is undefined, so divisor = 0 must be checked before shirt-circuiting dividend = 0 \$\endgroup\$ – AlexR Mar 6 '15 at 11:48
4
\$\begingroup\$

Firstly I would say the proper mathematical name for this class is Rational but perhaps that's just a matter of taste.

Secondly I would put the object in a canonical format. For example I would only allow the numerator to be negative. If the denominator is passed into a constructor as negative I would negate the numerator sign and use java.lang.Math.abs(denominator).

I would also divide numerator and denominator by their greatest common divisor (gcd) to make sure objects have a unique and simple form.

In that event your equals method is trivial and you also stave off overflow. Without reduction to simplest terms you will encounter overflow very quickly.

\$\endgroup\$
4
\$\begingroup\$

Evaluating further on Simon André Forsberg point about Returning something useful in his pretty good review.

Let us take a look at the addFraction() method

public String addFraction(Fraction otherFraction) {
    // Sum formula: a/b + c/d = (ad + cb)/bd
    int a = this.numerator;
    int b = this.denominator;
    int c = otherFraction.numerator;
    int d = otherFraction.denominator;
    return ((a*d) + (c*b)) + "/" + (b*d);
}

it would be very easy to change this to returning a Fraction instead of a String

public Fraction add(Fraction otherFraction) {
    // Sum formula: a/b + c/d = (ad + cb)/bd
    int a = this.numerator;
    int b = this.denominator;
    int c = otherFraction.numerator;
    int d = otherFraction.denominator;
    if(b == d) {
        return new Fraction(a + c, b);
    }
    return new Fraction(a * d + c * b, b * d);
}  

and together with a overridden toString() method we don't need the formerly returned String anymore.

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

which I would prefer over the former getFraction() method, because this isn't returning a Fraction.

This together would make e.g. this possible:

Fraction first = new Fraction(12, 3);
Fraction second = new Fraction(6, 3);
Fraction sum = first.add(second);

System.out.print(first + " + " + second + " = " + sum);  

Which would output

12/3 + 6/3 = 18/3  
\$\endgroup\$
  • 1
    \$\begingroup\$ Why so many parentheses in new Fraction(((a*d) + (c*b)), (b*d))? Also, the string concatenation in the call to System.out.println() does not need explicit .toString(). \$\endgroup\$ – 200_success Mar 5 '15 at 17:03
  • \$\begingroup\$ @200_success Good question. Will edit tomorrow when back in the office. Thanks. \$\endgroup\$ – Heslacher Mar 5 '15 at 17:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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