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

Positive integer to Roman Numeral conversion function

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

Problem

For the past two hours I was searching for a built-in function for converting an integer to an equivalent Roman numeral, but it failed to achieve the target. Hence I wrote my own routine for performing this conversion. I think it needs some more clarifications.

Public Class Form1
    Dim ind() As Integer = {1, 2, 3, 4, 5, 10, 50, 100, 500, 1000}
    Dim rom() As String = {"I", "II", "III", "IV", "V", "X", "L", "C", "D", "M"}
    Dim limit As Integer = 9
    Dim output As String = ""
    Private Sub Button1_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles Button1.Click
        Dim num As Integer
        output = ""
        num = CInt(txt1.Text)
        While num > 0
            num = find(num)
        End While
        txt2.Text = output
    End Sub
    Public Function find(ByVal Num As Integer) As Integer
        Dim i As Integer = 0
        While ind(i)  0 Then
            limit = i - 1
        Else
            limit = 0
        End If
        output = output & rom(limit)
        Num = Num - ind(limit)
        Return Num
    End Function
End Class


Note: This function can only be used for range of 1-999

Solution

-
This code is bound far too tight to your form. The first thing we need to do is separate the logic for converting an integer to a roman numeral from the event procedures of the form. This will allow us to reuse this code anywhere.

Module IntegerExtensions

    Dim ind() As Integer = {1, 2, 3, 4, 5, 10, 50, 100, 500, 1000}
    Dim rom() As String = {"I", "II", "III", "IV", "V", "X", "L", "C", "D", "M"}
    Dim limit As Integer = 9
    Dim output As String = ""

    Private Function ConvertToRomanNumeral(ByVal input As Integer) As String
        output = ""
        While input > 0
            input = find(input)
        End While
        Return output
    End Function

    Private Function Find(ByVal Num As Integer) As Integer
        Dim i As Integer = 0
        While ind(i)  0 Then
            limit = i - 1
        Else
            limit = 0
        End If
        output = output & rom(limit)
        Num = Num - ind(limit)
        Return Num
    End Function

End Module

Public Class Form1
    Private Sub Button1_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles Button1.Click

        txt2.Text = IntegerExtensions.ConvertToRomanNumeral(999)        

    End Sub
End Class


-
Next we need to make sure that the argument passed to ConvertToRomanNumeral is within the accepted range of 1-999. Receiving a Index out of bounds message is not nearly as useful as knowing that ConvertToRomanNumeral doesn't accept the value you passed into it.

Private Function ConvertToRomanNumeral(ByVal input As Integer) As String

    If (input  999) Then
        Throw New ArgumentOutOfRangeException("input","Input must be in the range 1-999")
    End If

    output = ""
    While input > 0
        input = find(input)
    End While
    Return output
End Function


-
ind and rom are not very good variable names. I get that rom stands for Roman Numerals, but I have no idea what ind means. Indices maybe? Also, I like plural names for arrays. Let's replace those. ind >> indices and rom >> romanNumerals.

-
There are way too many class level variables. Limit should be strictly declared inside of Find. I also don't find it a good idea to set the value of output from the Find function. I would consider this to be a side effect, and side effects are confusing and hard to debug. The only way for me to properly scope output was to merge Find into ConvertToRomanNumeral.

Module IntegerExtensions

    Private Function ConvertToRomanNumeral(ByVal input As Integer) As String

        If (input  999) Then
            Throw New ArgumentOutOfRangeException("input","Input must be in the range 1-999")
        End If

        Dim indices() As Integer = {1, 2, 3, 4, 5, 10, 50, 100, 500, 1000}
        Dim romanNumerals() As String = {"I", "II", "III", "IV", "V", "X", "L", "C", "D", "M"}
        Dim limit As Integer = 9
        Dim output As String = ""

        While input > 0
            Dim number as Integer = input
            Dim i As Integer = 0
            While indices(i)  0 Then
                limit = i - 1
            Else
                limit = 0
            End If
            output = output & romanNumerals(limit)
            number = number - indices(limit)
            ' this was previously our return statement
            input = number                  
        End While
        Return output
    End Function

End Module


-
You may have noticed earlier that I named the module IntegerExtensions. That's because I've been planning on making this exactly that the entire time. Extension methods will allow us to call the function directly on any integer, instead of passing an integer into it. It looks a little cleaner when in use. I changed the function name to ToRomanNumberal and adjusted the ArgumentOutOfRangeException message accordingly.

