8
\$\begingroup\$

As my first Java homework, I wrote a class to handle fractions. This Fraction class implements basic operations for the fractions (addition, subtraction, multiplication and division) and follows this interface*:

interface FractionInterface {
    public int getNumerator();
    public int getDenominator();

    public Fraction add(Fraction f);
    public Fraction sub(Fraction f);
    public Fraction mult(Fraction f);
    public Fraction div(Fraction f);
    public int comparesTo(Fraction f);
}

This interface doesn't actually exist, it's just a way to illustrate how the class works so I don't have to add the actual implementation.

Even though the code is incredibly easy and straightforward, I decided to, as an exercise, write a few unit tests using JUnit. The tests should verify that:

  • All operations work correctly.
  • All fractions should be simplified. (E.g: 5/10 -> 1/2)
  • An ArithmeticException is thrown when the denominator is set to 0.
  • Only the numerator can be negative, if the fraction is negative.

Even though it works, I wasn't very pleased with the final result. It doesn't feel very clean, specially the way I stores the predefined values, and I don't really know what else could be improved. That's how it looks:

public class FractionTest {

    @Test (expected = ArithmeticException.class)
    public void divisionByZeroShouldThrow() {
        @SuppressWarnings("unused")
        Fraction f = new Fraction(1, 0);
    }

    @Test
    public void fractionsShouldBeSimplified() {
        int tests[][] = {
            {36, 90, 2, 5},
            {83, 75, 83, 75},
            {18, 86, 9, 43},
            {72, 52, 18, 13},
            {10, 37, 10, 37},
            {99, 45, 11, 5},
            {54, 58, 27, 29},
            {61, 61, 1, 1},
            {46, 36, 23, 18},
            {96, 93, 32, 31}
        };

        for (int i = 0; i < tests.length; i++) {
            Fraction f = new Fraction(tests[i][0], tests[i][1]);

            assertEquals(tests[i][2], f.getNumerator());
            assertEquals(tests[i][3], f.getDenominator());
        }
    }

    @Test
    public void negativeFractionsShouldHaveTheNominatorNegative() {
        int tests[][] = {
            {-35, 3, -35, 3},
            {3, -5, -3, 5}
        };

        for (int i = 0; i < tests.length; i++) {
            Fraction f = new Fraction(tests[i][0], tests[i][1]);

            assertEquals(tests[i][2], f.getNumerator());
            assertEquals(tests[i][3], f.getDenominator());
        }
    }

    @Test
    public void additionsShouldWork() {
        int tests[][] = {
            {36, 90, 83, 75, 113, 75},
            {18, 86, 72, 52, 891, 559},
            {10, 37, 99, 45, 457, 185},
            {54, 58, 61, 61, 56, 29},
            {46, 36, 96, 93, 1289, 558}
        };

        for (int i = 0; i < tests.length; i++) {
            Fraction f1 = new Fraction(tests[i][0], tests[i][1]);
            Fraction f2 = new Fraction(tests[i][2], tests[i][3]);

            Fraction f = f1.add(f2);

            assertEquals(tests[i][4], f.getNumerator());
            assertEquals(tests[i][5], f.getDenominator());
        }
    }

    @Test
    public void subtractionsShouldWork() {
        int tests[][] = {
            {36, 90, 83, 75, -53, 75},
            {18, 86, 72, 52, -657, 559},
            {10, 37, 99, 45, -357, 185},
            {54, 58, 61, 61, -2, 29},
            {46, 36, 96, 93, 137, 558}
        };

        for (int i = 0; i < tests.length; i++) {
            Fraction f1 = new Fraction(tests[i][0], tests[i][1]);
            Fraction f2 = new Fraction(tests[i][2], tests[i][3]);

            Fraction f = f1.sub(f2);

            assertEquals(tests[i][4], f.getNumerator());
            assertEquals(tests[i][5], f.getDenominator());
        }
    }

    @Test
    public void multiplicationsShouldWork() {
        int tests[][] = {
            {36, 90, 83, 75, 166, 375},
            {18, 86, 72, 52, 162, 559},
            {10, 37, 99, 45, 22, 37},
            {54, 58, 61, 61, 27, 29},
            {46, 36, 96, 93, 368, 279}
        };

        for (int i = 0; i < tests.length; i++) {
            Fraction f1 = new Fraction(tests[i][0], tests[i][1]);
            Fraction f2 = new Fraction(tests[i][2], tests[i][3]);

            Fraction f = f1.mult(f2);

            assertEquals(tests[i][4], f.getNumerator());
            assertEquals(tests[i][5], f.getDenominator());
        }
    }

    @Test
    public void divisionsShouldWork() {
        int tests[][] = {
            {36, 90, 83, 75, 30, 83},
            {18, 86, 72, 52, 13, 86},
            {10, 37, 99, 45, 50, 407},
            {54, 58, 61, 61, 27, 29},
            {46, 36, 96, 93, 713, 576}
        };

        for (int i = 0; i < tests.length; i++) {
            Fraction f1 = new Fraction(tests[i][0], tests[i][1]);
            Fraction f2 = new Fraction(tests[i][2], tests[i][3]);

            Fraction f = f1.div(f2);

            assertEquals(tests[i][4], f.getNumerator());
            assertEquals(tests[i][5], f.getDenominator());
        }
    }

