tags:

views:

82

answers:

4

Hi everyone. My goal is to delete a user from the user list in my application.But i cannot get to the bottom of this error. Some one plz bail me out.

if (txtEmailID.Text.Length > 0)
{
    users = UserRespository.GetUserName(txtEmailID.Text);
    bool isUserAvailable=false;
    foreach (EduvisionUser aUser in users) // Exception thrown in this line
    {
        isUserAvailable = true;
        if(!aUser.Activated)
        {
            users.Remove(aUser);
        }
    }
    if (users.Count == 0 && isUserAvailable)
    {
        DeactivatedUserMessage();
        return;
    }
}
+5  A: 

You are trying to delete a user from the list you are looping trough.

this is impossible. Best is to create a new list and add the good ones in it instead of deleting the bad ones

if (txtEmailID.Text.Length > 0)
    {
        //@new list
        List<EduvisionUser> listOfAcceptedUsers = new List<EduvisionUser>()**

        users = UserRespository.GetUserName(txtEmailID.Text);
        bool isUserAvailable=false;
        foreach (EduvisionUser aUser in users) --->***Exception thrown in this line***
        {
            isUserAvailable = true;

            //Add user to list instead of deleting
            if(aUser.Activated)
            {
                ListOfAcceptedUsers.Add(aUser);
            }
        }

        //check new list instead of old one
        if (ListOfAcceptedUsers.Count == 0 && isUserAvailable)
        {
            DeactivatedUserMessage();
            return;
        }

    }
Nealv
@Nealv: Thanks 4 the reply. Can u suggest a modification in the existing code..
jithin
:s I added modification, is there more modification needed, or did you comment before end of my edit
Nealv
Thanks a lot Nealv...
jithin
+9  A: 

You can't modify a collection while you're iterating over it with a foreach loop. Typical options:

  • Use a for loop instead
  • Create a separate collection of the items you want to act on, then iterate over that.

Example of the second approach:

List<EduvisionUser> usersToRemove = new List<EduvisionUser>();
foreach (EduvisionUser aUser in users) --->***Exception thrown in this line***
{
    isUserAvailable = true;
    if(!aUser.Activated)
    {
        usersToRemove.Add(aUser);
    }
}
foreach (EduvisionUser userToRemove in usersToRemove)
{
    users.Remove(userToRemove);
}

Another alternative, if you're using List<T> is to use List<T>.RemoveAll:

isUserAvailable = users.Count > 0;
users.RemoveAll(user => !user.Activated);
Jon Skeet
As much as I hate to upvote someone with 200,000 rep, +1. I don't use RemoveAll as much as I could.
Dave Markle
@Dave: If it's any consolation, it's not like the upvote has increased my rep :)
Jon Skeet
Thanks a lot Jon Skeet
jithin
A: 

hi, you can do it like this. Use for instead foreach

for( int i =0; i< users.Count; i++ ) --->***Exception thrown in this line***
 {
  EduvisionUser aUser = users[i];
  isUserAvailable = true;
  if(!aUser.Activated)
  {
    users.Remove(aUser);
    i--;
  }
 }
IordanTanev
A: 

You cannot modify the collection while enumerating. Instead of removing select only what you need and leave the Garbage Collector take care of the rest:

users = users.Where(x => x.Activated);

Or even better, select only what you need from the repository:

users = UserRespository.GetUserName(txtEmailID.Text).Where(x => x.Activated);
Darin Dimitrov
Most of the time, this type can be used, very readable, less code. I use this all the time unless I have to do functions in my lambda expression.
Nealv
Thanks a lot Darin Dimitrov...
jithin