nosuchblogger

This is my first attempt at solving the Roman Numerals kata. My goal isn't to come up with the smallest or fastest implementation, but I am aiming for "simplicity" in that the code should be easily understandable. I'm following a TDD approach and one of the things I really enjoy while doing these katas is that the solution is always different in sometimes large ways. It's often those many little decisions that we all make while coding that can sometimes vastly influence the final outcome. And occasionally there are those larger leaps which greatly advance the program – which don't happen too often in my case!

Anyway, enough with the talk, let's begin. I'll start by focussing on the conversion from Arabic to Roman numerals first. The first test being a simple 1 -> I conversion:

[TestFixture]
public class RomanNumeralConverterTest
{
  [Test]  
  public void When_passed_1_should_convert_to_i()
  {
    Assert.AreEqual("I", RomanNumeralConverter.Convert("1"));
  }
}

And the code to get this to pass:

public class RomanNumeralConverter
{
  public static string Convert(string arabicOrRomanNumeral)
  {
    return "I";
  }
}

Next test is to simply extend this to 2:

[Test]
public void When_passed_2_should_convert_to_ii()
{
  Assert.AreEqual("II", RomanNumeralConverter.Convert("2"));
}

I could simply change to implementation by adding an if (arabicOrRomanNumeral == "2") clause, but went one step further at this point and introduced a loop for the number passed in to write out the equivalent number of "I" characters:

var arabic = int.Parse(arabicOrRomanNumeral);
var romanNumerals = new StringBuilder();
for (var i = 0; i < arabic; i++)
  romanNumerals.Append('I');
return romanNumerals.ToString();

I also added a similar test (which passes without modifying the implementation) for 3. The next, 4, fails:

[Test]
public void When_passed_4_should_convert_to_iv()
{
  Assert.AreEqual("IV", RomanNumeralConverter.Convert("4"));
}

Now, this I realise, is going to take a bit of a leap, so I decide to add an ignore attribute (I could delete or comment the test out, but nunit flags ignored tests which I like as it acts as a little reminder that I need to come back to this later). I plan on moving onto 5, then 6, at which point I should be able to come back to this test.

[Test]
public void When_passed_5_should_convert_to_v()
{
  Assert.AreEqual("V", RomanNumeralConverter.Convert("5"));
}

This fails (IIIII is returned). Now at this point, I feel I need to re-think the implementation. Refactoring time! I decide I want a class to represent a Roman numeral which holds the character and the Arabic value:

public class RomanNumeral
{
  public readonly char Numeral;
  public readonly int ArabicValue;
  public RomanNumeral(char numeral, int arabicValue)
  {
    this.Numeral = numeral;
    this.ArabicValue = arabicValue;
  }
}

In the converter (not sure this is the best place, so I may move this later) I create an array of the two numerals I need for 1 to 5:

static List<RomanNumeral> RomanNumerals = new List<RomanNumeral> {
  new RomanNumeral('V', 5),
  new RomanNumeral('I', 1)
};

Next, in the implementation, I want to return a list of the roman numerals for the Arabic value instead of just appending characters to a string. I create a new convert instance method:

private readonly int arabic;
public RomanNumeralConverter(int arabic)
{
  this.arabic = arabic;
}
public IList<RomanNumeral> ToRomanNumerals()
{
  var romanNumerals = new List<RomanNumeral>();
  for (var i = 0; i < this.arabic; i++)
    romanNumerals.Add(RomanNumerals[1]);
  return romanNumerals;
}

The main convert method changes to the following:

public static string Convert(string arabicOrRomanNumeral)
{
  var arabic = int.Parse(arabicOrRomanNumeral);
  return new RomanNumeralConverter(arabic)
      .ToRomanNumerals()
      .Aggregate(new StringBuilder(),
        (sb, romanNumeral) => sb.Append(romanNumeral.Numeral))
      .ToString();
}

Now we’re compiling again, but the "5" test is still failing. As an aside, I again added an ignore attribute on the "5" test while refactoring. As a rule, you want to be green whilst refactoring. The next effort has to be within the ToRomanNumerals method. The plan at this stage is to pick the highest possible valued numeral, then subtract that value from the running total until we get to zero. This is what I get:

public IList<RomanNumeral> ToRomanNumerals()
{
  var romanNumerals = new List<RomanNumeral>();
  var number = this.arabic;

  while (number > 0)
  {
    var romanNumeral = RomanNumerals.First(x => x.ArabicValue <= number);
    number -= romanNumeral.ArabicValue;
    romanNumerals.Add(romanNumeral);
  }
  return romanNumerals;
}

