6
\$\begingroup\$

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.

\$\endgroup\$
  • 2
    \$\begingroup\$ Please put your code through a formatter to reformat it according to some standard. That makes it more readable and gives you an idea of how to write code in a more standardised style. tutorialspoint.com/online_java_formatter.htm \$\endgroup\$ – user985366 Sep 16 at 1:17
  • \$\begingroup\$ I was not aware of the existence of such a website. Thank you very much. \$\endgroup\$ – Khashayar Baghizadeh Sep 16 at 9:05
  • \$\begingroup\$ Not sure why you wrote a recursive GCD implementation when Java doesn't support tail recursion. Granted, you are not likely to do so many recursion steps that you overrun the stack but still, an iterative (loop-based) method would be preferred in Java. \$\endgroup\$ – dumetrulo Sep 17 at 13:08
  • \$\begingroup\$ @dumetrulo There wasn't any particular reason (maybe it was just faster to write) and I absolutely agree with you. I was just more focused on the data-type implementation which I am currently learning. \$\endgroup\$ – Khashayar Baghizadeh Sep 18 at 12:18
  • 1
    \$\begingroup\$ This question reminded me that there is a very similar question in the SICP book, and I wrote an implementation of a Rational module in F# some time ago. It is less object-oriented but otherwise looks quite similar to your Java implementation. \$\endgroup\$ – dumetrulo Sep 18 at 12:22
9
\$\begingroup\$

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);
    }
}
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ I always find your suggestions very helpful. Thank you very much. :) \$\endgroup\$ – Khashayar Baghizadeh Sep 16 at 9:07
  • 1
    \$\begingroup\$ Very nice review. Only thing that bothers me is the naming of the parameters, because of the type you already know its a Rational, so why add it to the name as well? It makes it a bit too verbose imho. \$\endgroup\$ – RobAu Sep 18 at 14:04
  • \$\begingroup\$ @RobAu thanks. You are totally right, but I was too lazy to rename the variables. I hope OP will remember your suggestion for the next time. \$\endgroup\$ – Marc 11 hours ago
4
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
3
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Kudos for showing alternatives, but you're right, "almost all of it is highly opinion-based". E.g. using "var" makes code less verbose, but hides the exact data type, declaring "final" helps to avoid mistakes, but makes code more verbose. \$\endgroup\$ – Ralf Kleberhoff yesterday

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.