Data-type implementation for rational numbers

Question

This is exercise 3.2.7. from the book Computer Science An Interdisciplinary Approach by Sedgewick & Wayne:

Implement a data type for rational numbers that supports addition, subtraction, multiplication, and division.

Here is my program:

public class Rational
{
    private final int numerator;
    private final int denominator;

    public Rational(int numerator, int denominator)
    {
        this.numerator = numerator;
        this.denominator = denominator;
    }
    public int getNumerator()
    {
        return numerator;
    }
    public int getDenominator()
    {
        return denominator;
    }
    public Rational swapSigns()
    {
        if      (numerator > 0 && denominator < 0)
        {
            return new Rational(-1*numerator,-1*denominator);
        }
        else if (numerator < 0 && denominator < 0)
        {
            return new Rational(-1*numerator,-1*denominator);
        }
        else
        {
            return new Rational(numerator,denominator);
        }   
    }
    public Rational inverse()
    {
        return new Rational(denominator,numerator);
    }
    public Rational simplify()
    {
        int gcd = Number.calculateGCDRecursively(Math.abs(numerator),denominator);
        return new Rational(numerator/gcd,denominator/gcd);
    }
    public Rational add(Rational otherRational)
    {
        otherRational = otherRational.swapSigns();
        int otherNumerator = otherRational.getNumerator();
        int otherDenominator = otherRational.getDenominator();
        int newDenominator = denominator*otherDenominator;
        int newNumerator = numerator*otherDenominator+denominator*otherNumerator;
        return new Rational(newNumerator,newDenominator).simplify();
    }
    public Rational subtract(Rational otherRational)
    {
        Rational oldRational = new Rational(numerator, denominator);
        int newNumerator = -1*otherRational.getNumerator();
        Rational newRational = new Rational(newNumerator,otherRational.getDenominator());
        return oldRational.add(newRational);
    }
    public Rational multipply(Rational otherRational)
    {
        otherRational = otherRational.swapSigns();
        int otherNumerator = otherRational.getNumerator();
        int otherDenominator = otherRational.getDenominator();
        int newNumerator = numerator*otherNumerator;
        int newDenominator = denominator*otherDenominator;
        return new Rational(newNumerator,newDenominator).simplify();
    }
    public Rational divide(Rational otherRational)
    {
        Rational oldRational = new Rational(numerator, denominator);
        Rational newRational = otherRational.inverse();
        return oldRational.multipply(newRational);
    }
    public String toString()
    {
        Rational oldRational = new Rational(numerator, denominator);
        oldRational = oldRational.swapSigns();
        return oldRational.getNumerator() + "/" + oldRational.getDenominator();
    }
    public static void main(String[] args)
    {
        int numerator1 = Integer.parseInt(args[0]);
        int denominator1 = Integer.parseInt(args[1]);
        int numerator2 = Integer.parseInt(args[2]);
        int denominator2 = Integer.parseInt(args[3]);
        Rational rational1 = new Rational(numerator1,denominator1);
        Rational rational2 = new Rational(numerator2,denominator2);
        System.out.println(rational1.toString() + " plus " + rational2.toString() + " is equal to " + rational1.add(rational2).toString());
        System.out.println(rational1.toString() + " minus " + rational2.toString() + " is equal to " + rational1.subtract(rational2).toString());
        System.out.println(rational1.toString() + " times " + rational2.toString() + " is equal to " + rational1.multipply(rational2).toString());
        System.out.println(rational1.toString() + " divided by " + rational2.toString() + " is equal to " + rational1.divide(rational2).toString());
    }
}

Also I wrote the method calculateGCDRecursively as follows:

public static int calculateGCDRecursively(int p, int q)
{
    if (q == 0) return p;
    return calculateGCDRecursively(q, p % q);
}

I checked my program and it works correctly. Here are 4 different instances of it:

---

Input: 3 4 4 5
Output:
3/4 plus 4/5 is equal to 31/20
3/4 minus 4/5 is equal to -1/20
3/4 times 4/5 is equal to 3/5
3/4 divided by 4/5 is equal to 15/16

Input: 3 4 -4 5
Output:
3/4 plus -4/5 is equal to -1/20
3/4 minus -4/5 is equal to 31/20
3/4 times -4/5 is equal to -3/5
3/4 divided by -4/5 is equal to -15/16

Input: 3 4 4 -5
Output:
3/4 plus -4/5 is equal to -1/20
3/4 minus -4/5 is equal to 31/20
3/4 times -4/5 is equal to -3/5
3/4 divided by -4/5 is equal to -15/16

Input: 3 4 -4 -5
Output:
3/4 plus 4/5 is equal to 31/20
3/4 minus 4/5 is equal to -1/20
3/4 times 4/5 is equal to 3/5
3/4 divided by 4/5 is equal to 15/16

