Digit to words converter

Question

This is one of the longest programs I have made with methods and I think I must be doing this rather inefficiently. Any comments to improve would be appreciated.

package numberwords2;
import java.awt.Component;
import javax.swing.JOptionPane;
public class NumberWords2
{
    static String rawInput;
    static String output = "";
    static int hold;

    private static Component frame;

    public static void main(String[] args)
    {

        rawInput = JOptionPane.showInputDialog("Enter a number between 0 and 9999.");
        int input = Integer.parseInt(rawInput);

        if(input == 0)
        {
            output += "Zero";
        }
        else if(input < 10)
        {
            output += ones(0);
        }
        else if(input < 20)
        {
            output += teens(0);
        }
        else if(input < 100)
        {
            output += tens(0);
            output += ones(1);
        }
        else if(input < 1000)
        {
            output += hundreds(0);
            int temp = Integer.parseInt(rawInput.substring(1,3));
            if(temp > 10 && temp < 20)
            {
                output += teens(2);
            }
            else
            {
                output += tens(1);
                output += ones(2);
            }

        }
        else if(input < 10000)
        {
            output += thousands(0);
            output += hundreds(1);
            int temp = Integer.parseInt(rawInput.substring(2,4));
            if(temp > 10 && temp < 20)
            {
                output += teens(3);
            }
            else
            {
                output += tens(2);
                output += ones(3);
            }

        }

        JOptionPane.showMessageDialog(frame, rawInput + " is" + output + ".");
        output = "";
        main(args);
    }

    public static String teens(int hold)
    {
        String[] teens = {"Ten", "Eleven", "Twelve", "Thirteen", "Fourteen",
            "Fifteen", "Sixteen", "Seventeen", "Eighteen", "Nighteen"};
        int x = Character.getNumericValue(rawInput.charAt(hold));
        int y = 0;
        for(int i = 0; i < 10; i++)
        {
            if(i == x)
            {
                y = i;
                break;
            }
        }
        return " " + teens[y];
    }

    private static String tens(int hold)
    {
        String[] tens = {"", "Ten", "Twenty", "Thirty", "Fourty", "Fifty", "Sixty", "Seventy", "Eighty", "Ninety"};
        int x = Character.getNumericValue(rawInput.charAt(hold));
        int y = 0;
        for(int i = 0; i < 10; i++)
        {
            if(i == x)
            {
                y = i;
            }
        }
        return  " " + tens[y];
    }

    private static String ones(int hold)
    {
        String[] ones = {"", "One", "Two", "Three", "Four", "Five", "Six", "Seven", "Eight", "Nine"};
        int x = Character.getNumericValue(rawInput.charAt(hold));
        int y = 0;
        for(int i = 1; i < 10; i++)
        {
            if(i == x)
            {
                y = i;
                break;
            }
        }
        return " " + ones[y];
    }

    private static String hundreds(int hold)
    {
        return ones(hold) + " Hundred";
    }

    private static String thousands(int hold)
    {
        return ones(hold) + " Thousand";
    }
}

Answer 1

int input = Integer.parseInt(rawInput);

This can throw an exception if rawInput isn't a number.

---

    else if(input < 10)
    {
        output += ones(0);
    }
    else if(input < 20)
    {
        output += teens(0);
    }
    else if(input < 100)
    {
        output += tens(0);
        output += ones(1);
    }
    else if(input < 1000)
    {
        output += hundreds(0);
        int temp = Integer.parseInt(rawInput.substring(1,3));
        if(temp > 10 && temp < 20)
        {
            output += teens(2);
        }
        else
        {
            output += tens(1);
            output += ones(2);
        }

    }

Code (subroutine calls) are duplicated in the above:

• If the number is less than 1000 then you call tens, teens, and ones
• If the number is greater than 1000 then you call tens, teens, and ones

There would be less code if you processed it from left to right, like you do when you read and say a number:

if (input > 1000)
{
    output += thousands;
    subtract thousands from input;
}
if (input > 100)
{
    output += hundreds;
    subtract hundreds from input;
}
if (10 < input < 20)
{
    output += teens;
}
else
{
    if (input >= 20)
    {
        output += tens;
        subtract tens from input;
    }
    output += ones;
}
if (output is empty)
    output = "Zero";

---

int temp = Integer.parseInt(rawInput.substring(1,3));

This is a complicated way to remove the thousands from the input.

