40
\$\begingroup\$

WE_ALL_LOVE_SHOUTY_SNAKE_CASE_SO_I_WROTE_A_PROGRAM_FOR_IT!!1!

Alright, enough of this. This is a from this question. The pattern is pretty simple, it takes a String input, verifies if it is numerical, and if so, converts it to SHOUTY_SNAKE_CASE.

Example : "1231" -> "ONE_TWO_THREE_ONE"

It is fairly simple but it is also the first time I used the Java 8's streams, so I wanted an opinion on my (simple) use of it.

private static final String[] words = 
        new String[]{"zero","one","two","three","four","five","six","seven","eight","nine"};

public static String convertNumbersToWords(final String input) {

    if(input == null || !input.matches("^\\d*$")){
        throw new IllegalArgumentException("input cannot be null or non-numerical.");
    }

    return input.chars()
             .mapToObj(c -> getWordFromCharCode(c).toUpperCase())
             .collect(Collectors.joining("_"));
}

private static String getWordFromCharCode(int code){
    return words[code - 48];
}
\$\endgroup\$
5
  • 10
    \$\begingroup\$ Surprisingly few SHOUTY_SNAKE_CASE variables and text in this code?! \$\endgroup\$
    – holroy
    Commented Nov 27, 2015 at 15:26
  • 29
    \$\begingroup\$ I came here expecting this to be PCG, not CR. Disappointed. \$\endgroup\$
    – fluffy
    Commented Nov 27, 2015 at 18:40
  • 15
    \$\begingroup\$ @fluffy It's simple now. You see a logo that means "not fully graduated", you're on PCG :p \$\endgroup\$
    – IEatBagels
    Commented Nov 27, 2015 at 18:42
  • 1
    \$\begingroup\$ @holroy It's Java, not LOLCODE. \$\endgroup\$ Commented Nov 27, 2015 at 21:43
  • 3
    \$\begingroup\$ ArnoldC is definitely made for shouty stuff \$\endgroup\$
    – Doddy
    Commented Nov 28, 2015 at 12:15

6 Answers 6

22
\$\begingroup\$

Since you need an enumerated set of LOUD_WORDS that aligns with numbers, preferably as some form of numeric indices... an enum instead of String[] seems to be the perfect fit:

enum Word {
    ZERO, ONE, TWO, THREE, FOUR, FIVE, SIX, SEVEN, EIGHT, NINE;
}

(I think you can figure out the derivation.)

edit: Just one note about using Word.values() as pointed out in @Simon's comments: it creates a new array each time and this may not be desirable, performance-wise.

Since you're also experimenting with some of the Java 8 features, you may want to consider using an Optional too, to eliminate the if-validation:

private static final String NUMBERS_ONLY = "^\\d+$";

// Usage
Optional.ofNullable(input)
        .filter(v -> v != null && v.matches(NUMBERS_ONLY))
        .orElseThrow(IllegalArgumentException::new)
        .chars()
        // ...

If you prefer to throw NullPointerException for null values, you can stick with Optional.of(T).

\$\endgroup\$
4
  • \$\begingroup\$ How would you recommend to map from the digit to the String? Do a Word.values()[digit] for each call? \$\endgroup\$ Commented Nov 27, 2015 at 21:47
  • \$\begingroup\$ @SimonForsberg yeah, .map(i -> Character.getNumericValue(i)).mapToObj(i -> Word.values()[i].toString())... \$\endgroup\$
    – h.j.k.
    Commented Nov 28, 2015 at 3:12
  • 2
    \$\begingroup\$ @h.j.k. Word.values() creates a new array each and every time it is called. Storing all the values in one array once sounds better to me, and it's not like you'll create a Word w = Word.THREE; or anything. Sounds like a String[] is better to me. \$\endgroup\$ Commented Nov 28, 2015 at 10:13
  • 4
    \$\begingroup\$ I'd be a bit wary of using an enum to represent actual output values. For example, this could cause problems should you ever wish to support other languages with non-Latin characters. Using a String[] (or even Map<Integer, String>) also feels more semantically correct to me, though that's arguable. \$\endgroup\$
    – Bob
    Commented Nov 29, 2015 at 0:36
36
\$\begingroup\$

Why are you storing them as lowercase words only to call toUpperCase on them? Store them in the form you intend to use them and save yourself a function call on each word. It's also more understandable to have the strings stored the same way they'll be printed.

\$\endgroup\$
21
\$\begingroup\$

I would propose replacing the kinda low-level code - 48 with Character.getNumericValue.

Another consideration might be to use guava's CharMatcher.DIGIT.matchesAllOf instead of the regex for validating the input string, though I realize that using libraries is often besides the point for CR.stackexchange. Note that this approach would pass you digits with numeric values bigger than 9 as well (i.e. roman numeral 50) and you'd have to think of a way to support them. (Split to single digits?)

