Implementing common fraction using TDD

Question

Recently, during the interview, I was asked to implement basic arithmetic operations with common fractions using TDD. The task is fairly simple, but I was nervous so I did not do too well. :) Later on I decided to give it another try in more... comfortable environment. Here is what I came up with.

Tests:

[TestFixture]
class FractionTests
{
    [Test]
    public void NormalizationTest()
    {
        var fraction = new Fraction(24, 36).Normalize();
        Assert.That(fraction.Numerator, Is.EqualTo(2));
        Assert.That(fraction.Denominator, Is.EqualTo(3));
    }

    [Test]
    public void EqualityTest()
    {
        Assert.That(new Fraction(1, 2), Is.EqualTo(new Fraction(1, 2)));
        Assert.That(new Fraction(5, 10), Is.EqualTo(new Fraction(1, 2)));
    }

    [Test]
    public void ComparisonTest()
    {
        Assert.That(new Fraction(5, 10).CompareTo(new Fraction(1, 2)), Is.EqualTo(0));
        Assert.That(new Fraction(5, 10).CompareTo(new Fraction(9, 4)), Is.EqualTo(-1));
        Assert.That(new Fraction(5, 10).CompareTo(new Fraction(1, 5)), Is.EqualTo(1));
    }

    [Test]
    public void AdditionTestWithSameDenominators()
    {
        var fraction1 = new Fraction(1,5);
        var fraction2 = new Fraction(7,5);
        var result = fraction1.Add(fraction2);
        Assert.That(result, Is.EqualTo(new Fraction(8, 5)));
    }

    [Test]
    public void AdditionTestWithDifferentDenominators()
    {
        var fraction1 = new Fraction(1, 25);
        var fraction2 = new Fraction(6, 15);
        var result = fraction1.Add(fraction2);
        Assert.That(result, Is.EqualTo(new Fraction(33, 75)));
    }

    [Test]
    public void SubtractionTestWithSameDenominators()
    {
        var fraction1 = new Fraction(1, 5);
        var fraction2 = new Fraction(7, 5);
        var result = fraction1.Subtract(fraction2);
        Assert.That(result, Is.EqualTo(new Fraction(-6, 5)));
    }

    [Test]
    public void SubtractionTestWithDifferentDenominators()
    {
        var fraction1 = new Fraction(1, 25);
        var fraction2 = new Fraction(6, 15);
        var result = fraction2.Subtract(fraction1);
        Assert.That(result, Is.EqualTo(new Fraction(27, 75)));
    }

    [Test]
    public void MultiplicationTest()
    {
        var fraction1 = new Fraction(2, 3);
        var fraction2 = new Fraction(3, 5);
        var result = fraction1.Multiply(fraction2);
        Assert.That(result, Is.EqualTo(new Fraction(6, 15)));
    }

    [Test]
    public void DivisionTest()
    {
        var fraction1 = new Fraction(2, 3);
        var fraction2 = new Fraction(3, 5);
        var result = fraction2.Divide(fraction1);
        Assert.That(result, Is.EqualTo(new Fraction(9, 10)));
    }

    [Test]
    public void TestZeroHandling()
    {
        var fraction = new Fraction(2,3);
        var zero = new Fraction(0, 29);

        Assert.That(fraction.Add(zero), Is.EqualTo(fraction));
        Assert.That(fraction.Subtract(zero), Is.EqualTo(fraction));
        Assert.That(fraction.Multiply(zero), Is.EqualTo(zero));
        Assert.That(() => fraction.Divide(zero), Throws.TypeOf<DivideByZeroException>());
    }
}

I am uncertain, if those tests are good enough. I am also fairly new to TDD, so any advice is welcome.

Fraction class:

public class Fraction : IEquatable<Fraction>, IComparable<Fraction>
{
    public Fraction(int numerator = 0, int denominator = 1)
    {
        if (denominator < 1) throw new ArgumentOutOfRangeException("denominator", "Denominator should be greater than 0");

        Numerator = numerator;
        Denominator = denominator;
    }

    public int Numerator { get; private set; }
    public int Denominator { get; private set; }

