3

This is my Fraction class code with several methods, the requirement I have is to keep numerator an denominator as an int:

/**
 * @author GKsiazek
 * Reference: https://github.com/kiprobinson/BigFraction/blob/master/com/github/kiprobinson/util/BigFraction.java
 * Reference: https://github.com/kiprobinson/BigFraction/blob/master/com/github/kiprobinson/junit/BigFractionTest.java
 */
package Fraction;
import java.math.*;
public class Fraction {

    private int numerator;
    private int denominator;


    /**
     * Constructor with two int parameters
     * @param num is numerator
     * @param den is denominator
     */
    public Fraction()
    {}

    public Fraction(int num, int den) {
        if (den==0){//denominator cannot be zero
            System.out.println("Denominator cannot be zero");
            return;
            }
        this.numerator = num;
        this.denominator = den;
        this.normalize();
    }

    /**
     * Constructor with one int parameter
     * @param num is numerator
     * @param den is assumed one
     */

    public Fraction(int num){
        this.numerator = 1;
        this.denominator = num ;
    }

    /**
     * Constructor with String parameter
     * @param str  
     * Only String in a following format "numerator/denominator" allowed
     * If the String consists of one int it is considered denominator
     * Numerator will be considered 1
     * Denominator cannot be zero
     */

    public Fraction(String str)
    {
        if(str.isEmpty())
        {
            System.out.println("The str (String) parameter cannot be empty!");
        return;
        }

             String[] data = str.split("/");
             if(data.length==0)
                 System.out.println("The str (String) parameter cannot be empty");
             try
             {
                 this.numerator = Integer.parseInt(data[0]); 
             }
             catch (Exception ex)
             {
                 System.out.println(ex.toString());
             }
             try
             {

                 this.denominator = Integer.parseInt(data[1]);
                 if(this.denominator==0) throw new Exception("Denominator can't be 0");
             }
             catch (Exception ex)
             {
                 System.out.println(ex.toString());
             }
             this.normalize();
    }



    /**
     * the method is applied within the constructors
     * normalize method takes care of fraction normalization
     * 1.Converts the numerator and denominator into BigInteger 
     * 2.Finds the GCD of both
     * 3.if the GCD is larger than 1 it divides numerator and denominator by GCD
     * @param numerator
     * @param denominator
     */
    private void normalize()//int numerator, int denominator)
    {
        BigInteger gcd;
        BigInteger num = BigInteger.valueOf(this.numerator);
        BigInteger den = BigInteger.valueOf(this.denominator);
        gcd = num.gcd(den);
        if (gcd.intValue() > 1)
        {
            this.numerator = numerator / gcd.intValue();
            this.denominator = denominator / gcd.intValue();
        }
    }
    public Fraction abs() {
        return null;
    }
    public int getNumerator()
    {
        return this.numerator;
    }

    public int getDenominator()
    {
        return this.denominator;
    }
    /*
     * a/b + c/d is (ad + bc)/bd
     */
    public Fraction add(Fraction g)
    {

        int numerator = this.numerator * g.denominator + this.denominator * g.numerator;
        int denominator = this.denominator * g.denominator;

        return new Fraction(numerator,denominator);
    }
    /**
     * subtract method
     * a/b - c/d is (ad - bc)/bd
     * calls the normalize method to make sure that the Fraction h
     * is in the normalized  form
     */

    public Fraction substract (Fraction g)
    {

        int num = this.numerator * g.denominator - this.denominator * g.numerator;
        int den = this.denominator*g.denominator;
        return new Fraction(num,den);
    }
    /**
     * equals method
     * public boolean equals(Object o)
     */
    public boolean equals (Object o){
       if(o == null)return false;
        return o.equals(this);

    }
    /**
     * Multiplication
     * (a/b) * (c/d) is (a*c)/(b*d)
     * @param g
     * @return
     */
    public Fraction multiply(Fraction g)
    {
        int num = this.numerator * g.numerator;
        int den = this.denominator * g.denominator;
        return new Fraction(num,den);
    }
    /**
     * Division
     * (a/b) / (c/d) is (a*d)/(b*c)
     * @param g
     * @return
     */
    public Fraction divide(Fraction g)
    {
        int num = this.numerator * g.denominator;
        int den = this.denominator * g.numerator;
        return new Fraction(num,den);

    }
    /**
     * Negation
     * -(a/b) is -a/b
     */
    public Fraction negate()
    {
        int num = Math.abs(this.numerator) * -1;
        int den = this.denominator;
        return new Fraction(num,den);
    }
    /**
     * Inverse of a/b is b/a
     */
    public Fraction inverse()
    {
        int num = this.denominator;
        int den = this.numerator;
        return new Fraction(num,den);
    }
    /**
     * a/b > c/d if ad > bc
     * @return
     */
    public boolean greaterThan(Fraction g)
    {
        if(this.numerator * g.denominator > this.denominator * g.numerator)
        {
            return true;//use the subtract() but how to
        }
        else return false;
    }

