patternjavaMinor
Converting an integer number into its word representation
Viewed 0 times
numberintoitswordconvertingrepresentationinteger
Problem
I've written this code to convert an integer number into its word representation. I've tested it for many cases and it seems to be working as expected.
For example:
However, I think it could be improved especially because I'm doing some checks repeatedly. Can someone suggest some refactoring for this?
```
import java.util.HashMap;
public class NumberToWord {
static HashMap numberMap = new HashMap();
static{
numberMap.put(0,"Zero");
numberMap.put(1,"One");
numberMap.put(2,"Two");
numberMap.put(3,"Three");
numberMap.put(4,"Four");
numberMap.put(5,"Five");
numberMap.put(6,"Six");
numberMap.put(7,"Seven");
numberMap.put(8,"Eight");
numberMap.put(9,"Nine");
numberMap.put(10,"Ten");
numberMap.put(11,"Eleven");
numberMap.put(12,"Twelve");
numberMap.put(13,"Thirteen");
numberMap.put(14,"Fourteen");
numberMap.put(15,"Fifteen");
numberMap.put(16,"Sixteen");
numberMap.put(17,"Seventeen");
numberMap.put(18,"Eighteen");
numberMap.put(19,"Nineteen");
numberMap.put(20,"Twenty");
numberMap.put(30,"Thirty");
numberMap.put(40,"Forty");
numberMap.put(50,"Fifty");
numberMap.put(60,"Sixty");
numberMap.put(70,"Seventy");
numberMap.put(80,"Eighty");
numberMap.put(90,"Ninety");
numberMap.put(100,"Hundred");
numberMap.put(1000,"Thousand");
}
public static Str
For example:
- 1000 displayed as "One Thousand"
- 99999 displayed as "Ninety nine thousand nine hundred ninety nine"
- 999999999 displayed as "Nine hundred ninety nine million nine hundred ninety nine thousand nine hundred ninety nine"
However, I think it could be improved especially because I'm doing some checks repeatedly. Can someone suggest some refactoring for this?
```
import java.util.HashMap;
public class NumberToWord {
static HashMap numberMap = new HashMap();
static{
numberMap.put(0,"Zero");
numberMap.put(1,"One");
numberMap.put(2,"Two");
numberMap.put(3,"Three");
numberMap.put(4,"Four");
numberMap.put(5,"Five");
numberMap.put(6,"Six");
numberMap.put(7,"Seven");
numberMap.put(8,"Eight");
numberMap.put(9,"Nine");
numberMap.put(10,"Ten");
numberMap.put(11,"Eleven");
numberMap.put(12,"Twelve");
numberMap.put(13,"Thirteen");
numberMap.put(14,"Fourteen");
numberMap.put(15,"Fifteen");
numberMap.put(16,"Sixteen");
numberMap.put(17,"Seventeen");
numberMap.put(18,"Eighteen");
numberMap.put(19,"Nineteen");
numberMap.put(20,"Twenty");
numberMap.put(30,"Thirty");
numberMap.put(40,"Forty");
numberMap.put(50,"Fifty");
numberMap.put(60,"Sixty");
numberMap.put(70,"Seventy");
numberMap.put(80,"Eighty");
numberMap.put(90,"Ninety");
numberMap.put(100,"Hundred");
numberMap.put(1000,"Thousand");
}
public static Str
Solution
Some notes:
-
-
Instead of the
See: Why not just use System.out.println()? in the JUnit FAQ.
-
The
-
According to the Code Conventions for the Java Programming Language
if statements always use braces {}.
Omitting them is error-prone.
-
Numbers like
-
The method does not handle negative numbers. In this case you should throw an
-
The reference type of
-
What's the deal with Java's public fields?
-
I'd try to extract out a few methods:
It would remove some code duplication. Maybe you can find a bett
-
wordForm should be StringBuilder: https://stackoverflow.com/questions/73883/string-vs-stringbuilder Actually, I'd rename it to result too to make it clear that this object stores the result of the method.-
Instead of the
System.out.printlns, use a parametrized unit test:import static org.junit.Assert.assertEquals;
import java.util.Arrays;
import java.util.Collection;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
import org.junit.runners.Parameterized.Parameters;
@RunWith(value = Parameterized.class)
public class NumberToWordTest extends NumberToWord {
private final String expected;
private final int input;
public NumberToWordTest(final String expected, final int input) {
this.expected = expected;
this.input = input;
}
@Parameters
public static Collection data() {
final Object[][] data = new Object[][] {
{ "One Hundred Thousand", 100000 },
{ "Nine HundredNinetyNine Thousand Nine HundredNinetyNine", 999999 },
{ "Six HundredSeventyEight Thousand Nine Hundred", 678900 },
{ "Zero", 0 },
{ "One Hundred Thousand Five HundredSixtySeven", 100567 },
{ "FourThousandFive HundredEightyNine", 4589 },
{ "ThreeThousandThree HundredThirtyThree", 3333 },
{ "SixtySeven Thousand Five Hundred", 67500 },
{ "SeventyTwo", 72 },
{ "One HundredSeventyTwo Thousand Three HundredFortySix", 172346 },
{ "Eight HundredNinety Thousand", 890000 },
{ "Six Hundred Thousand Seven Hundred", 600700 },
{ "SixtySeven", 67 },
{ "Nine HundredNinetyNine Million Nine HundredNinetyNine Thousand Nine HundredNinetyNine", 999999999 } };
return Arrays.asList(data);
}
@Test
public void test() {
assertEquals(expected, NumberToWord.numberToWord(input));
}
}See: Why not just use System.out.println()? in the JUnit FAQ.
-
The
number parameter could be final. It would help readers, because they know that the value does not change later. https://softwareengineering.stackexchange.com/questions/115690/why-declare-final-variables-inside-methods but you can find other questions on Programmers.SE in the topic.-
According to the Code Conventions for the Java Programming Language
if statements always use braces {}.
Omitting them is error-prone.
-
Numbers like
1000000000, 1000000 etc. are magic numbers. You should use named constants.private static final int ZERO = 0;
private static final int TEN = 10;
private static final int ONE_HUNDRED = 100;
private static final int ONE_THOUSAND = 1000;
private static final int TEN_THOUSANDS = 10000;
private static final int ONE_BILLION = 1000000000;
private static final int ONE_MILLION = 1000000;-
The method does not handle negative numbers. In this case you should throw an
IllegalArgumentException instead of returning an empty string. (See: Effective Java, 2nd edition, Item 38: Check parameters for validity)-
The reference type of
numberMap should be simply Map. See: Effective Java, 2nd edition, Item 52: Refer to objects by their interfaces-
numberMap could be private and unmodifiable. It would prevent accidental modification.private static final Map numberMap = createNumberMap();
private static Map createNumberMap() {
final Map numberMap = new HashMap();
numberMap.put(0, "Zero");
numberMap.put(1, "One");
...
return Collections.unmodifiableMap(numberMap);
}What's the deal with Java's public fields?
-
I'd try to extract out a few methods:
private static void calc(final StringBuilder result, final int number,
final int divisor, final String postfix) {
final int quotient = number / divisor;
final int remainder = number % divisor;
if (quotient != 0) {
result.append(numberToWord(quotient));
result.append(" ");
result.append(postfix);
}
if (remainder != 0) {
result.append(" ");
result.append(numberToWord(remainder));
}
}
private static void calc2(final StringBuilder result, final int number,
final int divisor, final String postfix) {
final int quotient = number / divisor;
final int remainder = number % divisor;
if (quotient != 0) {
result.append(numberMap.get(quotient));
result.append(postfix);
}
if (remainder != 0) {
result.append(numberToWord(remainder));
}
}
private static void calc3(final StringBuilder result, final int number,
final int divisor) {
final int quotient = number / divisor;
final int remainder = number % divisor;
if (quotient != 0) {
result.append(numberMap.get(quotient * divisor));
}
if (remainder != 0) {
result.append(numberMap.get(remainder));
}
}It would remove some code duplication. Maybe you can find a bett
Code Snippets
import static org.junit.Assert.assertEquals;
import java.util.Arrays;
import java.util.Collection;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
import org.junit.runners.Parameterized.Parameters;
@RunWith(value = Parameterized.class)
public class NumberToWordTest extends NumberToWord {
private final String expected;
private final int input;
public NumberToWordTest(final String expected, final int input) {
this.expected = expected;
this.input = input;
}
@Parameters
public static Collection<Object[]> data() {
final Object[][] data = new Object[][] {
{ "One Hundred Thousand", 100000 },
{ "Nine HundredNinetyNine Thousand Nine HundredNinetyNine", 999999 },
{ "Six HundredSeventyEight Thousand Nine Hundred", 678900 },
{ "Zero", 0 },
{ "One Hundred Thousand Five HundredSixtySeven", 100567 },
{ "FourThousandFive HundredEightyNine", 4589 },
{ "ThreeThousandThree HundredThirtyThree", 3333 },
{ "SixtySeven Thousand Five Hundred", 67500 },
{ "SeventyTwo", 72 },
{ "One HundredSeventyTwo Thousand Three HundredFortySix", 172346 },
{ "Eight HundredNinety Thousand", 890000 },
{ "Six Hundred Thousand Seven Hundred", 600700 },
{ "SixtySeven", 67 },
{ "Nine HundredNinetyNine Million Nine HundredNinetyNine Thousand Nine HundredNinetyNine", 999999999 } };
return Arrays.asList(data);
}
@Test
public void test() {
assertEquals(expected, NumberToWord.numberToWord(input));
}
}private static final int ZERO = 0;
private static final int TEN = 10;
private static final int ONE_HUNDRED = 100;
private static final int ONE_THOUSAND = 1000;
private static final int TEN_THOUSANDS = 10000;
private static final int ONE_BILLION = 1000000000;
private static final int ONE_MILLION = 1000000;private static final Map<Integer, String> numberMap = createNumberMap();
private static Map<Integer, String> createNumberMap() {
final Map<Integer, String> numberMap = new HashMap<Integer, String>();
numberMap.put(0, "Zero");
numberMap.put(1, "One");
...
return Collections.unmodifiableMap(numberMap);
}private static void calc(final StringBuilder result, final int number,
final int divisor, final String postfix) {
final int quotient = number / divisor;
final int remainder = number % divisor;
if (quotient != 0) {
result.append(numberToWord(quotient));
result.append(" ");
result.append(postfix);
}
if (remainder != 0) {
result.append(" ");
result.append(numberToWord(remainder));
}
}
private static void calc2(final StringBuilder result, final int number,
final int divisor, final String postfix) {
final int quotient = number / divisor;
final int remainder = number % divisor;
if (quotient != 0) {
result.append(numberMap.get(quotient));
result.append(postfix);
}
if (remainder != 0) {
result.append(numberToWord(remainder));
}
}
private static void calc3(final StringBuilder result, final int number,
final int divisor) {
final int quotient = number / divisor;
final int remainder = number % divisor;
if (quotient != 0) {
result.append(numberMap.get(quotient * divisor));
}
if (remainder != 0) {
result.append(numberMap.get(remainder));
}
}public static String numberToWord(final int number) {
if (number < 0) {
throw new IllegalArgumentException("Number can't be negative: " + number);
}
final StringBuilder result = new StringBuilder();
if (number < ONE_BILLION && number >= ONE_MILLION) {
calc(result, number, ONE_MILLION, "Million");
} else if (number < ONE_MILLION && number >= TEN_THOUSANDS) {
calc(result, number, ONE_THOUSAND, "Thousand");
} else if (number < TEN_THOUSANDS && number >= ONE_THOUSAND) {
calc2(result, number, ONE_THOUSAND, "Thousand");
} else if (number < ONE_THOUSAND && number >= ONE_HUNDRED) {
calc2(result, number, ONE_HUNDRED, " Hundred");
} else if (number < ONE_HUNDRED && number >= TEN) {
calc3(result, number, TEN);
} else if (number < TEN && number >= ZERO) {
result.append(numberMap.get(number));
}
return result.toString();
}Context
StackExchange Code Review Q#15224, answer score: 8
Revisions (0)
No revisions yet.