# 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


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.
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.
• adding Case < 1 at the start, and Case > 3999 just before the case else should help with boundaries Jan 12, 2015 at 21:28