Iterate over the "entrySet" instead of "keySet"

Question

I'm scanning my code through SonarQube and it shows this code smell "Iterate over the "entrySet" instead of "keySet"". I tried, but I can't figure it out.

Sample code:

public Set<Date> getAccountShiftDate(Map<String, Set<String>> shiftDatesMap, List<Groups> shiftSchedule) {

    // set that has the account shift dates 
    Set<Date> accountShiftDatesTemplate = new Hashset<>();

    // iterate on accounts
    for (String accounts : shiftDatesMap.keySet()) {
        //get group of an account
        Optional <Groups> shiftOptional = shiftList
                                             .stream()
                                             .filter(g -> StringUtils.equalsIgnoreCase(accounts,g.getLongName()))
                                             .findFirst();
        // ...

Can someone give me a reference to understand this.

What is the rest of your loop? I think you are hiding the crucial parts – do you somewhere call `shiftDatesMap.get(accounts)`? — knittl, Jan 23 '23 at 14:18
shiftList hast no been declared in this part of the code - I feel like that might also be important — berse2212, Jan 23 '23 at 14:20
On which map/line of code does it notify you? Could you indicate this with a comment in your code please. Also do you understand the described code smell in general already? Sonarqube should be able to show you a description with good and bad example. — Simulant, Jan 23 '23 at 14:24
Is there something in SonarQube's description of the rule, https://rules.sonarsource.com/java/RSPEC-2864, that you don't understand? — k314159, Jan 23 '23 at 14:26
This question is incomplete. It does not include a [mre]. Please [edit] it to include the complete code required to reproduce the "problem". (The SonarQube rule should be pretty clear about it and even contain one compliant example and one non-compliant example) — knittl, Jan 23 '23 at 14:49
https://stackoverflow.com/questions/27785958/iterating-over-key-set-vs-iterating-over-entry-set — Sören, Jan 23 '23 at 15:48

Simulant · Accepted Answer · 2023-01-23T14:34:43.307

If you iterate over shiftDatesMap.keySet() and later on call in your loop shiftDatesMap.get(accounts) you execute an unnecessary get operation for each entry.
Try to read an understand the description of the metioned code smell from SonarQube.

Instead you should use shiftDatesMap.entrySet() which gives you an Entry, i.e. as pair of the key and the value. So if you later on in your loop want to access the value for your given accounts key, you must only access the value from your entry, which is cheaper than calling a get operation on the map for each value.

Before:

for (String accounts : shiftDatesMap.keySet()) {
    Set<String> shiftDates = shiftDatesMap.get(accounts); // unnecessary operation
}

After:

for (Map.Entry<String, Set<String>> entry: shiftDatesMap.entrySet()) {
    String accounts = entry.getKey();
    Set<String> shiftDates = entry.getValue(); // access value for free without 'get' on map

}

Iterate over the "entrySet" instead of "keySet"

1 Answers1