Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have a collection of phrases in a List. Each phrase is a String Array where each element in the array is a word.

I create a List<Entry<String, Integer>> which holds the words as keys and the times used as value. Everything is sorted by value in descending order.

Then what I do is print the top X words that were used along with how many times they were used. What I want to know is if there is a better/simpler way of doing it and generally anything you want to add to make my code look or perform better.

Here is my code:

public class WordCounting {

    public static void printTopWords(final int numberOfWords, List<String[]> phrases) {
        List<Entry<String, Integer>> wordsMap = entriesSortedByValues(wordCount(phrases));

        Iterator entries = wordsMap.iterator();
        int wordsCounter = 1;
        while (entries.hasNext() && wordsCounter <= numberOfWords) {
            Entry entry = (Entry) entries.next();
            String key = (String) entry.getKey();
            int value = (int) entry.getValue();

            System.out.println(wordsCounter + ": " + key + " - " + value);
            wordsCounter++;
        }
    }

    private static Map<String, Integer> wordCount(List<String[]> phrases) {
        Map<String, Integer> wordCounter = new TreeMap<>();

        for (String[] strings : phrases) {
            for (String string : strings) {
                wordCounter.put(string, wordCounter.get(string) == null
                        ? 1 : wordCounter.get(string) + 1);
            }
        }
        return wordCounter;
    }

    static <K, V extends Comparable<? super V>>
            List<Entry<K, V>> entriesSortedByValues(Map<K, V> map) {

        List<Entry<K, V>> sortedEntries = new ArrayList<>(map.entrySet());

        Collections.sort(sortedEntries,
                new Comparator<Entry<K, V>>() {
                    @Override
                    public int compare(Entry<K, V> e1, Entry<K, V> e2) {
                        return e2.getValue().compareTo(e1.getValue());
                    }
                }
        );

        return sortedEntries;
    }
}
share|improve this question
2  
Any special reason you are using a List<Entry<...>> instead of a Map? –  RoToRa Apr 22 at 14:28
    
@RoToRa because I've read that a Map wouldn't handle the equal valued correctly because of the entriesSortedByValues which I just posted. –  Sillicon Touch Apr 22 at 14:30
    