    public Fraction Add(Fraction other)
    {
        if (other == null) throw new ArgumentNullException("other");
        Fraction result;
        if (other.Denominator == Denominator)
        {
            result = new Fraction(Numerator + other.Numerator, Denominator);
        }
        else
        {
            var commonDenominator = Denominator*other.Denominator;
            var numerator = Denominator*other.Numerator + other.Denominator*Numerator;
            result = new Fraction(numerator, commonDenominator);
        }
        return result.Normalize();
    }

    public Fraction Subtract(Fraction other)
    {
        if (other == null) throw new ArgumentNullException("other");
        return Add(new Fraction(-other.Numerator, other.Denominator));
    }

    public Fraction Multiply(Fraction other)
    {
        if (other == null) throw new ArgumentNullException("other");
        return new Fraction(Numerator * other.Numerator, Denominator * other.Denominator).Normalize();
    }

    public Fraction Divide(Fraction other)
    {
        if (other == null) throw new ArgumentNullException("other");
        if (other.Numerator == 0) throw new DivideByZeroException();
        return Multiply(new Fraction(other.Denominator, other.Numerator));
    }

    public Fraction Normalize()
    {
        var commonDivisor = FindCommonDivisor(Math.Abs(Numerator), Math.Abs(Denominator));
        return new Fraction(Numerator / commonDivisor, Denominator / commonDivisor);
    }

    public bool Equals(Fraction other)
    {
        if (other == null) return false;
        var first = Normalize();
        var second = other.Normalize();
        return first.Numerator == second.Numerator && first.Denominator == second.Denominator;
    }

    public override bool Equals(object obj)
    {
        return Equals(obj as Fraction);
    }

    public  override int GetHashCode()
    {
        unchecked
        {
            var normalized = Normalize();
            return (normalized.Numerator * 397) ^ normalized.Denominator;
        }
    }

    public int CompareTo(Fraction other)
    {
        return ToDouble().CompareTo(other.ToDouble());
    }

    public override string ToString()
    {
        return String.Format("{0}/{1}", Numerator, Denominator);
    }

    public double ToDouble()
    {
        return (double)Numerator/Denominator;
    }

    private static int FindCommonDivisor(int first, int second)
    {
        if (second == 0)
        {
            return first;
        }
        var mod = first % second;
        return FindCommonDivisor(second, mod);
    }
}

Anything to improve?

Answer 1

1. A denominator can be negative!

   \$\dfrac{1}{-2} = \dfrac{-1}{2}\$

You should check that the denominator isn't zero, because that's bad.

2. You could use operator overriding!

   public static Fraction operator +(Fraction fraction)
   {
       return this.Add(fraction);
   }
   

   You can do the same for all operators you have method for at the moment. (It's not an obligation, it's just cool :p)

Now, the tests:

[Test]
public void NormalizationTest()
{
    var fraction = new Fraction(24, 36).Normalize();
    Assert.That(fraction.Numerator, Is.EqualTo(2));
    Assert.That(fraction.Denominator, Is.EqualTo(3));
}

What's the purpose of your test? You normalize a Fraction, then you check if it's equal to 2/3. How does this translates into code?

[Test]
public void NormalizationTest()
{
    var expected = new Fraction(2,3);
    var actual = new Fraction(24, 36).Normalize();

    Assert.That(expected, Is.EqualTo(actual));
}

See what I did there? Now, I know what's the expected result, I know what's the tested unit (actual), and I see clearly that I'm testing for equality, not that "members are equal one to each another". That seems like the same thing, but it's not!

[Test]
public void EqualityTest()
{
    Assert.That(new Fraction(1, 2), Is.EqualTo(new Fraction(1, 2)));
    Assert.That(new Fraction(5, 10), Is.EqualTo(new Fraction(1, 2)));
}

This method tests two things. The equality, and... something else, the normalization I suppose? Your test should test one thing, one. So test the equality, that's all!

What I meant is that you shouldn't test both Equals and Normalize in the same test. You already have tests that test your Normalize method. Have one that tests your Equals method and one that would test that your Equals uses Normalize!

