Removing every Nth element from Wildcard List - Java

Question

I am trying to create a method that removes every Nth element from a List of unknown type (Wildcard), however every way I try doing it, it doesn't remove the specified elements, but I cannot figure out why. I have been struggling with this for two days now, so I am posting here as a last resort. I thank you in advance for any help.

The code I currently have is as follows:

public static void removeEveryNthElement(List<?> list, int n) {

    //Set list equal to an ArrayList because List is immutable
    list = new ArrayList<>(list);

    //If n is negative or zero throw an exception
    if(n <= 0) {
        throw new IllegalArgumentException("Integer n needs to be a positive number.");
    }

    //If the list is null, throw an exception
    if(list == null) {
        throw new NullPointerException("The list must not be null.");
    }

    //Remove every nth element in the list
    for(int i = 0; i < list.size(); i++) {
        if(i % n == 0) {
            list.remove(i);
        }
    }

The other way I have attempted is replacing the for loop with the following:

list.removeIf(i -> i % 3 == 0);

However, when I do it like this I receive the error that the % operator is undefined for the argument type. I have also tried using a for loop to individually add each element from the list into another modifiable list, but no matter what I do, I am having no luck. If you could help me with this it would be greatly appreciated!

Answer 1

The most serious problem with your code is that removing an element at index i changes the index of all following elements and therefore your condition for removing elements (i % n) is wrong after removing the first element.

One way to get around the problem is iterating in reverse order:

for (int i = list.size()-1; i >= 0; i--) {
    if (i % n == 0) {
        list.remove(i);
    }
}

Another way is to increment i not by one, but by n and adjust it for the removed element:

for (int i = 0; i < list.size(); i += n) {
    list.remove(i);
    i--;
}

and, since i--; followed by i += n; is the same as i += n-1;:

for (int i = 0; i < list.size(); i += n-1) {
    list.remove(i);
}

---

An additional note: the check if (list == null) is useless after the statement list = new ArrayList<>(list);, since new ArrayList<>(list); already throws a NullPointerException if list is null

Answer 2

You need to keep in mind that creating new collection based on other collection is removing references to oryginal collection - values from collection are coppied to new one - any modification will not impact anything that is out of method scope. You'll need to pass the collection that supports deleting an object from itself or return new collection from method. Remember that the type does not define behaviour of object - its depending on the implementation that is compatible with class you cast to. Here is an example of what I said about the backend implementation (both variables are type List but the implementation is different).

Here is code when you want to do this 'in place':

public static void main(String[] args) {
    List<Integer> list2 = new ArrayList<>();
    list2.add(1);
    list2.add(2);
    list2.add(3);
    list2.add(4);

    removeEveryNthElement(list2, 3); // deleted number 3 because it is 3rd element
}

public static void removeEveryNthElement(List<?> list, int n) {
    for (int i = 2; i < list.size(); i += 3) {
        list.remove(i);
    }
}

But I would like to recommend not to do any operation that is not transparent to programmer. It's better to read and understand bigger programms when you know that you pass value to method and 'it does something' then get value back because it got changed. For this example I use generics and streams:

public static void main(String[] args) {
    List<Integer> list1 = Arrays.asList(1, 2, 3, 4);
    list1 = removeEveryNthElement2(list1, 3); //deleted number 3
    System.out.println();
}

public static <T> List<T> removeEveryNthElement2(List<T> list, int n) {
    final Predicate<T> p = new Predicate<T>() {
        int i = 0;

        @Override
        public boolean test(final T t) {
            return ++i % n != 0;
        }
    };

    return list.stream().filter(p).collect(Collectors.toList());
}

Answer 3

Brandon, let me first suggest that your method is side-effecting the list built in the calling method. This is allowed, but can lead to hard-to-understand bugs in more complicated code. Instead, try making a new list in your method, and assigned the returned value:

public class Remove {

    public static void main(String[] args) {
        List<String> list = Arrays.asList("a", "b", "c", "d", "e", "f", "g", "h");
        list = removeElements(list, 3);
        System.out.println(list);
    }

    public static <T> List<T> removeElements(List<T> list, int n) {
        List<T> newList = new ArrayList<T>();
        for (int i = 0; i < list.size(); i++) {
            if (i % n != 0) {
                newList.add(list.get(i));
            }
        }
        return newList;
    }
}

As a result, it makes the method simpler, because we're no longer iterating over the list being modified.

Have a look at Side effect--what's this? to read more about side-effecting.