HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Converting to Roman Numeral with Recursive Algorithm

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
romanwithalgorithmrecursivenumeralconverting

Problem

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)

Solution

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.

Context

StackExchange Code Review Q#77156, answer score: 4

Revisions (0)

No revisions yet.