Problem
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
ArithmeticExceptionis 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));
}
}
}
Solution
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.
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.