---

Is there any way that I can improve my program?

Thanks for your attention.

Answer 1

As always, nice implementation. Few suggestions about the code:

• Divide by 0 must not be allowed. Adding a check in the constructor should be enough:

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

• Reverse the sign: instead of -1*numerator you can use -numerator. It's more compact and still readable.
• Swap sign: the method swapSigns can be simplified from:

  public Rational swapSigns() {
      if (numerator > 0 && denominator < 0) {
          return new Rational(-1 * numerator, -1 * denominator);
      } else if (numerator < 0 && denominator < 0) {
          return new Rational(-1 * numerator, -1 * denominator);
      } else {
          return new Rational(numerator, denominator);
      }
  }
  

  To:

  private Rational swapSigns() {
    if (denominator < 0) {
        return new Rational(-numerator, -denominator);
    } else {
        return new Rational(numerator, denominator);
    }
  }
  

  Additionally, swapSigns seems to be used only internally by toString and before any operations. I think it makes sense to "swap the signs" in the constructor, for example:

  public Rational(int numerator, int denominator) {
    if (denominator == 0) {
        throw new ArithmeticException("Denominator cannot be 0");
    }
    // Change 3/-4 to -3/4 or -3/-4 to 3/4
    this.numerator = denominator < 0 ? -numerator : numerator;
    this.denominator = denominator < 0 ? -denominator : denominator;
  }
  

  So that swapSign can be removed.
• Code format: as @user985366 suggested, format the code to make it more readable and concise. Modern IDE like Eclipse and IntelliJ provide that function.
• Typo in the method multipply.
• Rational is immutable: which means that there is no need to create temporary objects like oldRational:

  public Rational subtract(Rational otherRational){
      Rational oldRational = new Rational(numerator, denominator);
      int newNumerator = -1*otherRational.getNumerator();
      Rational newRational = new Rational(newNumerator,otherRational.getDenominator());
      return oldRational.add(newRational);
  }
  

  Can be simplified to:

  public Rational subtract(Rational otherRational) {
      int newNumerator = -otherRational.getNumerator();
      Rational newRational = new Rational(newNumerator, otherRational.getDenominator());
      return add(newRational);
  }
  

  Similarly for multiply, divide and toString.

Testing

Testing in the main is not good practice, because the program needs to be "tested" by us looking at the output. Also it's not uncommon to see applications with hundreds or thousands of tests. In that case, testing in the main would be infeasible.

Use Junit for tests, few examples below:

import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;

public class RationalTest {
    
    @Test
    public void testAddPositive() {
        Rational a = new Rational(3, 4);
        Rational b = new Rational(4, 5);

        Rational actual = a.add(b);

        Assertions.assertEquals(31, actual.getNumerator());
        Assertions.assertEquals(20, actual.getDenominator());
    }

    @Test
    public void testAddNegative() {
        Rational a = new Rational(3, 4);
        Rational b = new Rational(-4, 5);

        Rational actual = a.add(b);

        Assertions.assertEquals(-1, actual.getNumerator());
        Assertions.assertEquals(20, actual.getDenominator());
    }

    @Test
    public void testAddNegative2() {
        Rational a = new Rational(3, 4);
        Rational b = new Rational(4, -5);

        Rational actual = a.add(b);

        Assertions.assertEquals(-1, actual.getNumerator());
        Assertions.assertEquals(20, actual.getDenominator());
    }

    @Test
    public void testSubstract() {
        Rational a = new Rational(3, 4);
        Rational b = new Rational(4, 5);

        Rational actual = a.subtract(b);

        Assertions.assertEquals(-1, actual.getNumerator());
        Assertions.assertEquals(20, actual.getDenominator());
    }

    @Test
    public void testDivideBy0() {
        Assertions.assertThrows(IllegalArgumentException.class, () -> {
            new Rational(3, 0);
        }, "Denominator cannot be 0");
    }

    //... more tests
}

Refactored Rational:

public class Rational {
    private final int numerator;
    private final int denominator;

    public Rational(int numerator, int denominator) {
        if (denominator == 0) {
            throw new IllegalArgumentException("Denominator cannot be 0");
        }
        // Change 3/-4 to -3/4 or -3/-4 to 3/4
        this.numerator = denominator < 0 ? -numerator : numerator;
        this.denominator = denominator < 0 ? -denominator : denominator;
    }

    public int getNumerator() {
        return numerator;
    }

    public int getDenominator() {
        return denominator;
    }

    public Rational inverse() {
        return new Rational(denominator, numerator);
    }

    public Rational simplify() {
        int gcd = Number.calculateGCDRecursively(Math.abs(numerator), denominator);
        return new Rational(numerator / gcd, denominator / gcd);
    }

