5
\$\begingroup\$

Here is a simplified implementation to obtain the duplicate words in a text using lambda expressions.

public class FindDuplicateWordsInText {

    public static Set<String> findDuplicateWordsInText(String text) {
        String[] words = text.split(" ");
        Set<String> duplicatesRemovedSet = new HashSet<>();
        Set<String> duplicatesSet = Arrays.stream(words).filter(string -> !duplicatesRemovedSet.add(string))
                .collect(Collectors.toSet());
        return duplicatesSet;
    }
}
\$\endgroup\$
  • \$\begingroup\$ what is your main concern? \$\endgroup\$ – Malachi Aug 12 '15 at 19:16
  • \$\begingroup\$ Lambda, not lamba \$\endgroup\$ – Nic Hartley Aug 13 '15 at 3:54
4
\$\begingroup\$

Your use of the boolean return value of the Set.add() call is a clever way to check for your duplicates. The concept you have is good, and I can't think of a faster way.

Additionally, I like how you have used Interface-based types on the left-side of assignments Set<String> and the concrete classes on the right new HashSet<>() .... people often put the concrete type on the left too, and it's good to see that you did not.

In terms of the Java streaming API, though, I can't help but feel that you missed out on an opportunity to improve the process by streaming the split.... The Pattern class has a splitAsStream method which would reduce your latency on the first words....

As an aside, a word should probably be on a contiguous whitespace, not just a single space (i.e. "\\s+" instead of " ").

Here's your code done differently:

private static final Pattern SPACE = Pattern.compile("\\s+");

public static Set<String> findDuplicateWordsInText(String text) {
    Set<String> duplicatesRemovedSet = new HashSet<>();
    return SPACE.splitAsStream(text)
            .filter(string -> !duplicatesRemovedSet.add(string))
            .collect(Collectors.toSet());
}
\$\endgroup\$
2
\$\begingroup\$

The implementation seems fine. I would change the return type of the method to be a bit more general. Callers don't care about whether the duplicates are stored in a Set or not.

public static Collection<String> findDuplicateWordsInText(String text) {
    String[] words = text.split(" ");
    Set<String> duplicatesRemovedSet = new HashSet<>();
    List<String> duplicates = Arrays.stream(words).filter(
            string -> !duplicatesRemovedSet.add(string)).collect(Collectors.toList());
    return duplicates;
}

But honestly, using stream/filter doesn't make it much clearer. Why not stick with the standard approach?

public static Collection<String> findDuplicateWordsInText(String text) {
    String[] words = text.split(" ");
    List<String> duplicates = new ArrayList<>();
    Set<String> duplicatesRemovedSet = new HashSet<>();
    for (String word: words) {
        if (!duplicatesRemovedSet.add(word)) {
            duplicates.add(word);
        }
    }       
    return duplicates;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ A Set has no duplicates. :) \$\endgroup\$ – h.j.k. Aug 12 '15 at 23:42
  • 1
    \$\begingroup\$ "Callers don't care about whether the duplicates are stored in a Set or not" - what if I want to find duplicate words and then pass them into another method that accepts a Set<String>? Callers benefit when a method has general input types and a specific output type. \$\endgroup\$ – Andrew McNamee Aug 13 '15 at 4:59
1
\$\begingroup\$

Set.add(...) has side effects by nature, which means that your filter method isn't "pure". In the given example, I don't see a concrete downside, but if you later wanted to make the stream parallel, you'd need to use a concurrent Set.

For comparison, here's a side-effect free solution using Guava's Multiset:

public static Set<String> findDuplicateWordsInText(String text) {
  Multiset<String> wordMultiset =
      ImmutableMultiset.copyOf(Splitter.on(' ').split(text));
  return wordMultiset.entrySet()
      .stream()
      .filter(entry -> entry.getCount() > 1)
      .map(Multiset.Entry::getElement)
      .collect(Collectors.toSet());
}

It probably doesn't perform as well, but the intent should (hopefully) come across clearly.

\$\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.