4
\$\begingroup\$

I am trying to use Java8 for a piece of code which I had written in Java6 and am able to achieve the result using both the version of java but I am thinking there could be more better way to write the code which I written in Java8

// Find vowels count in each given country names
// return map of country name and vowel count

//Given -- ['New Zealand', 'Australia'] 
//Result -- {New Zealand=4, Australia=5}

    List<String> countryNames = new ArrayList<>();
    countryNames.add("New Zealand");
    countryNames.add("Australia");

    List<String> VOWELS = Arrays.asList("a", "e", "i", "o", "u");
    Map<String, Integer> countryWithVowelCount = new HashMap<String, Integer>();

    //Using Java6       
    for (String name : countryNames) {
        int j = 0;

        for (int i = 0; i < name.length(); i++) {
            if (VOWELS.contains("" + name.toLowerCase().charAt(i))) {

                if (countryWithVowelCount.containsKey(name)) {
                    j = countryWithVowelCount.get(name);
                    countryWithVowelCount.put(name, ++j);
                } else {
                    countryWithVowelCount.put(name, 1);
                }
            }

        }

    }
    
    // Using Java8
    Map<Object, String> result = countryNames.stream()
            .collect(Collectors.groupingBy(cname -> cname, Collectors.mapping(cname -> {
                int j = 0;
                for (int i = 0; i < cname.length(); i++) {
                    if (VOWELS.contains("" + cname.toLowerCase().charAt(i))) {
                        ++j;
                    }
                }
                return "" + j;
            }, Collectors.joining(""))));
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Since we are in code review, it is necessary to remind that the definition of a vowel varies from language to language. It seems that you are working with english language, so it should be mentioned in the names of relevant classes and/or methods. \$\endgroup\$ Nov 15 '21 at 5:18
4
\$\begingroup\$

Welcome to Stack Review, the first thing I see in the code is the following:

List<String> VOWELS = Arrays.asList("a", "e", "i", "o", "u");

This is correct, but from the uppercase VOWELS name you chose for the variable it seems me you meant this as a constant, so you could declare it :

private static final String VOWELS = "aeiou";

I chose to modify the type to String from List because it seems me more easy to use, so for example you can isolate your code counting the vowels in a separate function like below :

private static int countVowels(String cname) {
    final char[] arr = cname.toLowerCase().toCharArray();
    int j = 0;
    for (char c : arr) {
        if (VOWELS.indexOf(c) != -1) {
            ++j;
        }
    }
    return j;
}

I just substituted your VOWELS.contains("" + cname.toLowerCase().charAt(i)) line with the indexOf string function because I'm seeing that the char is present or not in the VOWELS string.

The more important part in your code is the part about when you streams the list and convert it to a Map : you can avoid the Collectors.groupingBy and stream it directly like below :

Map<String, Integer> result = countryNames.stream()
 .collect(Collectors.toMap(Function.identity(), cname -> countVowels(cname)));

Or directly delete the countVowels function and the VOWELS constant and do all the process in one line :

Map<String, Integer> result = countryNames.stream()
  .collect(Collectors.toMap(Function.identity(), cname -> cname.toLowerCase().replaceAll("[^aeiou]","").length()));
\$\endgroup\$
2
  • 1
    \$\begingroup\$ I think one typo is there in the line .collect(Collectors.toMap(Function.identity(), cname -> countVowels(VOWELS))); instead of passing VOWELS it should be passed cname, Thanks for the review and provided a alternate and better solution \$\endgroup\$
    – Abs
    Nov 15 '21 at 4:30
  • \$\begingroup\$ @AbdullahQayum You are welcome and thank you for your observation about the typo, I haven't noticed it. \$\endgroup\$ Nov 15 '21 at 7:08
5
\$\begingroup\$

Functions...

When you're writing code, even when you're just playing about, it's usually a good idea to put the code into a function. This helps you to think about what the interface to the code should be, what it's dependencies are and what it should be called. Based on the code, I'd go with a function declaration something like:

