6
\$\begingroup\$

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) <= 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 Class

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

\$\endgroup\$
  • \$\begingroup\$ Does this help ? (Format numbers to roman numerals) stackoverflow.com/questions/11305822/… \$\endgroup\$ – SoEnLion Aug 5 '14 at 14:32
  • \$\begingroup\$ Why do you need clarification? This is just a reversed version of your own answer. The code is almost identical. With that being said, it failed with an IndexOutOfRangeException at the first run. Try enter 1234. \$\endgroup\$ – Bjørn-Roger Kringsjå Aug 5 '14 at 14:57
  • \$\begingroup\$ SoEnLion : defenitly work but need to extend. now the function is capable of provide answer below 1000. \$\endgroup\$ – Sujith Karivelil Aug 5 '14 at 15:12
  • 1
    \$\begingroup\$ The code functions as advertised from 1-999. I can review this. Please leave it open. \$\endgroup\$ – RubberDuck Aug 5 '14 at 16:43
10
\$\begingroup\$
  • 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) <= 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
    
  • 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 < 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
    
  • 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 < 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
    
  • 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
    
        <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
    

Excercises for you:

  • How could this be done using a dictionary instead.
  • How can this be changed so that numbers outside of 1-999 can be converted?
\$\endgroup\$
  • 1
    \$\begingroup\$ I would just like to note that there is still a fair bit of improvement that can be made. I was just getting a bit tired. \$\endgroup\$ – RubberDuck Aug 5 '14 at 19:25
6
\$\begingroup\$

To catch up where ckuhn203 got tired, I would like to finish ckuhn203's review.

  • Validating the input

    As it is senseless to check wether input > 999 in the case that input < 1 we can use the short-circuiting boolean operator OrElse

    If (input < 1 OrElse input > 999) Then
        Throw New ArgumentOutOfRangeException("input","Cannot convert integers outside of the range 1-999")
    End If
    
  • Initializing String variables

    For readability you should initialize String variables by assigning String.Empty.

    Dim output As String = ""
    

    will become

    Dim output As String = String.Empty
    
  • Inside the while loop

    This

    If i <> 0 Then
        limit = i - 1
    Else
        limit = 0
    End If
    

    is logically wrong. What would happen if i=-1 ? As it is <>0 limit would become -2. So it needs to be changed to

    If i > 0 Then
        limit = i - 1
    Else
        limit = 0
    End If
    

    next we get rid of the number variable as it isn't needed

    While input > 0
    
        Dim i As Integer = 0
        While indices(i) <= input
            i += 1
        End While
        If i > 0 Then
            limit = i - 1
        Else
            limit = 0
        End If
        output = output & romanNumerals(limit)
        input = input - indices(limit)
    
    End While
    

    And because we have the validation of the input number, I don't see a reason to keep the limit variable at all, so let us remove it

    While input > 0
    
        Dim i As Integer = 0
        While indices(i) <= input
            i += 1
        End While
        If i > 0 Then
            i -= 1
        Else
            i = 0
        End If
        output = output & romanNumerals(i)
        input = input - indices(i)
    
    End While
    

    and because i < 0 can't happen, we simplify the if i > 0 by removing the else part and get the result

    <System.Runtime.CompilerServices.Extension()>
    Public Function ToRomanNumeral(ByVal input As Integer) As String
    
        If (input < 1 OrElse 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 output As String = String.Empty
    
        While input > 0
    
            Dim i As Integer = 0
            While indices(i) <= input
                i += 1
            End While
            If i > 0 Then
                i -= 1
            End If
            output = output & romanNumerals(i)
            input = input - indices(i)
    
        End While
    
        Return output
    End Function
    
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.