patternModerate
Number to Words
Viewed 0 times
wordsnumberstackoverflow
Problem
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 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)
```
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 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)
Solution
The function name
If
Why not just take the absolute value?
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.
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.
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 numberIf
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 neededWhy 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 = 0I'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 IfI'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.
Code Snippets
Dim wnum As Integer = num 'working numberIf wnum < 0 Then wnum = -wnum 'convert to positive if neededwnum = 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 = 0If num < 0 Then
rv.Insert(0, " ")
rv.Insert(0, negativeprfx)
End IfContext
StackExchange Code Review Q#58833, answer score: 10
Revisions (0)
No revisions yet.