A simpler way to subtract thousands from the input is:

input = input - ((input / 1000) * 1000);

---

main(args);

This is a strange way to loop forever.

Better would be:

public static void main(String[] args)
{
    while (true)
        run();
}

static void run()
{
    rawInput = JOptionPane.showInputDialog("Enter a number between 0 and 9999.");
    ... etc ...
}

---

Perhaps only the first letter should be capitalized; perhaps with hyphens; and the word "and"; for example "Four hundred and forty-three" not "Four Hundred Forty Three".

Answer 2

There is no way to stop this program. It will continue to recall main() if no exceptions are thrown. The problem isn't so much that it will run forever, but how you are achieving the looping. If this program is run for a long enough time, it will eventually throw a StackOverflowException because main() has recursed so many times. It would be better to use a real loop instead of using this recursion trick.

String getInput() {
  return JOptionPane.showInputDialog("Enter a number between 0 and 9999. q to Quit");
}

//...

String rawInput = getInput();
while (!rawInput.equal("q") {
  //...
  rawInput = getInput();
}

---

Variable:

output are only used in main(). It should be scoped accordingly. Having them as static variables means any of your methods can change them at any time. This would a bug that is hard to track down. You might also want to think about passing rawInput as an argument to functions instead of using a static variable.

The static hold variable is never used. Even worse, it is shadowed by a number of method parameters. This is another bug that is hard to track down when it happens. You could set a value to hold in main(), expecting it to be present in a method that uses a variable named hold. But it the method defines a parameter named hold, the value in that scope will be the value the method was called with.

The name hold, what does it mean? It is being used as an index into the input, but without diving into the code, I'm not sure what it is. The name should help people reading your code know what the variable is.

frame is defined, but never assigned a value. This means when you call JOptionPane.showMessageDialog(), you are actually passing null as the first argument. This happens to be a valid value, but passing null explicitly is better. Seeing the variable used as an argument may make someone assume that it has a value and attempt to call a method on it.

---

Code repetition:

There is a lot of it. From little things like hundreds() and thousands() to big structural things like most of the code in main().

hundreds() and thousands() is simple. The only difference is the string appended to the result of ones(). You can make s single function that tasks a value and suffix, which hundreds() and thousands() call. Then when you need to add millions(), it more code reuse instead of code copying. This may seem extreme in this instance, but starting to look for these types of things will make your code much cleaner.

ones(), tens(), teens() also look very structurally similar.

private static String suffix(String input, int index, String[] suffixes)
{
    int x = Character.getNumericValue(input.charAt(index));
    for(int i = 0; i < suffixes.length; i++)
    {
        if(i == x)
        {
            return " " + suffixes[i];
        }
    }
}

This function can now be used by each of the three you wrote, simplifying you code. This is a better example of what I was talking about with hundreds() and thousands().

I made a few other changes I wanted to point out:

• The for loop is up to the array length. 10 was a magic number that could mean anything. If later you change the array to have more values, you would never try those. If there were less, you would get an exception.
• I moved the return into the if statement. You aren't doing anything interesting after the loop and it eliminates the need for y. It also removes the need for the break.

The best part about having one function execute the body of these three functions is that you only have one place to have a functional bug. If you look closely at tens(), you forgot the break statement. The code is still correct, but it does more work than it needs to. It's easy to assume that 3 functions that are doing the same thing are implemented the same way, but a subtle bug like this is hard to find.

---

main():

This is a bugger structural change and involves some work to build it up. But if you think about it, the code is going through a list values and checking if the input is less than the specific value. Once it finds the specific value that the input is less than, some work is done to create the output.

interface Converter {
 String convert(int value);
}

Using this concept, you can create a number of classes that implement Converter. Then you can think of the specific values we were testing against as keys that point to the specific Converter that knows how to convert the input. This is done by using a Map.

A LinkedHashMap will allow you to create this mapping and also control how you iterate over the keys in the map.

Map<Integer, Converter> converters = new LinkedHashMap<>();
converters.put(1, new ZeroConverter());
converters.put(10, new OnesConverter());
//...

for (Entry<Integer, Converter> e : converters.entrySet()) {
  if (input < e.getKey()) {
    output = e.getValue().convert(input);
    break;
  }
}

With this structure, your main() method becomes the simple processing constructing the Map at the start, and putting this for loop in the while loop I mentioned earlier.