Feuermagier / autograder

Automatic grading of student's Java code
MIT License
13 stars 7 forks source link

Can not convert to for-each loop when collection is modified while looping #617

Open Luro02 opened 1 week ago

Luro02 commented 1 week ago

Summary

For example, the following code will throw an exception:

import java.util.ArrayList;
import java.util.List;

public class Main {
    public static void main(String[] args) {
        var list = new ArrayList<>(List.of("a", "toRemove", "b", "toRemove"));

        for (var e : list) {
            if (e.equals("toRemove")) {
                list.remove(e);
            }
        }

        System.out.println(list);
    }
}

which would be suggested with

import java.util.ArrayList;
import java.util.List;

public class Main {
    public static void main(String[] args) {
        var list = new ArrayList<>(List.of("a", "toRemove", "b", "toRemove"));

        for (int i = 0; i < list.size(); i++) {
            var e = list.get(i);
            if (e.equals("toRemove")) {
                list.remove(e);
            }
        }

        System.out.println(list);
    }
}

Lint Name

FOR_CAN_BE_FOREACH

Reproducer

<code>
lunagl commented 1 week ago

The original code is most likely a bug (elements will be skipped), autograder should suggest using Iterator.remove instead

Luro02 commented 2 days ago

Technically, the "original code" is not really the original and the suggested code isn't really suggested.

Currently it only suggests something like, "hey your for loop, should be a for-each loop" and doesn't give any detailed suggestions.

Iterator.remove is in my opinion not something that a first year student can figure out themselves, and to be honest, I don't know how that would be done with a for-each loop either?

var iterator = list.iterator();
// NOTE: this crashes when the iterator is empty
for (var value = iterator.next(); iterator.hasNext(); value = iterator.next()) {
    // call iterator.remove here when necessary
}

For now I will make an exception for any code that modifies the collection while looping. This is the way it seems to be in IntelliJ as well.

In the future one could make the above (if worth the time, I don't think collections are modified frequently in loops) into a separate check.