Return to Answer

• 1

• 1

• 30.4k
• 9
• 82
• 157

Some notes:

1. wordForm should be StringBuilder: http://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.

2. 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<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));
    }
   }
   

See: Why not just use System.out.println()? in the JUnit FAQ.

3. The number parameter could be final. It would help readers, because they know that the value does not change later. http://programmers.stackexchange.com/questions/115690/why-declare-final-variables-inside-methods but you can find other questions on Programmers.SE in the topic.

4. According to the Code Conventions for the Java Programming Language

   if statements always use braces {}.

Omitting them is error-prone.

5. 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;
   

6. 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)

7. The reference type of numberMap should be simply Map<Integer, String>. See: Effective Java, 2nd edition, Item 52: Refer to objects by their interfaces

8. numberMap could be private and unmodifiable. It would prevent accidental modification.

   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);
   }
   

What's the deal with Java's public fields?

9. 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 better names for them.

Then, the numberToWord looks like this:

 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();
 }