@SilliconTouch To be honest you are not doing anything noteworthy in that while loop, except you are explicitly using an iterator instead of a for (Entry enty: map) { loop. –  RoToRa Apr 22 at 14:31
    
I wanted a for loop but I thought it would be better to use a while since I have two conditions. –  Sillicon Touch Apr 22 at 14:32
    
See stackoverflow.com/questions/109383/… about how to sort a map by value. –  RoToRa Apr 22 at 14:35

3 Answers 3

up vote 12 down vote accepted

As others have pointed out the most obvious improvements already, I would like to talk about Java 8. Maybe you cannot use it yet, but I would recommend looking into it and this answer should proivde useful for anyone reading this.

My main focus point here will be designing the code such that it will logically do what it is supposed to do.

You have a method in which you take a List<String[]> as argument, and you want to return the top x occurences, that is all you want, in order that means:

  1. Put all Strings together in some structure.
  2. Keep track of how many occurences they have.
  3. Sort the list to have the words with the highest amount of occurences first.
  4. Return the top x occurences.
  5. Then decide what you want to do with it.

Issue 4 and 5 are seperated explicitely here, because that is what you ought to do. Every method should serve one purpose, and doing something and printing it is not one purpose.

Another improvement is that as input I want a structure consisting of Strings, and not your (ugly) List<String[]>, I will also deal with that. The code will be fully explained below. While we are at it, I'll also improve the error checking.

The code:

public static Stream<Map.Entry<String, Long>> getTopWords(final int topX, final Stream<String> words) {
    if (topX < 1) {
        throw new IllegalArgumentException("invalid value for topX: " + topX);
    }
    Objects.requireNonNull(words);
    Comparator<Map.Entry<String, Long>> comparator = Comparator.comparingLong(Map.Entry::getValue);
    return words.collect(Collectors.groupingBy(i -> i, Collectors.counting()))
            .entrySet().stream()
            .sorted(comparator.reversed())
            .limit(topX);
}

List<String[]> phrases = Arrays.asList(new String[]{"a", "b", "c"}, new String[]{"a", "a", "b", "d"});
List<Map.Entry<String, Long>> topEntries = getTopWords(2, phrases.stream().flatMap(Arrays::stream))
        .collect(Collectors.toList());
int counter = 1;
for (Map.Entry<String, Long> entry : topEntries) {
    System.out.println(counter + ": " + entry.getKey() + " - " + entry.getValue());
    counter++;
}

The output:

1: a - 3
2: b - 2

The explanation:

  1. You have your List<String[]> phrases first, you want to simply have an object that holds your Strings. Here a Stream<String> is a suitable object, because you only need to have a view on your phrases object, there is no point in actually storing the new data. You do this by calling phrases.stream().flatMap(Arrays::stream).
    1. This will first turn your List<String[]> in a Stream<String[]>.
    2. Then you use a method reference that describes the lambda stringArray -> Arrays.stream(stringArray) to acquire a Stream<String>.
    3. Then with the flatMap you add all elements of the resulting Stream<String> back into the original Stream<String>.
  2. Then you start at the getTopWords method, which returns a Stream<Map.Entry<String, Long>>, again to offer the flexibility to do what you want with the results, they are not stored yet at the point where it gets returned.
  3. First I added some error checking.
  4. Then I obtain a Comparator<Map.Entry<String, Long>> that will compare what the entry is with the lowest number of occurences. This is done by using a Comparator.comparingLong on the value of the entry, which is obtained with the method reference Map.Entry::getValue.
  5. Then I start the chain of operations on the input Stream<String>:
    1. First group the results by their identity, which normally produces a Map<String, List<String>>.
    2. The trick here is that I also used a downstream Collector, which counts the number of times the string occurs, hence it is called Collectors.counting().
    3. At this point I have a Map<String, Long> denoting the word and the number of occurences. It uses a Long, because this is what Collectors.counting() returns.
    4. Then I obtain a Set<Map.Entry<String, Long>> and convert it into a stream.
    5. Then I call sorted() on the Stream<Map.Entry<String, Long>> with the reversed comparator. This is done here, because type interference is not strong enough to use Comparator.comparingLong(Map.Entry::getValue).reversed().
    6. Then I limit() the stream by the top x elements.
  6. Now we have the Stream<Map.Entry<String, Long>>, and here we decide to collect it into a List<Map.Entry<String, Long>>.
  7. Here we continue with your old logic of having a counter attached to it.

A few points that are worth to note:

  1. The explicit comparator.reversed() is ugly, but neccessary to not result in type casting, which is even more ugly, it is a limitation of the current type interference. It might be only an issue in IDE's and the javac compiler might actually compile it though.
  2. The usage of Map.Entry<String, Long> is pretty bloated, but our most reasonable option, besides creating a Pair class ourselves and using Pair<String, Long>. This will hopefully be easier if Java 9 includes tuples (which logically include pairs) as more or less first-class citizens.
  3. In the whole method getTopWords we end up storing all the entries in memory once, with the Map<String, Long>, I am pretty sure there are ways around that, but not worth the effort here, only optimize this if it becomes a real bottleneck.
  4. I was hoping to use the Stream.forEach() method when processing the results, however this is not possible with the requirement that you want to have a counter. Again, more possibilities open up in Java 9 when we hopefully have BiStreams and tuples.

I hope this review has been helpful for you.

share|improve this answer
    
Very good and detailed answer with new stuff for me to learn. Thank you. –  Sillicon Touch Apr 23 at 15:49
    
I have a question. Some of the phrases are an empty string and I want to filter out the empty ones using the '.filter()' function. Do I need to create another comparator for this purpose or can I get away with just having something like '.filter(isEmpty())' in one line? –  Sillicon Touch Apr 25 at 11:01
    
@SilliconTouch You'd want to use .filter(str -> !str.isEmpty()), the filter method actually describes which elements you wish to keep, and not which ones you want to throw out. –  skiwi Apr 25 at 11:05
    
Oh ok I get it now, thank you. –  Sillicon Touch Apr 25 at 11:16

In wordCount you don't need a TreeMap. A TreeMap orders entries by keys, but you don't need it at all. The purpose of this method is to return a map of word counts, the ordering of entries doesn't matter. It's not an error to use a TreeMap, it's just pointless. A HashMap would have been better.

In printTopWords you are using iterators without type. That's not a good practice, and the casts inside the loop are ugly. The loop would have been better like this, using the iterator pattern:

int wordsCounter = 1;
for (Entry<String, Integer> entry : wordsMap) {
    String key = entry.getKey();
    int value = entry.getValue();
    System.out.println(wordsCounter + ": " + key + " - " + value);
    if (++wordsCounter > numberOfWords) {
        break;
    }
}

Your program doesn't separate responsibilities well. You should not sort-and-print in the same method. It would be better to separate that to 2 methods, one to sort and another to print. That way unit testing will be easier too, as your test cases could be based on what the sorting method returns.

A somewhat simpler method to sort by values would have been using a Comparator with a TreeSet, for example:

static class WordCountComparator implements Comparator<String> {
    Map<String, Integer> base;
    public WordCountComparator(Map<String, Integer> base) {
        this.base = base;
    }

    public int compare(String a, String b) {
        if (base.get(a) >= base.get(b)) {
            return -1;
        }
        return 1;
    }
}

public static List<String> printTopWords(final int numberOfWords, List<String[]> phrases) {
    Map<String, Integer> wordCountMap = wordCount(phrases);
    Map<String, Integer> wordsSortedByCount = new TreeMap<String, Integer>(new WordCountComparator(wordCountMap));
    wordsSortedByCount.putAll(wordCountMap);
    // ...
}

You could iterate over entries in wordsSortedByCount, they are sorted by the word count.

Keep in mind that you did not specify the ordering of words that have the same count, so the ordering of those will be unspecified.

share|improve this answer
    
In for (Entry<String, Integer> entry : wordsMap) { you should loop over the wordsMap.entrySet() instead. –  skiwi Apr 22 at 20:17
2  
@skiwi his wordsMap is actually a List, not a Map, that's why. He has several naming issues, hence the confusion... –  janos Apr 22 at 20:20

A few notes for now:

You're not using generics for your iterator, if you use

Iterator<Entry<String, Integer>> entries = wordsMap.iterator();

and Entry<String, Integer> entry = entries.next();

you won't have to typecast anything.


You could use an AtomicInteger instead of an Integer to avoid having to use .put. By using AtomicInteger you could call .incrementAndGet() on it to increase the value.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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