6
\$\begingroup\$

Task

Write an implementation that returns whether a String has vowels (English language) in alphabetical (A-Z) order or not.

Feedback

  • Is my vowels variable appropriate? Is there a better way to implement a vowel lookup?
  • If a candidate String has no vowels, the validator will return true - is that reasonable? From a logical perspective, the program isn't lying, but you could make the same argument if it returned false. Would it be better to return an exception - something like NoVowelsInCandidateException?

Implementation

public class VowelOrderValidator {
    private static final List<Character> vowels = new ArrayList<>(
            Arrays.asList('A', 'E', 'I', 'O', 'U', 'Y')
    );

    public static boolean areVowelsOrdered(final String candidate) {
        final char[] chars = candidate.toUpperCase().toCharArray();
        int lastVowelIndex = 0;
        for (final char c : chars) {
            final int lookupIndex = vowels.indexOf(c);
            if (-1 != lookupIndex) {
                if (lookupIndex < lastVowelIndex) {
                    return false;
                } else {
                    lastVowelIndex = lookupIndex;
                }
            }

        }

        return true;
    }
}
\$\endgroup\$
  • \$\begingroup\$ As a side-note, matching the lowercase candidate against the following regular expression would work :) : "[^aeiouy]*(?:a[^eiouy]*)?(?:e[^aiouy]*)?(?:i[^aeouy]*)?(?:o[^aeiuy]*)?(?:u[^aeioy]*)?y?[^aeiou]*" (but I find it really less readable!) \$\endgroup\$ – Tunaki Feb 7 '16 at 18:39
  • 1
    \$\begingroup\$ Not a review, but I suggest Java 8, in cases like this it cuts down a ton of code. \$\endgroup\$ – Caridorc Feb 7 '16 at 21:48
7
\$\begingroup\$
  • Since vowels is a constant for your program, the convention would be that its name is in all uppercase, like VOWELS.
  • Take great care when uppercasing Strings:

    final char[] chars = candidate.toUpperCase().toCharArray();
    

    Uppercasing a String depends on the locale. By not specifying a locale, you are relying on the default locale of the JVM, which may cause problems for Turkish users (for example). It would be better to specify the ROOT locale:

    final char[] chars = candidate.toUpperCase(Locale.ROOT).toCharArray();
    
  • I'm not a big fan of inversing the conditions in an if:

    if (-1 != lookupIndex) {
    

    The following would be easier to read:

    if (lookupIndex != -1) {
    

    In fact, it would be even better to not check against -1 but test that the result is positive:

    if (lookupIndex >= 0) {
    

    It decouples the fact that indexOf returns -1 from what you really want to test, which is that the index was found.

  • I think returning true when there is no vowels is the least surprising result: when there are no vowels, then they all are in the right order. Throwing an exception would be a bad idea: no vowels in a String is not a real exceptional condition.
\$\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.