Remove User from list of users in java

Question

I'm new to java, so please excuse me if my explanation is poor but I have a list :

private LinkedList<User> users= new LinkedList<User>();

and a method removeUser:

private void removeUser() {
        System.out.println("All Users: ");
        viewUsers();
        System.out.print("Please enter the ID of the user to be removed: ");
        int choice = In.nextInt();
        for (User user : users){ 
            if (users.contains(choice)) { 
                users.remove(choice); 
            } 

        }
    }

Now when I run the code it successfully prints out everything. Yet when I check the list contents, nothing has changed and I'm unsure of what I have done wrong with the removal part of the code.

@Aniox Even if I print it after it still shows the user in the account — FirstPersonHuman, Sep 17 '20 at 02:32
what are the member variables for the `user` object (that is the type of your linked list)? I assume that object has a `user id` field that is an integer. If thats the case then you probably should update your inner if statement to say `if (users.getID() == choice)` then proceed with the rest of your code. — CAMD_3441, Sep 17 '20 at 02:34
Few problems: (1) `if (users.contains(choice))` is checking if list that contains objects of type User also contains some `int` which will never be evaluated to true. You probably wanted something more like `if(user.getID == choice)`. (2) `users.remove(choice);` will try to remove element at *position* `choice` in list which you are currently iterating over, not with ID==choice. (3) Since it will also modify list you are currently iterating over you may face `ConcurrentModificationException`. What you are after is probably something like `users.removeIf(user->user.getID()==choice)`. — Pshemo, Sep 17 '20 at 02:35
**LIfe lesson**: you wrote `for (User user : users)`, but you didn't use `user` anywhere inside the loop. Normally when you write a `for-each` loop it's because you want to _do something_ with each item of the collection you're iterating; that's you didn't do anything with `user` is a warning sign that something isn't quite right. — Kevin Anderson, Sep 17 '20 at 02:44
It’s not a full answer to your question, but I think it’s very helpful: [Iterating through a Collection, avoiding ConcurrentModificationException when removing objects in a loop](https://stackoverflow.com/questions/223918/iterating-through-a-collection-avoiding-concurrentmodificationexception-when-re). — Ole V.V., Sep 17 '20 at 04:00

score 1 · Answer 1 · edited Sep 17 '20 at 14:28

1

Alternative solution: if you want to use lambda and streams to remove first User with corresponding ID.


private void removeUser() {
    System.out.println("All Users: ");
    viewUsers();
    System.out.print("Please enter the ID of the user to be removed: ");
    int choice = In.nextInt();
    
    users.stream()
         .filter(user->user.getId() == choice)
         .findFirst()
         .ifPresent(users::remove);
}

edited Sep 17 '20 at 14:28

Pshemo

122,468
25
185
269

answered Sep 17 '20 at 03:57

Raj

131
6

Thanks. I’d prefer the `users.removeIf(user -> user.getID() == choice)` from the comment by Pshemo, though. Feel free to include it as an option in your answer if you like. – Ole V.V. Sep 17 '20 at 04:28
It is tested .. it does not cause `ConcurrentModificationException` – Raj Sep 17 '20 at 13:45
1

OK, I made a mistake by not noticing that when you call `.findFirst()` it is stream's *terminal operation* which ends stream processing and returns `Optional`. This means that calling `.ifPresent(users::remove)` will not throw `ConcurrentModificationException` (as stream processing ended). +1 but still IMO `users.removeIf(user->user.getId() == choice)` is more intuitive here. – Pshemo Sep 17 '20 at 14:33
Agreed... its concise and cleaner.. however `removeIf` will remove all element with matching predicate. deletion will go for O(n). however `findFirst` will exit once we find first element. so its faster on average. – Raj Sep 17 '20 at 14:41
1

Had to edit something if I wanted to change my incorrect vote :) Anyway `users::remove` will invoke List#remove which usually also need to iterate over entire list in case element exist more than once which makes it `O(n)`. – Pshemo Sep 17 '20 at 14:45
Oh ya... still the list inside... thanks for clarification.. – Raj Sep 17 '20 at 14:46

score -1 · Answer 2 · answered Sep 17 '20 at 03:45

If you want remove some item from list you can't use for-each loop. Refer: What is a difference between traditional loop and for-each loop?

I think you need same:

 private void removeUser() {
    System.out.println("All Users: ");
    viewUsers();
    System.out.print("Please enter the ID of the user to be removed: ");
    int choice = In.nextInt();
    for (int i = 0; i < users.size(); i++){ 
        User user = users.get(i);
        if (user.contains(choice)) { 
            users.remove(i); // remove at index i
        } 

    }
}

Hope can help you!

Remove User from list of users in java

2 Answers2