    public Rational add(Rational otherRational) {
        int otherNumerator = otherRational.getNumerator();
        int otherDenominator = otherRational.getDenominator();
        int newDenominator = denominator * otherDenominator;
        int newNumerator = numerator * otherDenominator + denominator * otherNumerator;
        return new Rational(newNumerator, newDenominator).simplify();
    }

    public Rational subtract(Rational otherRational) {
        int newNumerator = -otherRational.getNumerator();
        Rational newRational = new Rational(newNumerator, otherRational.getDenominator());
        return add(newRational);
    }

    public Rational multiply(Rational otherRational) {
        int otherNumerator = otherRational.getNumerator();
        int otherDenominator = otherRational.getDenominator();
        int newNumerator = numerator * otherNumerator;
        int newDenominator = denominator * otherDenominator;
        return new Rational(newNumerator, newDenominator).simplify();
    }

    public Rational divide(Rational otherRational) {
        return multiply(otherRational.inverse());
    }

    public String toString() {
        return String.format("%d/%d", numerator, denominator);
    }
}

Answer 2

In addition to Marc's answer:

Add some test cases needing simplification of the result, e.g. 1/10 + 1/15 = 1/6.

If numerators or denominators reach ~50000, their products (computed in intermediate steps) will exceed the integer range, even if the simplified result fits well into that range. You can improve that by using long for the intermediate calculations and check for the integer range before creating the new Rational. Your current version will silently produce wrong results. Throwing an ArithmeticException in such a case would be better.

Answer 3

This is not a complete answer, only a couple of observations. This answer sits on top of Marc's excellent answer and further refines his code.

Here's what I did:

• The biggest change is that I changed class Rational into a record. The reduction in code is not quite as dramatic as in some of the artificial examples you have probably seen, since we're not using the auto-generated toString() and constructor, but still, we get rid of the getters and the field declarations. Plus, we get sensible implementations of hashCode() and equals() for free!
• I inlined simplify() into the constructor, so that a Rational is always simplified; no need to explicitly call simplify() in multiply() and add().
• I also inlined some of the local variables in several methods.
• I added some whitespace to give the code more room to breathe, and visually distinguish the "steps" in the methods.
• I made everything that can be final actually final.
• I used local variable type inference wherever possible.
• I renamed some local variables and method parameters.
• I renamed the calculateGcdRecursively method to just gcd. Nobody cares whether the method uses recursion, iteration, a web API call, magic pixie dust, or prints out the problem, faxes it to Malaysia, and has some guy in a garage solve it. Nor should they care, and in fact they shouldn't be able to know. That's what encapsulation is all about. Also, of course it calculates the GCD, what else would it do with it? Those two words are redundant. Really, the only word that maybe should be spelled out is GreatestCommonDivisor, but remember that code is written for an audience, and I believe that the kind of audience that reads the internal implementation of a Rational should know what a gcd is.
• I added an @Override annotation to the toString() method, for good measure.
• And some minor formatting changes.

And this is what the result looks like:

public record Rational(final int numerator, final int denominator) {
    public Rational(int numerator, int denominator) {
        if (denominator == 0) {
            throw new IllegalArgumentException("Denominator cannot be 0");
        }

        // Change 3/-4 to -3/4 or -3/-4 to 3/4
        numerator = denominator < 0 ? -numerator : numerator;
        denominator = denominator < 0 ? -denominator : denominator;

        final var gcd = gcd(Math.abs(numerator), denominator);

        this.numerator = numerator / gcd;
        this.denominator = denominator / gcd;
    }

    public Rational inverse() {
        return new Rational(denominator, numerator);
    }

    public Rational add(final Rational other) {
        final var otherNumerator = other.numerator;
        final var otherDenominator = other.denominator;

        final var newDenominator = denominator * otherDenominator;
        final var newNumerator = numerator * otherDenominator + denominator * otherNumerator;

        return new Rational(newNumerator, newDenominator);
    }

    public Rational subtract(final Rational other) {
        return add(new Rational(-other.numerator, other.denominator));
    }

    public Rational multiply(final Rational other) {
        final var otherNumerator = other.numerator;
        final var otherDenominator = other.denominator;

        final var newNumerator = numerator * otherNumerator;
        final var newDenominator = denominator * otherDenominator;

        return new Rational(newNumerator, newDenominator);
    }

    public Rational divide(final Rational other) {
        return multiply(other.inverse());
    }

    @Override public String toString() {
        return String.format("%d/%d", numerator, denominator);
    }

    private static int gcd(final int p, final int q) {
        if (q == 0) { return p; }
        return gcd(q, p % q);
    }
}

I personally find this somewhat easier to read, although all praise should go to Marc. However, I admit that almost all of it is highly opinion-based. Also, Records are an experimental feature, and thus may change in an incompatible manner, or be removed from the language completely, at any time without warning. Plus, they require ugly command line options to work.