    @Test
    public void comparisonsShouldWork() {
        int tests[][] = {
            {36, 90, 83, 75, -1},
            {18, 86, 72, 52, -1},
            {10, 37, 99, 45, -1},
            {54, 58, 61, 61, -1},
            {46, 36, 96, 93, 1},
            {36, 6, 6, 1, 0}
        };

        for (int i = 0; i < tests.length; i++) {
            Fraction f1 = new Fraction(tests[i][0], tests[i][1]);
            Fraction f2 = new Fraction(tests[i][2], tests[i][3]);

            assertEquals(tests[i][4], f1.comparesTo(f2));
        }
    }
}
\$\endgroup\$
4
  • \$\begingroup\$ Fraction is an interface, not a class, yet you have new Fraction(....) calls throughout your tests. Additionally, the Fraction interface has methods getNumerator and getDenominator, but your tests call getNumerador and getDenominador. Your question is poorly put together and is misleading. \$\endgroup\$ – rolfl Mar 7 '15 at 20:48
  • 5
    \$\begingroup\$ @rolfl I'm pretty sure that's because the interface isn't part of the actual code, but just created for us to illustrate the class, so the OP doesn't have to post the complete Fraction class. And other than that, I think the question is phrased quite well. \$\endgroup\$ – tim Mar 7 '15 at 21:00
  • 1
    \$\begingroup\$ Yes, as @tim pointed out, the interface is just so I didn't need to bother you with stuff that's not really needed for the review. And the d/t thing was just a typo. I've edited the question to fix the typo and explain the interface. \$\endgroup\$ – rmobis Mar 7 '15 at 21:12
  • 1
    \$\begingroup\$ It's wonderful to see a beginner writing unit tests! Keep it up! \$\endgroup\$ – Boris the Spider Mar 8 '15 at 16:40
10
\$\begingroup\$

Instead of suppressing the warning here for the unused variable f:

@Test (expected = ArithmeticException.class)
public void divisionByZeroShouldThrow() {
    @SuppressWarnings("unused")
    Fraction f = new Fraction(1, 0);
}

You could just omit the local variable completely:

@Test (expected = ArithmeticException.class)
public void divisionByZeroShouldThrow() {
    new Fraction(1, 0);
}

Instead of comparing the values of the getters like this:

Fraction f = new Fraction(tests[i][0], tests[i][1]);

assertEquals(tests[i][2], f.getNumerador());
assertEquals(tests[i][3], f.getDenominador());

It would be simpler and more intuitive to compare Fraction objects directly (assuming the class implements the equals method properly):

Fraction f = new Fraction(tests[i][0], tests[i][1]);
Fraction expected = new Fraction(tests[i][2], tests[i][3]);

assertEquals(expected, f);

Except, in the case when the behavior of .equals itself would be called into question, for example when verifying that 6/8 gets simplified properly to 3/4. To test that new Fraction(6, 8) becomes 3/4, you cannot use .equals, you need the explicit assertions on the numerator and the denominator being 3 and 4, respectively.

\$\endgroup\$
5
  • \$\begingroup\$ Thanks! I had no idea you could instantiate an object without assigning it to a variable. \$\endgroup\$ – rmobis Mar 7 '15 at 21:29
  • 1
    \$\begingroup\$ Note though that using the object's equals may not be desirable in tests if, for instance, it equates 3/4 and 6/8. \$\endgroup\$ – WaelJ Mar 8 '15 at 4:14
  • \$\begingroup\$ 6/8 should get simplified to 3/4. It should be verified with an appropriate test case. Then using equals should be fine. Generally speaking, when using equals is not desirable for some reason, I would consider the design itself not desirable, and the design should be fixed. \$\endgroup\$ – janos Mar 8 '15 at 6:08
  • \$\begingroup\$ @janos this is the appropriate test case that verifies that 6/8 gets simplified to 3/4 - and you proposed making it do something else instead. (Specifically, your suggestion doesn't test whether getNumerador and getDenominador return the simplified fraction or not) \$\endgroup\$ – user253751 Mar 8 '15 at 8:19
  • \$\begingroup\$ @immibis you're right, that wasn't very clear. I updated my post to clarify this. \$\endgroup\$ – janos Mar 8 '15 at 8:24
5
\$\begingroup\$

You could create the fraction objects inside the test array:

        {new Fraction(2, 5), 36, 90},
        {new Fraction(83, 75), 83, 75},

        {new Fraction(36, 90), new Fraction(83, 75), 30, 83},
        {new Fraction(18, 86), new Fraction(72, 52), 13, 86},

Which still looks quite readable, and might be a bit clearer than what you have (because it's more obvious which is the input to be tested and which is the expected result).

Either way, I would add comments on the top of each array entry, so it's immediately clear what it contains:

        // the following represents f1 + f2 = result (each with two entries for numerator/denominator)
        {36, 90, 83, 75, 113, 75},
        {18, 86, 72, 52, 891, 559},

Or add one explicit comments for the first line:

        {36, 90, 83, 75, 113, 75},  // (36 / 90) * (83 / 75) = (113 / 75)
        {18, 86, 72, 52, 891, 559},

But in general I think it's also fine the way you have it. It's not the most readable, but adding too much complexity to have nicer unit tests doesn't seem like a good idea.

\$\endgroup\$
3
  • \$\begingroup\$ To use the Fraction objects inside the array, along with the ints, I'd have to declare it as Object[][] and then cast it later, right? For the comparisonsShouldWork method, I mean. \$\endgroup\$ – rmobis Mar 7 '15 at 21:31
  • 1
    \$\begingroup\$ @Raphael_ hm, yes, that's true. And that might get a bit ugly. One cleaner solution would be to just split that test up in something like testComparisonEquals, testComparisonSmaller, and testComparisonLarger. \$\endgroup\$ – tim Mar 7 '15 at 21:41
  • \$\begingroup\$ Hmm, that's a great solution. Guess I'll go for it. Thanks. \$\endgroup\$ – rmobis Mar 7 '15 at 21:51

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.