Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have to convert positive numbers, that are less than 1000, to words. A quick search didn't find anything that looked good to my eye, so I came up with the following. While I was at it I expanded the conversion. I am looking for comments, suggestions, or errors I am missing.

Public Class NumberToWords

    Public Shared Function Convert(num As Integer, Optional negativeprfx As String = "negative") As String

        Dim wnum As Integer = num 'working number

        If wnum = Integer.MinValue Then
            Throw New ArgumentException("Can't convert")
        End If

        If wnum < 0 Then wnum = -wnum 'convert to positive if needed

        Dim rv As New System.Text.StringBuilder

        'is the number known?
        rv.Append(Defined(wnum))
        If rv.Length > 0 Then Return rv.ToString 'yes, return it

        'the groups
        Dim units As Integer = 0
        Dim tens As Integer = 0
        Dim hunds As Integer = 0
        Dim thous As Integer = 0
        Dim mils As Integer = 0
        Dim bils As Integer = 0

        'get count of each grouping
        'decreasing wnum by the grouping total

        bils = wnum \ NumWords.billion
        wnum -= bils * NumWords.billion

        mils = wnum \ NumWords.million
        wnum -= mils * NumWords.million

        thous = wnum \ NumWords.thousand
        wnum -= thous * NumWords.thousand

        hunds = wnum \ NumWords.hundred
        wnum -= hunds * NumWords.hundred

        'special case for tens
        'if the number is less than 21
        'don't bother dividing by ten
        'because all numbers less than 21 are defined

        If wnum > 20 Then
            tens = wnum \ NumWords.ten
            wnum -= tens * NumWords.ten
        End If

        units = wnum

        'now check each group
        'and recursively call convert on the
        'groups amount

        If bils > 0 Then
            rv.Append(Convert(bils))
            rv.Append(" ")
            rv.Append(NumWords.billion.ToString)
            rv.Append(" ")
        End If

        If mils > 0 Then
            rv.Append(Convert(mils))
            rv.Append(" ")
            rv.Append(NumWords.million.ToString)
            rv.Append(" ")
        End If

        If thous > 0 Then
            rv.Append(Convert(thous))
            rv.Append(" ")
            rv.Append(NumWords.thousand.ToString)
            rv.Append(" ")
        End If

        If hunds > 0 Then
            rv.Append(Convert(hunds))
            rv.Append(" ")
            rv.Append(NumWords.hundred.ToString)
            rv.Append(" ")
        End If

        If tens > 0 Then
            'the tens (10, 20, 30 ...) are defined
            rv.Append(Convert(tens * NumWords.ten)) ' times ten to get the defined number string
            rv.Append(" ")
        End If

        If units > 0 Then
            rv.Append(Convert(units))
            rv.Append(" ")
        End If
        'if the number was negative fix return
        If num < 0 Then
            rv.Insert(0, " ")
            rv.Insert(0, negativeprfx)
        End If
        Return rv.ToString.Trim
    End Function

    Private Shared Function Defined(num As Integer) As String
        'is the number defined and less than 100
        If [Enum].IsDefined(GetType(NumWords), num) AndAlso num < 100 Then
            'yes
            Return [Enum].GetName(GetType(NumWords), num)
        Else
            'no
            Return ""
        End If
    End Function

    Enum NumWords
        zero = 0
        one = 1
        two = 2
        three = 3
        four = 4
        five = 5
        six = 6
        seven = 7
        eight = 8
        nine = 9
        ten = 10
        eleven = 11
        twelve = 12
        thirteen = 13
        fourteen = 14
        fifteen = 15
        sixteen = 16
        seventeen = 17
        eightteen = 18
        nineteen = 19
        twenty = 20
        thirty = 30
        forty = 40
        fifty = 50
        sixty = 60
        seventy = 70
        eighty = 80
        ninety = 90
        hundred = 100
        thousand = 1000
        million = 1000000
        billion = 1000000000
    End Enum
End Class
share|improve this question
    
1  
Sorry. Won't do it again. –  dbasnett Aug 2 at 16:23
    
FYI: Copy-cat question from me: Converting a number to the text representation –  rolfl Aug 3 at 4:34

2 Answers 2

The function name Convert doesn't seem particularly helpful to me. Convert how? From what to what? In fact, we're not converting anything... we're representing the same unconverted value in a different format.


Dim wnum As Integer = num 'working number

