Junit Testing issue with for loop and return statement

Question

I'm kind of confused with a for-loop & return statements. I hope someone could explain me what's going on.

    private Map<Ticket, Integer> list;

public RessourceBasedAccessControl32603() {
    super();
    list = new HashMap<>();
}

I have to write a working method out of this junit test:

@Test
void testRemoveCollection2() {
    accessControl.add(t1, 10);
    accessControl.add(t2, 20);
    accessControl.add(t3, 30);
    Set<Ticket> s = new HashSet<>();
    s.add(t1);
    s.add(t3);
    int actual = accessControl.remove(s);
    assertEquals(40, actual);
}

If I write this code I get the expected 40 as a result.

    @Override
public int remove(Collection<Ticket> c) { 
    int points = 0;
    for (Ticket i : c) {
        if (list.containsKey(i)) {
            points += list.remove(i);
        }
    }
    return points;
}

If I write this code I get 30 instead of 40. I'm just getting the number of the last add (t3) instead of t1 + t3). I'm pretty sure it's not summing up because of the first return but if I delete the first return I get a "java.util.ConcurrentModificationException". Why can't I just use one return as in the example above? What's the difference? Is there still a way to get the method to sum up all tickets t in the HashSet s?

    @Override
public int remove(Collection<Ticket> c) {
    int points = 0;
    for (Ticket i : list.keySet()) {
        if (c.contains(i)) {
            points += list.remove(i);
            return points;
        }
    }
    return points;
}

Thanks a lot in advance!

Unrelated: make sure your NAMES are meaningful. To call a Map a "list" is super confusing. Why not `pointsByTicket` or something like that. — GhostCat, Apr 20 '22 at 13:44
Note that this has NOTHING to do with JUnit. It is great that you add the test code you are using, but there is no point in tagging with JUnit, because the kind of testing framework you are using doesn't contribute at all to the problem in your production code. — GhostCat, Apr 20 '22 at 13:46

score 0 · Accepted Answer · answered Apr 20 '22 at 13:32

The ConcurrentModificationException happens, because you remove an item from the HashMap, while still iterating over it. If you leave the return in, then you don't iterate the key set while modifying it.

If you want to go that route, remove the items after you are done iterating.

public int remove(Collection<Ticket> c) {
    int points = 0;
    for (Ticket i : list.keySet()) {
        if (c.contains(i)) {
            points += list.get(i);
            // return points;
        }
    }
    list.keySet().removeAll(c);
    return points;
}

score 0 · Answer 2 · answered Apr 20 '22 at 13:37

In Java you cannot operate on collection using this statement format inside for loop. for (Ticket i : list.keySet()) - internally it's using Iterator from the collection and modifying it during the Iteration breaks the order, thus the exception.

There are two approaches you can use:

First one would be just adding points first and then doing list.removeAll(Collection<?>...).

Second one can be done using older for statement with indexes after transforming the keySet into an actual list like new ArrayList<>(list.keySet())

Loop below uses reversed order to not affect indexing by removing entries within the loop. (Removing index j affects only elements that are at positions greater than j).

for (int i=keyList.length-1; i>=0;i--){
   Ticket t = keyList[i];
   if(c.contains(t)){
      points+=list.remove(t)
   }
}

Junit Testing issue with for loop and return statement

2 Answers2