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

Numbers to words converter

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
numberswordsconverter

Problem

Please review my code:

```
import java.lang.*;
import java.util.*;
import java.io.*;

class NumberToWords
{
static public boolean HelperConvertNumberToText(int num, String[] result)
{
String [] strones = {
"One", "Two", "Three", "Four", "Five", "Six", "Seven", "Eight",
"Nine", "Ten", "Eleven", "Twelve", "Thirteen", "Fourteen",
"Fifteen", "Sixteen", "Seventeen", "Eighteen", "Nineteen",
};

String [] strtens = {
"Ten", "Twenty", "Thirty", "Fourty", "Fifty", "Sixty",
"Seventy", "Eighty", "Ninety", "Hundred"
};

result[0] = "";
int single, tens, hundreds;

if(num > 1000)
return false;

hundreds = num / 100;
num = num - hundreds * 100;
if( num 0)
{
result[0] += strones[hundreds-1];
result[0] += " Hundred ";
}
if(tens > 0)
{
result[0] += strtens[tens - 1];
result[0] += " ";
}
if(single > 0)
{
result[0] += strones[single - 1];
result[0] += " ";
}
return true;
}

static public boolean ConvertNumberToText(int num, String[] result)
{
String tempString[] = new String[1];
tempString[0] = "";
int thousands;
int temp;
result[0] = "";
if(num 100000)
{
System.out.println(num + " \tNot Supported");
return false;
}

if( num == 0)
{
System.out.println(num + " \tZero");
return false;
}

if(num < 1000)
{
HelperConvertNumberToText(num, tempString);
result[0] += tempString[0];
}
else
{
thousands = num / 1000;
temp = num - thousands * 1000;
HelperConvertNumberToText(thousands, tempString);
result[0] += tempString[0];
result[0] += "Thousand ";
HelperConvertNumberToText(temp, tempString);
result[0] += tempString[0];
}

Solution

"Strone"? What's a "strone"? Oh… of course!

In seriousness, naming could be improved. They are obviously arrays of strings; the Hungarian prefix is unnecessary. Also, as constants, they should be declared static final and named with ALL_CAPS.

public class NumberToWords {
    private static final ONES = { "One", "Two", … };
    private static final TENS = { "Ten", "Twenty", … };
    …
}


Helper functions should be private.

It is customary to put the static keyword after the access modifier, as in public static void main(…). It is unconventional to say static public boolean ConvertNumberToText(…). Also, functions should be namedStartingWithLowerCase().

Using a single-element array as a parameter so that you can pass back the result as an out-parameter is very weird. Using a StringBuilder would be a bit more obvious, but still very odd. The right approach should be to return a String, and throw an exception if necessary — perhaps NumberFormatException. Ideally, your code should be able to handle every possible int input, making errors impossible.

Code Snippets

public class NumberToWords {
    private static final ONES = { "One", "Two", … };
    private static final TENS = { "Ten", "Twenty", … };
    …
}

Context

StackExchange Code Review Q#60859, answer score: 11

Revisions (0)

No revisions yet.