If wnum is the working number, and you feel wnum is so unclear that you need to add a comment to explain what it is, then why don't you just name the variable workingNumber so it's more clear through the entirety of the function?


If wnum < 0 Then wnum = -wnum 'convert to positive if needed

Why not just take the absolute value?

wnum = Math.Abs(wnum)

Dim units As Integer = 0
Dim tens As Integer = 0
Dim hunds As Integer = 0
Dim thous As Integer = 0
Dim mils As Integer = 0
Dim bils As Integer = 0

I'd call these as: ones, tens, hundreds, thousands, millions, billions

Shorter names don't make your program run faster or use less memory. It just makes it harder to read.


    If num < 0 Then
        rv.Insert(0, " ")
        rv.Insert(0, negativeprfx)
    End If

I'd also recommend doing this first, for the same reason we're starting with billions and working down to the ones place. I'm not 100% familiar with how .NET's string builder workers, but generally speaking, adding things to the front of a collection (in this case, a string is a collection of characters) is significantly more expensive and time consuming then adding to the back of a collection. So let's start at the front.

share|improve this answer
    
Typically I work on the algorithm, and then fix the names, so your naming comments are noted. The insert is needed at the end so that it reads negative one hundred, not one hundred negative. –  dbasnett Aug 2 at 15:18
1  
Which is why I'm suggesting you check for negative as the first thing before you build any other part of the string, then append the number itself to either negative or nothing. It can be changed so you don't need an insert and can still build the string as is. –  nhgrif Aug 2 at 15:18
3  
Worrying about names last and only after the algorithm is good and correct also seems like the exactly worst thing to do. Here, the algorithm is relatively simple. In other cases, the algorithm may not be so simple. Better variable names makes working on the algorithm easier, and it's also the easiest thing to get right on the first try. –  nhgrif Aug 2 at 15:20
    
To a point I agree, but the names I used were in the ballpark. –  dbasnett Aug 2 at 15:31
    
About appending the 'negative' as opposed to inserting it after the fact... That would mean that the check for defined values would have to be re-written. I take your point but appending v. inserting has its own overhead. BTW - there was a bug I just discovered, initial known numbers that were negative. Should I re-post the code as is or leave the old and add the new? –  dbasnett Aug 2 at 15:41

Normally I would say that magic numbers are a bad thing and are to be avoided, but since we're explicitly talking about numbers, I think you're justified in their use here.

Why are you doing this?

    If wnum = Integer.MinValue Then
        Throw New ArgumentException("Can't convert")
    End If

The minimum value of an integer is -2,147,483,648. I don't readily see a reason why you couldn't convert that value. If anything, you would want to check for values less than Integer.MinValue or greater than Integer.MaxValue, but since the num parameter is an integer, the compiler will do that check for you.

@Nhgrif pointed out that this is an "off by one issue" that causes an exception when getting the absolute value of Integer.MinValue. I take back what I said and recommend adding a comment in the code to explain the reason for this check instead.

Also consider giving a more meaningful message than "Can not convert". "Can not convert numbers less than '-2,147,483,647'." Would be much more meaningful to someone debugging a program that calls this code.

share|improve this answer
2  
Probably because one of the first steps of his function is to essentially take the absolute value of of the number, and the minimum value for an integer is one farther from zero than the maximum value for an integer. Try calling Math.Abs(Integer.MinValue). It throws an exception. –  nhgrif Aug 2 at 19:12
1  
Ok, well, the XML documentation wasn't included, so ok. I still think you need to return a better exception message here though. –  RubberDuck Aug 2 at 20:44
1  
Also, I agree though that including comments in the XML documentation isn't enough. I can't ever remember looking at XML documentation of any code I've ever maintained, regardless of whether or not it existed. If I'm maintaining code, I want the code to tell me what it does. –  nhgrif Aug 2 at 22:08
2  
@nhgrif I wouldn't consider letting Math.Abs() throw to be sufficient in this case. Callers will discover that they can't convert Integer.MinValue, but they won't know if that's an intentional limitation of the method or just a bug. It's also leaking information that the caller doesn't need to know ("I use Math.Abs()") and delegating validation of input to an outside authority. –  Chris Hayes Aug 2 at 23:54
2  
Instead of throwing an exception for Integer.MinValue, why not just Return "negative two billion one hundred forty-seven million four hundred eighty-three six hundred forty-seven" as a special case? –  200_success Aug 3 at 7:27

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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