This now makes all the tests pass - ignoring the test for "4" which is still disabled. I add an additional set of tests for the numbers 6 to 8 which pass. Now is the time to tackle that test for "4". First of all, I think this is the time for some refactoring to enable the subtractive principle. If a lower-valued numeral appears before a higher base, then the lower value is subtracted from the higher – i.e. IV is used instead of IIII. However, there are some rules. For example, I can only be used up to X (10), so that 49 isn't written as IL but as XLIX (40 + 9). So I'm thinking the RomanNumeral class is the place to put this rule. I add the following to the class:

private readonly RomanNumeral canBeModifiedBy;
public RomanNumeral(string numeral, int arabicValue) : this(numeral, arabicValue, null) { }
public RomanNumeral(string numeral, int arabicValue, RomanNumeral canBeModifiedBy)
{
  this.Numeral = numeral;
  this.ArabicValue = arabicValue;
  this.canBeModifiedBy = canBeModifiedBy;
}

I also take this opportunity to move the array of numerals out of the converter:

public class RomanNumerals
{
  public static RomanNumeral I = new RomanNumeral("I", 1);
  public static RomanNumeral V = new RomanNumeral("V", 5, I);

  public static IEnumerable<RomanNumeral> All = new List<RomanNumeral>
  {
    V, I
  };
}

The method in the converter, ToRomanNumerals, currently picks the highest numeral for the current value. This will have to change slightly as it's now possible that given the substitution logic, a match may be possible. So, initially I move the "matching" logic into the RomanNumeral class:

public RomanNumeral Matches(int arabicValue)
{
  if (ArabicValue <= arabicValue)
    return this;

  return null;
}

The ToRomanNumerals method then becomes:

public IList<RomanNumeral> ToRomanNumerals()
{
  var romanNumerals = new List<RomanNumeral>();
  var number = this.arabic;

  while (number > 0)
  {
    var romanNumeral = (from numeral in RomanNumerals.All
                        where numeral.Matches(number) != null
                        select numeral.Matches(number)).First();
    number -= romanNumeral.ArabicValue;
    romanNumerals.Add(romanNumeral);
  }
  return romanNumerals;
}

Getting close now - the test is still failing, but it should be just a matter of adding the substitution into the Matches method:

public RomanNumeral Matches(int arabicValue)
{
  if (ArabicValue <= arabicValue)
    return this;

  if (canBeModifiedBy != null && this.ArabicValue - canBeModifiedBy.ArabicValue <= arabicValue)
    return new RomanNumeral(canBeModifiedBy.Numeral + this.Numeral, this.ArabicValue - canBeModifiedBy.ArabicValue);

  return null;
}

And success! "4" is now returning IV. I now add a test for a wide range of numbers to see if the implementation is correct:

[Test]
public void When_passed_arabics_should_convert_to_roman_numerals()
{
  Assert.AreEqual("CCXCI", RomanNumeralConverter.Convert("291"));
  Assert.AreEqual("XLIX", RomanNumeralConverter.Convert("49"));
  Assert.AreEqual("CDXC", RomanNumeralConverter.Convert("490"));
  Assert.AreEqual("CML", RomanNumeralConverter.Convert("950"));
  Assert.AreEqual("CMXCIX", RomanNumeralConverter.Convert("999"));
}

This didn't pass. Finally need to just add the additional numerals and that should fix it.

public class RomanNumerals
{
  public static RomanNumeral I = new RomanNumeral("I", 1);
  public static RomanNumeral V = new RomanNumeral("V", 5, I);
  public static RomanNumeral X = new RomanNumeral("X", 10, I);
  public static RomanNumeral L = new RomanNumeral("L", 50, X);
  public static RomanNumeral C = new RomanNumeral("C", 100, X);
  public static RomanNumeral D = new RomanNumeral("D", 500, C);
  public static RomanNumeral M = new RomanNumeral("M", 1000, C);

  public static IEnumerable<RomanNumeral> All = new List<RomanNumeral>
  {
    M, D, C, L, X, V, I
  };
}

Yes, all tests are green. That's it for the first part; the conversion of Arabic to Roman Numeral. Here's all the code in one chunk showing the solution at this stage.

The next part is to implement the reverse, where the convert method receives a Roman numeral and convert that to Arabic. Will this implementation need significant re-factoring? Was it a good decision to implement fully one part of problem before tackling the other part? Perhaps some answers in the next part.