Implementing common fraction using TDD

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()

-
A denominator can be negative!

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

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

-
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!

StackExchange Code Review Q#109311, answer score: 11