5
\$\begingroup\$

(See also the next iteration.)

I have rolled this short program that converts int values to the human readable strings:

package com.github.coderodde.fun;

import java.util.HashMap;
import java.util.Map;
import java.util.Scanner;

public class IntToHumanReadableStringConverter {
    
    private static final Map<Integer, String> TENS_MAP = new HashMap<>();
    
    static {
        TENS_MAP.put(1, "one");
        TENS_MAP.put(2, "two");
        TENS_MAP.put(3, "three");
        TENS_MAP.put(4, "four");
        TENS_MAP.put(5, "five");
        TENS_MAP.put(6, "six");
        TENS_MAP.put(7, "seven");
        TENS_MAP.put(8, "eight");
        TENS_MAP.put(9, "nine");
        TENS_MAP.put(10, "ten");
        TENS_MAP.put(11, "eleven");
        TENS_MAP.put(12, "twelve");
        TENS_MAP.put(13, "thirteen");
        TENS_MAP.put(14, "fourteen");
        TENS_MAP.put(15, "fifteen");
        TENS_MAP.put(16, "sixteen");
        TENS_MAP.put(17, "seventeen");
        TENS_MAP.put(18, "eighteen");
        TENS_MAP.put(19, "nineteen");
        TENS_MAP.put(20, "twenty");
        TENS_MAP.put(30, "thirty");
        TENS_MAP.put(40, "fourty");
        TENS_MAP.put(50, "fifty");
        TENS_MAP.put(60, "sixty");
        TENS_MAP.put(70, "seventy");
        TENS_MAP.put(80, "eighty");
        TENS_MAP.put(90, "ninety");
    }
    
    public static String convert(int num) {
        StringBuilder sb = new StringBuilder();
        
        if (num < 0) {
            sb.append("minus ");
            num = -num;
        }
        
        int billions = num / 1_000_000_000;
        
        num -= billions * 1_000_000_000;
        
        int millions = num / 1_000_000;
        
        num -= millions * 1_000_000;
        
        int thousands = num / 1_000;
        
        num -= thousands * 1_000;
        
        int units = num;
                
        if (billions > 0) {
            sb.append(convertUnitsImpl(billions));
            sb.append(" billion ");
        }
        
        if (millions > 0) {
            sb.append(convertHundredsImpl(millions));
            sb.append(" million ");
        }
        
        if (thousands > 0) {
            sb.append(convertHundredsImpl(thousands));
            sb.append(" thousand ");
        }
        
        if (units > 0) {
            sb.append(convertHundredsImpl(units));
        }
        
        return sb.toString();
    }
    
    private static String convertUnitsImpl(int unit) {
        return TENS_MAP.get(unit);
    }
    
    // Converts a num in range [0, 999] to the human readable string:
    private static String convertHundredsImpl(int num) {
        StringBuilder sb = new StringBuilder();
        
        int hundreds = num / 100;
        num -= 100 * hundreds;
        
        if (hundreds > 0) {
            sb.append(convertUnitsImpl(hundreds));
            sb.append(" hundred ");
        }
        
        if (num > 0) {
            String tensString = TENS_MAP.get(num);
            
            if (tensString == null) {
                int units = num % 10;
                num -= units;
                sb.append(TENS_MAP.get(num));
                sb.append("-");
                sb.append(convertUnitsImpl(units));
            } else {
                sb.append(tensString);
            }
        }
        
        return sb.toString();
    }

    public static void main(String[] args) {
        Scanner scanner = new Scanner(System.in);
        
        while (true) {
            int num = scanner.nextInt();
            String s = convert(num);
            System.out.println(s);
        }
    }
}

For example, when I input -123456, I get minus one hundred twenty-three thousand four hundred fifty-six.

Critique request

As always, I would like to hear whatever comes to mind.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ How would you translate this to other human languages? \$\endgroup\$ Jun 17 at 10:47
  • \$\begingroup\$ @ThorbjørnRavnAndersen See my updated question title in case something ain’t obvious to ya, sir. \$\endgroup\$
    – coderodde
    Jun 17 at 11:56

4 Answers 4

6
\$\begingroup\$

I'm not keen on the class name or method names - see my alternatives below.

I find perhaps a little more logic in the code than I'd look for - normally I try to find some way of making this sort of process data-driven, using some tabular representation of ranges, limits, and so on.

With reference to the string names, I don't believe you need a map at all. This is crying out for use of arrays, with some wrinkles for the peculiarities of spelling numbers between ten and twenty.

I'd go for an array for small numbers (1 - 19), one for multiples of ten upto ninety. I'd have a tabular representation of the "big" numbers, and a bit of special-casing for hundreds.