Map<String, Integer> getVowelCounts(List<String> countryNames) {

Version difference

Splitting the code like this, it becomes evident that there's a different between the results of the J6 and J8 code versions. Whilst the J6 version does indeed return a Map<String, Integer>, the J8 version returns Map<Object,String>. It's not clear why this discrepancy is there. It doesn't really seem to make sense, but perhaps you want the vowel count in a string for some reason...

Naming

I'm not a huge fan of single letter variable names. Using something like for(int i... can be ok, if it's the only iterator in a block of code. As soon as you start introducing more iterations, or other single letter variables it starts to become confusing as to which i is for the inner iteration and which j is for the vowel count. Consider being more expressive with your names.

Break is down...

Counting the vowels in a string seems like something that you might want to do other than when counting the vowels in a list of countries. To me, this suggests that it should be its own function.

Grouping

I'm not sure groupingBy is the correct usage here. You're not really grouping, you're using it as a convenient way to turn the result into a map. If you were to pass in a duplicate country, you start getting some pretty unexpected results. For example, adding in another 'New Zealand', rather than producing the correct '4' vowels, or the '8' vowels which the J6 version returns, it returns '44' (4 from the first instance appended with 4 from the second instance). Instead it might be better to use toMap. This will actually throw an exception if you try to process a list that has multiple instances of the same country, so that you know your source data might be corrupted. Or, you can use distinct in combination to allow multiples in the source data.

Putting it together

You end up with something like:

Map<String, Integer> getVowelCounts(List<String> countryNames) {

    return  countryNames.stream()
            .distinct()  // Do you want to support multiple instances of countryNames?
            .collect(Collectors.toMap(name -> name, this::vowelCount));
}


static Set<Integer> vowels = "aeiouAEIOU".codePoints()
                                         .boxed().collect(Collectors.toSet());

int vowelCount(String str) {
    return str.codePoints()
              .map(c -> vowels.contains(c) ? 1 : 0)
              .sum();
}
\$\endgroup\$
4
  • \$\begingroup\$ I think the method codePoints is from Java9, Thank you for the review and providing alternate better solution \$\endgroup\$
    – Abs
    Nov 15 '21 at 4:31
  • \$\begingroup\$ @AbdullahQayum Both chars and codePoint are Java8, They're part of the CharSequence interface: docs.oracle.com/javase/8/docs/api/java/lang/CharSequence.html \$\endgroup\$
    – forsvarir
    Nov 15 '21 at 11:19
  • 2
    \$\begingroup\$ Why the mapToObj then the mapToInt? They add unnecessary cognition and complexity while .map(codePoint -> vowels.contains((char) codePoint) ? 1 : 0) is very legible. Also, You cast a codePoint to char, so why not use chars() in the first place? Better yet, why not have a Set<Integer> vowelCodePoints? Then the code can easily become str.codePoints().filter(vowelCodePoints::contains).count(). \$\endgroup\$ Nov 15 '21 at 12:18
  • \$\begingroup\$ @OlivierGrégoire Development, at least for me, is an iterative process. My original version has mapToObj->map->mapToInt and at the time I didn't immediately think of a clean way to create a set of code points, (int)'a' wasn't very satisfactory. Your filter/count is probably more expressive than my map/sum. \$\endgroup\$
    – forsvarir
    Nov 15 '21 at 20:12
2
\$\begingroup\$

Some of my suggestions won't necessarily make sense for this specific example, but for the general idea of counting occurrences of a set of characters in a string, they might scale better to bigger workloads.

public class StringUtils {
    private static final Set<Character> ENGLISH_VOWELS = new HashSet<>('a', 'e', 'i', 'o', 'u', 'A', 'E', 'I', 'O', 'U');


    public static Integer countOccurrences(final String container, final Set<Character> toCount) {
        return container
                   .chars()
                   .reduce(0,
                           (subtotal, element) ->
                                subtotal + (toCount.contains(element) ? 1 : 0));
    }

    public static Map<String, Integer> countOccurrencesMultiple(final List<String> containers, final Set<Character> toCount) {
        return containers
                   .stream()
                   .collect(Collectors.toMap(Function.identity(), 
                                             StringUtils::countOccurrences));
    }

    public static Integer countVowelOccurrences(final String container) {
        return countOccurrences(container, ENGLISH_VOWELS);
    }

    public static Map<String, Integer> countVowelOccurrencesMultiple(final List<String> containers) {
        return countOccurrencesMultiple(containers, ENGLISH_VOWELS);
    }
}

The main ideas here are:

  1. Make the characters you're going to count the occurrences of a Set, instead of any other Iterable type, including a String or an array. Iterating over all of your characters to count for every character in your test strings is fine for now but if your list of characters has to grow in the future (maybe count vowels in multiple languages?), the constant-time presence check characteristic of Set could become useful. Plus, it automatically takes care of any duplicate entries.
  2. Declare your parameters final, so that you don't reassign to them (mutation of variables is not a great idea in functional code).
  3. reduce is really powerful!
  4. Try to solve your problems in as general a way as possible, but not too general!
\$\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.