Converting to Roman Numeral with Recursive Algorithm

Question

Summary

I'm getting ready to dive back into a "more proper" project, so I wanted to take a moment to get my TDD hat on before doing so. I decided to tackle this Roman Numeral Kata for practice. The purpose of the function is simply to take in a Long and output a string containing the equivalent Roman Numeral.

I'm confident that it's accurate out to 3999. Beyond that a standard keyboard doesn't quite have the proper characters, so 4000 comes out as MMMM and 14841 comes out as MMMMMMMMMMMMMMDCCCXLI. Unit Tests can be found in this Gist if you're interested.

I didn't plan on doing this recursively. It just kind of came out naturally after a few refactorings. I did at one point extract a few functions to reduce the redundancy in the code below, but it really felt like it obfuscated the algorithm and hurt the clarity.

Questions

• Was the recursive solution a good way to go? I feel like it's clear, but I have this nagging feeling there might be a better algorithm. (One that may involve determining decimal places perhaps?)

• Is it dry enough as it is, or should I have extracted a few functions like this.

  Private Function UpFrom(ByVal number As Long, ByVal from As Long) As String
   UpFrom = ConvertToRomanNumeral(from) & ConvertToRomanNumeral(number - from)
  End Function
  

• Should I have raised an error after reaching 4000, instead of returning "numeral like" strings?

• Did I do anything else stupid or dirty? Anything you'd do differently?

Numbers.bas

Public Function ConvertToRomanNumeral(ByVal number As Long) As String
 Dim result As String
 Select Case number
 Case 1
 result = "I"
 Case 2 To 3
 result = ConvertToRomanNumeral(number - 1) & ConvertToRomanNumeral(1)
 Case 4
 result = ConvertToRomanNumeral(1) & ConvertToRomanNumeral(5)
 Case 5
 result = "V"
 Case 6 To 8
 result = ConvertToRomanNumeral(5) & ConvertToRomanNumeral(number - 5)
 Case 9
 result = ConvertToRomanNumeral(1) & ConvertToRomanNumeral(10)
 Case 10
 result = "X"
 Case 11 To 39
 result = ConvertToRomanNumeral(10) & ConvertToRomanNumeral(number - 10)
 Case 40
 result = ConvertToRomanNumeral(10) & ConvertToRomanNumeral(50)
 Case 41 To 49
 result = ConvertToRomanNumeral(40) & ConvertToRomanNumeral(number - 40)
 Case 50
 result = "L"
 Case 51 To 89
 result = ConvertToRomanNumeral(50) & ConvertToRomanNumeral(number - 50)
 Case 90
 result = ConvertToRomanNumeral(10) & ConvertToRomanNumeral(100)
 Case 91 To 99
 result = ConvertToRomanNumeral(90) & ConvertToRomanNumeral(number - 90)
 Case 100
 result = "C"
 Case 101 To 399
 result = ConvertToRomanNumeral(100) & ConvertToRomanNumeral(number - 100)
 Case 400
 result = ConvertToRomanNumeral(100) & ConvertToRomanNumeral(500)
 Case 401 To 499
 result = ConvertToRomanNumeral(400) & ConvertToRomanNumeral(number - 400)
 Case 500
 result = "D"
 Case 501 To 899
 result = ConvertToRomanNumeral(500) & ConvertToRomanNumeral(number - 500)
 Case 900
 result = ConvertToRomanNumeral(100) & ConvertToRomanNumeral(1000)
 Case 901 To 999
 result = ConvertToRomanNumeral(900) & ConvertToRomanNumeral(number - 900)
 Case 1000
 result = "M"
 Case Else
 result = ConvertToRomanNumeral(1000) & ConvertToRomanNumeral(number - 1000)
 End Select
 ConvertToRomanNumeral = result
End Function

Answer 1

Not to be negative, but...

Your Else case is making a bad assumption, that whichever number reaches that branch is going to be greater than 1000. I don't mind the recursion; I don't even mind the Select Case - but by not handling negative integers you have a bug there, where -42 is going to be treated as if it were greater than 1000, and even worse, it's going to be subtracted 1000 before being fed into a recursive call: I haven't executed it, but it seems your code would blow up with a very easily avoidable overflow error with just about any negative input.

There should be an input validation guard clause before the select block, to ensure you're not entering a recursive logic with a zero or negative input.

Now if valid input is an integer between 1 and 3999, then there is no need to intake a Long - the Integer data type is sufficient.

• RubberDuck: Yup. I think that qualifies as "something stupid". Thanks. =)
• SeanC: adding Case < 1 at the start, and Case > 3999 just before the case else should help with boundaries