\$\endgroup\$
4
  • 1
    \$\begingroup\$ Proposing libraries is a good idea! :) It's just that you need to be cautious not to center the whole review over the use of an external library. But your answer is good, thanks! :) \$\endgroup\$
    – IEatBagels
    Commented Nov 27, 2015 at 15:40
  • 5
    \$\begingroup\$ Can code - 48 be replaced with code - '0'? (which is still not perfect, but better IMO) \$\endgroup\$
    – oliverpool
    Commented Nov 27, 2015 at 16:31
  • 1
    \$\begingroup\$ @oliverpool: programmers.stackexchange.com/questions/303553/… :) \$\endgroup\$ Commented Nov 27, 2015 at 16:44
  • \$\begingroup\$ Filter with Character.isDigit, convert with Character.getNumericValue, then filter again to restrict to the range 0 - 9. \$\endgroup\$ Commented Nov 27, 2015 at 20:16
12
\$\begingroup\$

Naming and method decomposition

Let's take a look at this method signature:

public static String convertNumbersToWords(final String input) {

The method name suggests converting numbers... And it works with inputs like "1234". It's a bit unnatural that the input is a string, as opposed to a numeric type, and "1234" is one number, not plural numbers.

In addition, the method does two things:

  • validate the input consists of digits only
  • convert the digits to SHOUT_CASE

It would be better to decompose this to two methods, for example:

private static String convertValidatedDigitsToWords(final String digits) {
    return digits.chars()
            .mapToObj(ShoutySnake::getWordFromCharCode)
            .collect(Collectors.joining("_"));
}

public static String convertDigitsToWords(final String input) {
    if (input == null || !input.matches("\\d*")) {
        throw new IllegalArgumentException("input cannot be null or non-numerical.");
    }
    return convertValidatedDigitsToWords(input);
}

... and perhaps add an overloading for numeric input:

public static String convertDigitsToWords(final int number) {
    if (number < 0) {
        throw new IllegalArgumentException("input must be non-negative");
    }
    return convertValidatedDigitsToWords(Integer.toString(number));
}

A small related tip: String.matches implies ^ and $, no need to include that in the pattern.

Optimizing for repeated calls

If you expect the method to be called repeatedly, then it would be good to compile the regular expression.

private static final Pattern pattern = Pattern.compile("\\d*");

public static String convertDigitsToWords(final String input) {
    if (!pattern.matcher(input).matches()) {
        throw new IllegalArgumentException("input must be non-null and non-negative.");
    }
    return convertValidatedDigitsToWords(input);
}

Magic number 48

In this code, 48 is a magic number:

private static String getWordFromCharCode(int code){
    return words[code - 48];
}

You can easily make it less magic by referring to '0' instead:

private static String getWordFromCharCode(int code) {
    return words[code - '0'];
}
\$\endgroup\$
2
  • \$\begingroup\$ Awesome, combine this with @benjaminssp's advice to replace code - 48 with Character.getNumericValue and I think you've got your answer :) \$\endgroup\$ Commented Nov 28, 2015 at 18:16
  • \$\begingroup\$ Thanks @user1477388, your comment inspired me to add another section ;-) (Magic number 48) \$\endgroup\$
    – janos
    Commented Nov 28, 2015 at 18:22
10
\$\begingroup\$

The name convertNumbersToWords() does not clearly reflect what the method is doing. You can either do this using the java convention like convertNumbersToShoutySnakeCaseWords() or in a way it matches the purpose of the application like CONVERT_NUMBERS_TO_SHOUTY_SNAKE_CASE_WORDS()

\$\endgroup\$
2
  • 10
    \$\begingroup\$ snakeShouts() could also be an option if you value humor above professionalism \$\endgroup\$
    – Aaron
    Commented Nov 27, 2015 at 15:02
  • 10
    \$\begingroup\$ Just because the code is about shouty and snake case things, doesn't mean that the code itself should be shouty and snakey. \$\endgroup\$ Commented Nov 27, 2015 at 21:46
10
\$\begingroup\$

I would suggest to change three thinks which haven't been mentioned yet.

  1. Your regex works but is not what I call "easy on the eyes". What about the following:

    ^[0-9]*$
    

    When I read something like this I immediately think: "Some regex with number filters."

  2. Also same topic. Do you actually want to allow empty inputs? I would suggest not. Replace * with +:

    ^[0-9]+$
    ^\\d+$
    
  3. I like static imports. It is not everyone's favourite, but in my opinion, it makes the code more readable. That is why this would be my final solution:

    import java.util.Optional;
    import static java.util.stream.Collectors.joining;
    
    public class ShoutySnake {
    
        private enum WORDS{ZERO,ONE,TWO,THREE,FOUR,FIVE,SIX,SEVEN,EIGHT,NINE}
    
        private static final String NUMBERS_ONLY = "^[0-9]+$";
    
        private static String getNumericalCharAsSnakeShoutCaseWord(final int codePoint){
            return WORDS.values()[Character.getNumericValue(codePoint)].name();
        }
    
        public static String convertNumbersToShoutySnakeCaseWords(final String input) {
            return Optional.ofNullable(input)
                .filter(s -> s.matches(NUMBERS_ONLY))
                .orElseThrow(() -> new IllegalArgumentException("input cannot be null, empty or non-numerical."))
                .chars()
                .mapToObj(ShoutySnake::getNumericalCharAsSnakeShoutCaseWord)
                .collect(joining("_"));
        }
    }
    
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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