    /**
     * lessThan method
     * a/b < c/d if c/d > a/b
     * @param g
     * @return
     */
    public boolean lessThan(Fraction g)
    {
        if (this.greaterThan(g)==false)
        {
            return true;
        }
        else return false;
    }
    @Override
    public String toString()
    {
        return this.getNumerator()+"/"+this.getDenominator();
    }
}

This is my test class, all tests are successful except for negate, I've tried several options, just multiplying by -1 or simply negating with -, no joy.

package Fraction;
import static org.junit.Assert.*;

import java.lang.reflect.Method;

import org.junit.Test;
import org.junit.Ignore;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;

@RunWith(JUnit4.class)
public class FractionsTest {

    /**
     * test methods checking the constructors 
     */
    @Test
    public void testFractionNum_Den() 
    {
        Fraction f = new Fraction(2,6);
        assertEquals(1, f.getNumerator());
        assertEquals(3, f.getDenominator());
    }
    @Test
    public void testFractionNum() 
    {
        Fraction f = new Fraction(5);
        assertEquals(1, f.getNumerator());
        assertEquals(5, f.getDenominator());
    }
    @Test
    public void testFractionStr()
    {
        Fraction f = new Fraction("1/5");
        assertEquals(1, f.getNumerator());
        assertEquals(5, f.getDenominator());

    }
    @Test
    public void testNormalize()
    {
        Fraction f = new Fraction(2,4);
        assertEquals(1, f.getNumerator());
        assertEquals(2,f.getDenominator());
    }
    /**
     * Method m=Dummy.class.getDeclaredMethod("foo");
     * m.setAccessible(true);//Abracadabra 
        m.invoke(d);
     */
    @Test
    public void testAdd()
    {
        Fraction g = new Fraction(1,3);
        Fraction toTest = g.add(new Fraction(1,3));
        assertEquals(2, toTest.getNumerator());
        assertEquals(3, toTest.getDenominator());
    }
    @Test
    public void testSubtract()
    {
        Fraction g = new Fraction (4,6);
        Fraction toTest = g.substract(new Fraction(2,6));
        assertEquals(1, toTest.getNumerator());
        assertEquals(3, toTest.getDenominator());
    }
    @Test
    public void testMultiply()
    {
        Fraction g = new Fraction (2,3);
        Fraction toTest = g.multiply(new Fraction(1,2));
        assertEquals(1, toTest.getNumerator());
        assertEquals(3, toTest.getDenominator());
    }
    @Test
    public void testDivide()
    {
        Fraction g = new Fraction (2,3);
        Fraction toTest = g.divide(new Fraction(1,3));
        assertEquals(2, toTest.getNumerator());
        assertEquals(1, toTest.getDenominator());
    }
    @Test
    public void testNegate()
    {
        Fraction g = new Fraction(1,3);
        g.negate();
        assertEquals(-1, g.getNumerator());
        assertEquals(3, g.getDenominator());
    }
    @Test
    public void testgreaterThan()
    {
        Fraction g = new Fraction(1,3);
        assertEquals(false, g.greaterThan(new Fraction(2,3)));
        assertEquals(true, g.greaterThan(new Fraction(1,5)));
    }
    @Test
    public void testlessThan()
    {
        Fraction g = new Fraction(2,3);
        assertEquals(false, g.lessThan(new Fraction(1,3)));
        assertEquals(true, g.lessThan(new Fraction(4,5)));

    }

    @Test
    public void testtoString()
    {
        Fraction g = new Fraction(1,3);
        String f = g.toString();
        assertEquals(true, f.contentEquals("1/3"));
    }

    }
Kip
  • 107,154
  • 87
  • 232
  • 265
user3274207
  • 103
  • 1
  • 8
  • hey, FYI I wrote the code you're referencing at the top. :) if it's of any interest to you, the latest & greatest is now on github: https://github.com/kiprobinson/BigFraction – Kip Jun 02 '14 at 23:21

2 Answers2

6

The problem is that your negate method returns a new Fraction object that you're never re-assigning to your g variable:

public void testNegate()
{
    Fraction g = new Fraction(1,3);
    g = g.negate(); //added "g ="
    assertEquals(-1, g.getNumerator());
    assertEquals(3, g.getDenominator());
}

As a side note, your negate method has an issue, which is that the numerator will always be negative due to multiplying the absolute value of numerator (which will always be positive) by -1. Just remove the Math#abs usage from there:

public Fraction negate()
{
    int num = this.numerator * -1;
    int den = this.denominator;
    return new Fraction(num,den);
}

An example to make your class immutable:

public class Fraction {
    //marking the fields as final in order to only be initialized
    //in class constructor
    private final int numerator;
    private final int denominator;

    public Fraction() {
        //always initialize the fields in constructor
        numerator = 0;
        denominator = 1; //because it cannot be zero
    }

