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

Number to Words

Submitted by: @import:stackexchange-codereview··
0
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)

Solution

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.

Code Snippets

Dim wnum As Integer = num 'working number
If wnum < 0 Then wnum = -wnum 'convert to positive if needed
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
If num < 0 Then
        rv.Insert(0, " ")
        rv.Insert(0, negativeprfx)
    End If

Context

StackExchange Code Review Q#58833, answer score: 10

Revisions (0)

No revisions yet.