39
\$\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\$
  • 10
    \$\begingroup\$ Surprisingly few SHOUTY_SNAKE_CASE variables and text in this code?! \$\endgroup\$ – holroy Nov 27 '15 at 15:26
  • 29
    \$\begingroup\$ I came here expecting this to be PCG, not CR. Disappointed. \$\endgroup\$ – fluffy Nov 27 '15 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 Nov 27 '15 at 18:42
  • 1
    \$\begingroup\$ @holroy It's Java, not LOLCODE. \$\endgroup\$ – Simon Forsberg Nov 27 '15 at 21:43
  • 3
    \$\begingroup\$ ArnoldC is definitely made for shouty stuff \$\endgroup\$ – Doddy Nov 28 '15 at 12:15
21
\$\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\$
  • \$\begingroup\$ How would you recommend to map from the digit to the String? Do a Word.values()[digit] for each call? \$\endgroup\$ – Simon Forsberg Nov 27 '15 at 21:47
  • \$\begingroup\$ @SimonForsberg yeah, .map(i -> Character.getNumericValue(i)).mapToObj(i -> Word.values()[i].toString())... \$\endgroup\$ – h.j.k. Nov 28 '15 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\$ – Simon Forsberg Nov 28 '15 at 10:13
  • 3
    \$\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 Nov 29 '15 at 0:36
37
\$\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\$
  • 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 Nov 27 '15 at 15:40
  • 5
    \$\begingroup\$ Can code - 48 be replaced with code - '0'? (which is still not perfect, but better IMO) \$\endgroup\$ – oliverpool Nov 27 '15 at 16:31
  • 1
    \$\begingroup\$ @oliverpool: programmers.stackexchange.com/questions/303553/… :) \$\endgroup\$ – Benjaminssp Nov 27 '15 at 16:44
  • \$\begingroup\$ Filter with Character.isDigit, convert with Character.getNumericValue, then filter again to restrict to the range 0 - 9. \$\endgroup\$ – David Conrad Nov 27 '15 at 20:16
11
\$\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\$
  • \$\begingroup\$ Awesome, combine this with @benjaminssp's advice to replace code - 48 with Character.getNumericValue and I think you've got your answer :) \$\endgroup\$ – user1477388 Nov 28 '15 at 18:16
  • \$\begingroup\$ Thanks @user1477388, your comment inspired me to add another section ;-) (Magic number 48) \$\endgroup\$ – janos Nov 28 '15 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\$
  • 10
    \$\begingroup\$ snakeShouts() could also be an option if you value humor above professionalism \$\endgroup\$ – Aaron Nov 27 '15 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\$ – Simon Forsberg Nov 27 '15 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, privacy policy and cookie policy

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