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.