    /**
     * Constructor with two int parameters
     * @param num is numerator
     * @param den is denominator
     */
    public Fraction(int num, int den) {
        if (den==0) {
            //denominator cannot be zero
            //it is better to throw an exception than just returning
            throw new IllegalArgumentException("Denominator cannot be zero");
        }
        int[] fractionData = this.normalize(num, den);
        //always initialize the fields in constructor
        this.numerator = fractionData[0];
        this.denominator = fractionData[1];
    }

    private int[] normalize(int numParam, int denParam) {
        int[] fractionData = new int[2];
        fractionData[0] = numParam;
        fractionData[1] = denParam;
        BigInteger gcd;
        BigInteger num = BigInteger.valueOf(numParam);
        BigInteger den = BigInteger.valueOf(denParam);
        gcd = num.gcd(den);
        if (gcd.intValue() > 1) {
            fractionData[0] = numParam / gcd.intValue();
            fractionData[1] = denParam / gcd.intValue();
        }
        return fractionData;
    }

    //leaving the rest of the implementation up to you...
}
Luiggi Mendoza
  • 85,076
  • 16
  • 154
  • 332
  • Hi Luiggi. Thanks for your comments, but I think Peter's answer is slightly neater as I my testNegate() essentially failed because my Negate() was not designed well enough. I have taken on board the Math#abs suggestion. Best Regards. Greg. – user3274207 Feb 05 '14 at 14:35
  • @user3274207 as noted by almost all the implementation details of your `Fraction` class, looks like it is immutable like `String` (except that you forgot to mark `numerator` and `denominator` fields as `final`), so when realizing every operation you end by retrieving a complete new instance of `Fraction`. In Peter's answer, his implementation breaks the immutability of your class, that's why I just limited my answer to remove the `Math#abs` and return a complete new `Fraction` instance. See here for [immutability](http://docs.oracle.com/javase/tutorial/essential/concurrency/immutable.html) – Luiggi Mendoza Feb 05 '14 at 15:09
  • @user3274207 about this comment *creating new instances is always costly*: this is true for objects that can take lot of memory (RAM) e.g. creating a `Workbook` from Apache POI to read a 10MB (or more) Excel file. For this case, the *cost* of creating a new `Fraction` object instance can be ignored, note that **premature optimization is the root of all evil**: http://programmers.stackexchange.com/q/80084 http://stackoverflow.com/q/385506/1065197 http://ubiquity.acm.org/article.cfm?id=1513451 – Luiggi Mendoza Feb 05 '14 at 15:17
  • Hi Luiggi, thanks for your comment. In fact as you mentioned, Peter's answer breaks the immutability and my Fractions should be immutable. So I'm going with your answer now! The same with the cost, creating an int object doesn't really take that much memory. Many Thanks for following this thing up and explaining this to me. – user3274207 Feb 09 '14 at 11:46
  • I have an issue with initializing numerator and denominator fields as final.When I do so,Eclipse complains with every method:'The blank final field numerator may not have been initialized' or 'The final field Fraction.Numerator (or Denominator) cannot be assigned and suggests to change it back. – user3274207 Feb 09 '14 at 12:04
  • @user3274207 when you declare your fields as `final` you can only assign them **once** in the class constructor, but your `normalize` method also tries to change their values. – Luiggi Mendoza Feb 09 '14 at 20:58
  • Hi Luiggi, thanks for getting back to me. Each constructor throws a compilation error.Other methods compile after I initialized both numerator and denominator to zero.I don't quite understand how can I initialize only once as I have to assign a value to it every time I construct an instance of Fraction.In the tutorials `ImmutableRGB(int red, int green, int blue, String name) { check(red, green, blue); this.red = red; this.green = green; this.blue = blue; this.name = name; }`isOK – user3274207 Feb 09 '14 at 21:56
  • Continuing. Event hough `final private int red; final private int green; final private int blue; final private String name;`@Luiggi if I just make the whole class final `final public class Fraction` would this achieve immutability? – user3274207 Feb 09 '14 at 22:05
  • @user3274207 updated my answer to show how can you design your class as immutable. If you mark the class as `final`, it means that it cannot be extended by any class e.g. `String`, `Integer`. – Luiggi Mendoza Feb 10 '14 at 00:40
1

I would not create a new instance of Fraction unless that's really needed.

public Fraction negate()
{
    this.numerator *= -1;
    return this;
}

Also, I don't think you need BigInteger in normalize().

I would implement it something like this.

private void normalize()
{
    int gcd = gcd(this.numerator, this.denominator);
    if (gcd > 1)
    {
        this.numerator = this.numerator / gcd;
        this.denominator = this.denominator / gcd;
    }
    if (this.denominator < 0){
        this.numerator *= -1;
        this.denominator *= -1;
    }
}
peter.petrov
  • 38,363
  • 16
  • 94
  • 159