Get top 3 counts from list using stream

Question

I am trying to convert java7 program into java 8. I want below output using stream API.

public List<String> getTopThreeWeatherCondition7() {
    List<String> _Top3WeatherList = new ArrayList<String>();
    Map<String, Integer> _WeatherCondMap = getWeatherCondition7();
    List<Integer> _WeatherCondList = new ArrayList<Integer>(_WeatherCondMap.values());
    Collections.sort(_WeatherCondList, Collections.reverseOrder());
    List<Integer> _TopThreeWeathersList = _WeatherCondList.subList(0, 3);
    Set<String> _WeatherCondSet = _WeatherCondMap.keySet();
    Integer count = 0;
    for (String _WeatherCond : _WeatherCondSet) {
        count = _WeatherCondMap.get(_WeatherCond);
        for (Integer _TopThreeWeather : _TopThreeWeathersList) {
            if (_TopThreeWeather == count) {
                _Top3WeatherList.add(_WeatherCond);
            }
        }
    }
    _WeatherCondList = null;
    _WeatherCondMap = null;
    _TopThreeWeathersList = null;
    _WeatherCondSet = null;
    return _Top3WeatherList;

}

Please set more tags to speak to more users due to tagging. For example add: java, java7, java8. — manuzi1, Nov 22 '16 at 09:23
Just from the top of my head based on what it seems you are trying to do: getWeatherCondition7().valueSet().stream().sorted(Collections.reverseOrder()).limit(3).collect(Collections.toList()); this should get you started in the right direction. — AlexC, Nov 22 '16 at 13:55
I wouldn’t call this a “Java 7 program”. First, use the standard naming conventions, i.e. start variable names with a lower case letter instead of `_`+upper case letter. Second, don’t assign variables to `null` after use. Third, don’t initialize variables with an unused default (like the `count = 0`). After fixing these issues, we could call this a “Java program”. Start using the “diamond operator” `<>` instead of repeating the type parameters, and we can call it “Java 7 program”. — Holger, Nov 22 '16 at 14:19

score 6 · Answer 1 · answered Nov 22 '16 at 15:09

I strongly suggests to adhere to Java coding conventions. Start variable names with a lower case letter instead of _+upper case letter. Second, don’t assign local variables to null after use. That’s obsolete and distracts from the actual purpose of the code. Also, don’t initialize variables with an unused default (like the count = 0). In this specific case, you should also declare the variable within the inner loop, where it is actually used.

Note also that you are comparing Integer references rather than values. In this specific case it might work as the objects originate from the same map, but you should avoid that. It’s not clear whether there might be duplicate values; in that case, this loop will not do the right thing. Also, you should not iterate over the keySet(), just to perform a get lookup for every key, as there is entrySet() allowing to iterate over key and value together.

Since you said, this code ought to be a “Java 7 program” you should mind the existence of the “diamond operator” (<>) which removes the need to repeat type arguments when creating new instances of generic classes.

Instead of sorting the values only and searching for the associated keys, you should sort the entries in the first place.

So a clean Java 7 variant of your original code would be:

static final Comparator<Map.Entry<String, Integer>> BY_VALUE_REVERSED=
    new Comparator<Map.Entry<String, Integer>>() {
        public int compare(Map.Entry<String, Integer> o1, Map.Entry<String, Integer> o2) {
            return Integer.compare(o2.getValue(), o1.getValue());
        }
    };
public List<String> getTopThreeWeatherCondition7() {
    List<String> top3WeatherList = new ArrayList<>();
    Map<String, Integer> weatherCondMap = getWeatherCondition7();
    List<Map.Entry<String, Integer>> entryList=new ArrayList<>(weatherCondMap.entrySet());
    Collections.sort(entryList, BY_VALUE_REVERSED);
    List<Map.Entry<String, Integer>> topThreeEntries = entryList.subList(0, 3);
    for(Map.Entry<String, Integer> entry: topThreeEntries) {
        top3WeatherList.add(entry.getKey());
    }
    return top3WeatherList;
}

This also handles duplicates correctly. Only if there is a tie on the third place, just one of the valid candidates will be chosen.

Only if you have a clean starting point, you may look, how this can benefit from Java 8 features

Instead of copying the content to a List to sort it, you can create a Stream right from the Map and tell the stream to sort
You can create a comparator much easier, or even use one of the new builtin comparators
You can chain the task of limiting the result to three elements, map to the key and collect to the result List right to the stream of the previous steps:

public List<String> getTopThreeWeatherCondition7() {
    Map<String, Integer> weatherCondMap = getWeatherCondition7();
    List<String> top3WeatherList = 
        weatherCondMap.entrySet().stream()
            .sorted(Collections.reverseOrder(Map.Entry.comparingByValue()))
            .limit(3)
            .map(Map.Entry::getKey)
            .collect(Collectors.toList());
    return top3WeatherList;
}

Do you know if this Stream is as faster as a for getting the 3 best condition? because a sort is O(log n) and a 3 for is a O(n)... or the limit "changes" the sort algorithm? — Thiago Suchorski, Mar 04 '21 at 14:17
@ThiagoSuchorski in principle, it would be legal if a Stream implementation uses the knowledge about such subsequent operations (enabling such optimization is the purpose of the API), but in case of the reference implementation (all OpenJDK based environments), such optimization does not happen yet. If performance is more important than simplicity for your task, something in [this direction](https://stackoverflow.com/a/30774023/2711488) would be better. — Holger, Mar 04 '21 at 14:41

Get top 3 counts from list using stream

1 Answers1