# Number to Words

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

-
Sorry. Won't do it again. –  dbasnett Aug 2 '14 at 16:23
FYI: Copy-cat question from me: Converting a number to the text representation –  rolfl Aug 3 '14 at 4:34
One of the lawyers at work spotted a misspelling in the NumWords enum. 18 should be eighteen. Sorry. –  dbasnett Feb 25 at 12:38

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.

-
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 '14 at 15:18
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 '14 at 15:18
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 '14 at 15:20
To a point I agree, but the names I used were in the ballpark. –  dbasnett Aug 2 '14 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 '14 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.

-
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 '14 at 19:12
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 '14 at 20:44
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 '14 at 22:08
@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 '14 at 23:54
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 '14 at 7:27