Now, you don't test the cases where Equals returns false. If I read only your tests, I could believe that:

new Fraction(1,10).Equals(new Fraction(2,10)) == true

Because you didn't test this. The theory implies that you should test valid cases, edge cases and invalid cases!

[Test]
public void ComparisonTest()
{
    Assert.That(new Fraction(5, 10).CompareTo(new Fraction(1, 2)), Is.EqualTo(0));
    Assert.That(new Fraction(5, 10).CompareTo(new Fraction(9, 4)), Is.EqualTo(-1));
    Assert.That(new Fraction(5, 10).CompareTo(new Fraction(1, 5)), Is.EqualTo(1));
}

Here's a cool trick. The use of [TestCase].

[Test]
[TestCase(1,2,0)]
[TestCase(9,4,-1)]
[TestCase(1,5,1)]
public void ComparisonTest(int nominator, int denominator, comparisonResult)
{
    Assert.That(new Fraction(5, 10).CompareTo(new Fraction(nominator, denominator)), Is.EqualTo(comparisonResult));
}

Cool right? Remember, a test should have only one Assert. Otherwise, either your code is bad (which isn't the case here) or your test is poorly written (which is the case here).

Last thing, you noticed how I used expected and actual in my tests? You should do the same thing!

Here's an example:

[Test]
public void AdditionTestWithSameDenominators()
{
    var expected = new Fraction(8, 5);
    var actual = new Fraction(1,5).Add(new Fraction(7,5));
    Assert.That(expected, Is.EqualTo(actual));
}

Ok, real last thing. That syntax : Assert.That(...) is weird, I feel.

Why don't you use Assert.AreEqual? I think it's much clearer what happens with this!

Answer 2

• I recommend to normalize fractions in the constructor. Otherwise you have a problem with numerator and denominator grow unnecessarily large.

• bool less() is usually required. BTW, given less(), you may infer all other comparisons in a very mechanical way.

  EDIT: Given a '<' relation, you may implement

  a > b as return b < a;

  a == b as return !(a < b) && !(b < a);

  etc, regardless of the underlying type of arguments, as long as they form a strict weak order.

• I recommend to have an unsigned denominator.

Answer 3

I find the Fraction should be a struct. This would allow you to get rid of all those ugly != null checks.

Then, I would create a new MathHelper class and put two functions there that have wider usage then only for a fraction:

public static class MathHelper
{
    public static int GreatestCommonDivisor(int a, int b)
    {
        var mod = a % b;
        while (mod != 0)
        {
            a = b;
            b = mod;
            mod = a % b;
        }
        return b;
    }

    public static int LeastCommonMultiple(int a, int b)
    {
        return (a * b) / GreatestCommonDivisor(a, b);
    }
}

---

• As others already sugested I would use real operators instead of methods... but just for everything =, !=, <, >, +, -, /, *.

• I wouldn't reduce the fraction in the constructor, it might not be always desired, but instead add a method that returns a new, Reduced fraction, with two other utility methods like Negate and Reciprocal, all returning new fractions.

• You're cheating in your CompareTo method ;-) because instead of comparing fractions like one would do you convert them to doubles instead.

• The new LeastCommonMultiple you can use to add fractions so that you have smaller divisors.

• By adding <, > operators you can get rid fo the IEquatable interface.

This will also simplify your tests that then could look more natural like this:

Assert.IsTrue(new Fraction(1, 3) + new Fraction(2, 3) == new Fraction(1, 1));
Assert.IsTrue(new Fraction(1, 2) + new Fraction(1, 3) == new Fraction(5, 6));
Assert.IsTrue(new Fraction(1, 3) < new Fraction(2, 3));
Assert.IsTrue(new Fraction(1, 3) < new Fraction(2, 4));
Assert.IsTrue(new Fraction(3, 3) == new Fraction(2, 2));

---

public struct Fraction : IComparable<Fraction>
{
    public bool Equals(Fraction other)
    {
        return Numerator == other.Numerator && Denominator == other.Denominator;
    }

    public Fraction(int numerator, int denominator)
    {
        if (denominator == 0)
        {
            throw new ArgumentOutOfRangeException(nameof(denominator), "Denominator must not be 0");
        }

        Numerator = numerator;
        Denominator = denominator;
    }

    public int Numerator { get; }

    public int Denominator { get; }

    public Fraction Reduce()
    {
        var commonDivisor = MathHelper.GreatestCommonDivisor(Math.Abs(Numerator), Math.Abs(Denominator));
        return new Fraction(Numerator / commonDivisor, Denominator / commonDivisor);
    }

    public Fraction Negate()
    {
        return new Fraction(-Numerator, Denominator);
    }

    public Fraction Reciprocal()
    {
        return new Fraction(Denominator, Numerator);
    }

    public static Fraction operator +(Fraction f1, Fraction f2)
    {
        if (f1.Denominator == f2.Denominator)
        {
            return new Fraction(f1.Numerator + f2.Numerator, f1.Denominator);
        }

        var commonDenominator = MathHelper.LeastCommonMultiple(f1.Denominator, f2.Denominator);
        var result = new Fraction(f1.Numerator * f2.Denominator + f2.Numerator * f1.Denominator, commonDenominator);

        return result;
    }

    public static Fraction operator -(Fraction f1, Fraction f2)
    {
        return f1.Negate() + f2;
    }

    public static Fraction operator *(Fraction f1, Fraction f2)
    {
        return new Fraction(f1.Numerator * f2.Numerator, f1.Denominator * f2.Denominator);
    }

    public static Fraction operator /(Fraction f1, Fraction f2)
    {
        if (f2.Numerator == 0)
        {
            throw new DivideByZeroException();
        }
        return f1 * f2.Reciprocal();
    }

    public static bool operator ==(Fraction f1, Fraction f2)
    {
        var f1r = f1.Reduce();
        var f2r = f2.Reduce();
        return f1r.Numerator == f2r.Numerator && f1r.Denominator == f2r.Denominator;
    }

    public static bool operator !=(Fraction f1, Fraction f2)
    {
        return !(f1 == f2);
    }

    public static bool operator <(Fraction f1, Fraction f2)
    {
        return f1.Reduce().CompareTo(f2.Reduce()) < 0;
    }

    public static bool operator >(Fraction f1, Fraction f2)
    {
        return f1.Reduce().CompareTo(f2.Reduce()) > 0;
    }

    public int CompareTo(Fraction other)
    {
        var result = (Numerator * other.Denominator).CompareTo(other.Numerator * Denominator);
        return result;
    }

    public override string ToString()
    {
        return $"{Numerator}/{Denominator}";
    }

    #region autogenerated 

    public override bool Equals(object obj)
    {
        if (ReferenceEquals(null, obj)) return false;
        return obj is Fraction && Equals((Fraction)obj);
    }

    public override int GetHashCode()
    {
        unchecked
        {
            return (Numerator * 397) ^ Denominator;
        }
    }

    #endregion
}

Answer 4

A lot of what I would have said has already been covered, but on that note, I agree with your comment TopinFrassi regarding normalization (specifically, I would leave it as written versus using equal, although I would suggest that if there's a clean way to do it in C# (something like assert(fraction.Numerator == 2 && fraction.Denumerator == 3), breaking up all tests so each has exactly one assert wouldn't be a bad thing.) At any rate, my main suggestions would be cover some of the corner/bad-news cases:

1. Add or subtract fractions produces zero.
2. Normalizing a fraction that is 0/N produces 0/1 (which I'm pretty sure your code does do, but as a user, it would give me a warm fuzzy that you checked for that)
3. Normalizing a fraction that is 0/-N still produces 0/1 (which I again think your code does, but I get nervous about % and negative numbers)
4. Normalizing a fraction with -A/-B
5. I would prefer you follow something like the suggestion to use structs (or another non-nullable mechanism), but if you're going to have code that checks for null, you should verify this in tests (if for no other reason than if you're using a tool that does test code coverage, this gets you close to 100% coverage)
6. On the topic of coverage, you may want to have at least a minimal test for toString and toDouble