Then the code can be a bit more elegant and modular, at least in my opinion ;-)

Note my use of "long" to handle big negatives.

My example hard-codes the numbers for testing. Like "200_success" I'm not taken with the loop control in yours!

I enjoyed thinking about this - here's how I might do it.

(Working out how to add "and" in at points where an native speaker would put it is an interesting extension that I haven't tackled!)

public class NumberSpeller {

  private static final String[] SMALL_NUMBERS = {
      null, // special case zero
      "one", "two", "three", "four", "five", "six", "seven", "eight", "nine", "ten", //
      "eleven", "twelve", "thirteen", "fourteen", "fifteen", "sixteen", "seventeen", "eighteen", "nineteen" //
  };

  private static final String[] MULTIPLES_OF_TEN = {
      null, // special case zero
      "ten", "twenty", "thirty", "fourty", "fifty", "sixty", "seventy", "eighty", "ninety"
  };

  private static final class BigNumber { // poor choice of name, perhaps

    private int size;
    private String name;

    private BigNumber(int size, String name) {
      this.size = size;
      this.name = name;
    }

    public int getSize() {
      return size;
    }

    public String getName() {
      return name;
    }
  }

  // int can't go over single-digit billions ...
  private static final BigNumber[] BIG_NUMBERS = {
      new BigNumber(1_000_000_000, "billion"),
      new BigNumber(1_000_000, "million"),
      new BigNumber(1_000, "thousand")
  };

  // Special case, but try and be as consistent as possible
  private static final BigNumber HUNDRED = new BigNumber(100, "hundred");

  public static String spell(int candidate) {

    if (candidate == 0) {
      return "zero";
    }

    StringBuilder sb = new StringBuilder();

    long num = candidate; // Handle full range of negative ints...

    if (num < 0) {
      sb.append("minus ");
      num = -num;
    }

    for (BigNumber bigNumber : BIG_NUMBERS) {
      long current = num / bigNumber.getSize();
      num %= bigNumber.getSize();
      if (current > 0) {
        spellBigNumber(sb, current);
        sb.append(bigNumber.getName()).append(" ");
      }
    }

    if (num > HUNDRED.getSize()) {
      spellSmallNumber(sb,  num / HUNDRED.getSize());
      sb.append(HUNDRED.getName()).append(" ");
      num %= HUNDRED.getSize();
    }

    spellMediumNumber(sb,num);

    return sb.toString();
  }

  private static void spellBigNumber(StringBuilder sb, long current) {
    long hundreds = current / HUNDRED.getSize();
    if (hundreds > 0) {
      spellSmallNumber(sb, hundreds);
      sb.append(HUNDRED.getName()).append(" ");
      current %= HUNDRED.getSize();
    }

    spellMediumNumber(sb, current);
  }

  private static void spellMediumNumber(StringBuilder sb, long num) {
    if (num >= 20) {
      sb.append(MULTIPLES_OF_TEN[(int) (num / 10)]).append(" ");
      num %= 20;
    }

    if (num > 0) {
      spellSmallNumber(sb, num);
    }

  }

  private static void spellSmallNumber(StringBuilder sb, long l) {
    sb.append(SMALL_NUMBERS[(int) l]).append(" ");
  }

  public static void main(String[] args) {
    int[] candidates = {
        2147483647,
        147483648,
        47483648,
        7483648,
        483648,
        83648,
        3648,
        648,
        48,
        8,
        -2147483648,
        2_000_000_000,
        2_000_000,
        2_000,
        200,
        20,
        14,
        1,
        0
    };

    for (int candidate : candidates) {
      System.out.format("%d - %s%n", candidate, spell(candidate));
    }
  }

}
\$\endgroup\$
8
\$\begingroup\$

For the input -2147483648, your program outputs just minus . Also, you never handle 0 anywhere.

In convert(), I personally wouldn't bother with int units = num;, and I'd just use num.

Instead of instantiating a StringBuilder in convertHundredsImpl(), I'd just pass the original StringBuilder to the helper function. For consistency then, I'd also pass a StringBuilder to the convertUnitsImpl() helper. In accordance with the design of the StringBuilder class, I'd have those helpers return the same StringBuilder as well, to allow for fluent chaining, like convertHundredsImpl(sb, thousands).append(" thousand ").

The while (true) loop in main() gives you no graceful way to exit. It should be while (scanner.hasNextInt()) instead.

\$\endgroup\$
4
\$\begingroup\$

Naming

class IntToHumanReadableStringConverter {

I would consider "123" to be a human readable string of an int. But that's not what you output. You input numeric values like that and output words. I.e. I'd describe this as "Convert numeric input to words". Or

class NumericToWordsConverter {

Similarly,

    private static final Map<Integer, String> TENS_MAP = new HashMap<>();

Your TENS_MAP includes "tens" but also other numbers. Consider

    private static final Map<Integer, String> WORDS = new HashMap<>();

If you want to be even clearer about what it contains, NUMBER_WORDS. Although I think that makes the code read less well most other places. E.g. WORDS.get(5) is clearer to me than NUMBER_WORDS.get(5).

Needless indirection

    private static String convertUnitsImpl(int unit) {
        return TENS_MAP.get(unit);
    }

What does "convert units impl" mean? My first thought was that it was going to convert to or from imperial units. But no, more inspection of the code reveals that what you are calling units is numbers like thousands, millions, and billions. And I'm guessing that "impl" is short for implementation. In Java, it is common to use "Impl" to denote an implementation of an interface where the most reasonable name of the implementation is the same as the interface. But that's not what you are doing here.

A simpler name would be either just convertUnits or convertToWords. But in truth, you don't need that. A simple WORDS.get(unit) is going to be more readable than convertUnitsImpl(unit) or any of the alternatives. And that's even though unit itself is not particularly readable. It seems like there should be a better word for that, although I don't know what it is. Multiplier might be better than units but is still relatively generic.

The point of making an alias function like this is to make the code more readable. But in this case, the original code is about as readable as it's going to get. The alias just makes it harder to understand what is really happening.

DRY

        int billions = num / 1_000_000_000;
        
        num -= billions * 1_000_000_000;
        
        int millions = num / 1_000_000;
        
        num -= millions * 1_000_000;
        
        int thousands = num / 1_000;
        
        num -= thousands * 1_000;
        
        int units = num;
                
        if (billions > 0) {
            sb.append(convertUnitsImpl(billions));
            sb.append(" billion ");
        }
        
        if (millions > 0) {
            sb.append(convertHundredsImpl(millions));
            sb.append(" million ");
        }
        
        if (thousands > 0) {
            sb.append(convertHundredsImpl(thousands));
            sb.append(" thousand ");
        }
        
        if (units > 0) {
            sb.append(convertHundredsImpl(units));
        }

You do the same thing three times (and almost do it a fourth time).

Consider

        if (num == 0) {
            return "zero";
        }

        Integer multiplier = 1;
        Deque<Pair<String, Integer>> parts = new ArrayDeque<>();
        do {
            parts.addLast(new Pair<String, Integer>(WORDS.get(multiplier), num % 1000));
            num /= 1000;
            multiplier *= 1000;
        } while (num > 0);

        while (parts.size() > 1) {
            Pair<String, Integer> pair = parts.removeLast();
            sb.append(convertHundredsImpl(pair.getValue()))
              .append(" ").append(pair.getKey()).append(" ");
        }

        sb.append(convertHundredsImpl(parts.removeLast().getValue()));

Now the repeated code has moved into two loops. And if we change from int to long or BigInteger, the logic would stay the same (although for BigInteger, we'd have to write it differently). Obviously we'd need more WORDS for either.

I do not consider parts or pair to be great names but didn't think of better ones while writing this. I also don't like convertHundredsImpl but don't have a great replacement. As previously discussed, I do think convertHundreds would be better. But perhaps convertSmall or similar would be even better.

I did not make a constant for 1000, mostly because I don't have a good name. Because the WORDS are hardcoded, we're kind of stuck with it. So there's no point in making it variable. It might make sense to move 1000 (as a constant) and the WORDS into a separate class. Then pass that class to this at construction time. In that case, we might return WORDS.get(0) rather than "zero".

The check for 0 could go higher in the method, although it should probably still go after the check for negatives if you don't convert up to long before the negative check. If you handle negatives correctly, it could go first in the method.

This code still won't handle the maximally negative value unless you convert the int up to long. It does handle 0 now.

We know that num will be greater than zero on entry to the do/while loop. It's not zero because we checked for that explicitly. It's not less than zero, because if it had been, you negate it (prior to this code).

\$\endgroup\$
0
\$\begingroup\$

Comment by @PeterCsala:

It is minor, but instead of these num -= billions * 1_000_000_000; calculations you could use simply num %= 1_000_000_000; compound assignment operator.

\$\endgroup\$
1
  • 2
    \$\begingroup\$ @PeterCsala If you care to repost this answer under your own name, we can delete this placeholder. \$\endgroup\$ Jun 16 at 20:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.