Module IntegerExtensions

    
    Public Function ToRomanNumeral(ByVal input As Integer) As String

        If (input  999) Then
            Throw New ArgumentOutOfRangeException("input","Cannot convert integers outside of the range 1-999")
        End If

        Dim indices() As Integer = {1, 2, 3, 4, 5, 10, 50, 100, 500, 1000}
        Dim romanNumerals() As String = {"I", "II", "III", "IV", "V", "X", "L", "C", "D", "M"}
        Dim limit As Integer = 9
        Dim output As String = ""

        While input > 0
            Dim number as Integer = input
            Dim i As Integer = 0
            While indices(i)  0 Then
                limit = i - 1
            Else
                limit = 0
            End If
            output = output & romanNumerals(limit)
            number = number - indices(limit)
            ' this was previously our return statement
            input = number                  
        End While
        Return output
    End Function

End Module


Excercises for you:

  • How could this be done using a dictionary instead.



  • How can this be changed so that numbers outside of 1-9

Code Snippets

Module IntegerExtensions

    Dim ind() As Integer = {1, 2, 3, 4, 5, 10, 50, 100, 500, 1000}
    Dim rom() As String = {"I", "II", "III", "IV", "V", "X", "L", "C", "D", "M"}
    Dim limit As Integer = 9
    Dim output As String = ""

    Private Function ConvertToRomanNumeral(ByVal input As Integer) As String
        output = ""
        While input > 0
            input = find(input)
        End While
        Return output
    End Function

    Private Function Find(ByVal Num As Integer) As Integer
        Dim i As Integer = 0
        While ind(i) <= Num
            i += 1
        End While
        If i <> 0 Then
            limit = i - 1
        Else
            limit = 0
        End If
        output = output & rom(limit)
        Num = Num - ind(limit)
        Return Num
    End Function

End Module

Public Class Form1
    Private Sub Button1_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles Button1.Click

        txt2.Text = IntegerExtensions.ConvertToRomanNumeral(999)        

    End Sub
End Class
Private Function ConvertToRomanNumeral(ByVal input As Integer) As String

    If (input < 1 Or input > 999) Then
        Throw New ArgumentOutOfRangeException("input","Input must be in the range 1-999")
    End If

    output = ""
    While input > 0
        input = find(input)
    End While
    Return output
End Function
Module IntegerExtensions

    Private Function ConvertToRomanNumeral(ByVal input As Integer) As String

        If (input < 1 Or input > 999) Then
            Throw New ArgumentOutOfRangeException("input","Input must be in the range 1-999")
        End If

        Dim indices() As Integer = {1, 2, 3, 4, 5, 10, 50, 100, 500, 1000}
        Dim romanNumerals() As String = {"I", "II", "III", "IV", "V", "X", "L", "C", "D", "M"}
        Dim limit As Integer = 9
        Dim output As String = ""

        While input > 0
            Dim number as Integer = input
            Dim i As Integer = 0
            While indices(i) <= number
                i += 1
            End While
            If i <> 0 Then
                limit = i - 1
            Else
                limit = 0
            End If
            output = output & romanNumerals(limit)
            number = number - indices(limit)
            ' this was previously our return statement
            input = number                  
        End While
        Return output
    End Function

End Module
Module IntegerExtensions

    <Extension()>
    Public Function ToRomanNumeral(ByVal input As Integer) As String

        If (input < 1 Or input > 999) Then
            Throw New ArgumentOutOfRangeException("input","Cannot convert integers outside of the range 1-999")
        End If

        Dim indices() As Integer = {1, 2, 3, 4, 5, 10, 50, 100, 500, 1000}
        Dim romanNumerals() As String = {"I", "II", "III", "IV", "V", "X", "L", "C", "D", "M"}
        Dim limit As Integer = 9
        Dim output As String = ""

        While input > 0
            Dim number as Integer = input
            Dim i As Integer = 0
            While indices(i) <= number
                i += 1
            End While
            If i <> 0 Then
                limit = i - 1
            Else
                limit = 0
            End If
            output = output & romanNumerals(limit)
            number = number - indices(limit)
            ' this was previously our return statement
            input = number                  
        End While
        Return output
    End Function

End Module

Context

StackExchange Code Review Q#59102, answer score: 10

Revisions (0)

No revisions yet.