c# modifying Data in Foreach?

Discussion in 'OT Technology' started by jdog12, Apr 8, 2008.

  1. jdog12

    jdog12 New Member

    Joined:
    Nov 27, 2004
    Messages:
    5,474
    Likes Received:
    0
    Im working on a project for a class. It is basically a content management system. I am dealing with managing distribution lists on the system. I am currently having an issue with Deleting distribution Lists. I am having some trouble. here is how the classes are laid out.

    DistributionList
    name : string
    description : string
    users : ICollection<IUsers>

    Email
    to : string
    from : string
    subject : string
    body : string
    distributionLists : ICollection<IDistributionList> (stores which distribution lists this email was sent to)
    date : DateTime

    we are using NHibernate for our ORM. Basically, in order to delete a distribution list, you have to clear all users from the list and remove the reference to that list from every email that was sent out. Here is a little snippet of what I'm trying to do:

    Code:
    //get all emails that have been sent
    IList<IEmail> emails = emailService.GetAllEmail(runState.Domain, "");
                                    
                
                foreach (IEmail email in emails)
                {
                                   
                    foreach (IDistributionList innerDistributionList in email.DistributionLists)
                    {
                        
                        //list is the distribution list I'm trying to delete
                        if (list == innerDistributionList)
                        {
                            email.DistributionLists.Remove(innerDistributionList);
                            
                            //save email w/ reference to taht list not there
                            emailService.SaveEmail(email, runState.Domain);
                            
                        }   
    
                    }
                }
    
                
    
                //clear users from list
                list.Users.Clear();
                
                //save list to db w/ no users
                service.SaveList(list, runState.Domain);
    
                //delete list
                service.DeleteList(list, runState);
    
    

    This works fine and dandy if there's only one email, but if there is more than one email, it all goes to hell and I get:

    Message: Collection was modified; enumeration operation may not execute.


    I'm aware that you're not supposed to use a foreach loop for modifying data, but can not think of any other way to do it. Does anyone have any suggestions?
     
    Last edited: Apr 8, 2008
  2. ge0

    ge0 New Member

    Joined:
    Oct 31, 2005
    Messages:
    8,398
    Likes Received:
    0
    Location:
    JERSEY
    regular for loop?
     
  3. SLED

    SLED build an idiot proof device and someone else will

    Joined:
    Sep 20, 2001
    Messages:
    28,118
    Likes Received:
    0
    Location:
    AZ, like a bauce!
    you can't modify a collection that you're currently looping through.

    create a temporary list of IDs ( or whatever you're using ) with the loop:
    Code:
    ArrayList toDelete = new ArrayList();
    
    // your loops
    // ...
    if (list == innerDistributionList)
    {
      toDelete.Add( list.ID ); // Or whatever property you want  
    }
    
    // after loops
    foreach( int x in toDelete )
    {
       // your delete code using the "x" variable
    }
    
     
  4. jdog12

    jdog12 New Member

    Joined:
    Nov 27, 2004
    Messages:
    5,474
    Likes Received:
    0

    I was trying to do this, but used email.PrimaryKey instead, since the list will have the same pk. The problem came in the second loop, I didnt know how to tell it to call an email based on primarykey from that list of emails and then remove the list(this I could figure out, but the first part I couldnt, I had a brain fart). I think the way you suggested was the way that the class lead told me to do it, except he said soemthing about a removeAll() method, which I didn't find.

    Im trying this out.. the web and db server this stuff is hosted on is down, so I can't test it, but we'll see.

    Code:
    IEmailService emailService = locator.GetObject<IEmailService>();
                IList<IEmail> emails = emailService.GetAllEmail(runState.Domain, "");
                IList<IEmail> updatedEmails = new List<IEmail>();
                IEmail updatedEmail;
                          
                foreach (IEmail email in emails)
                {
                    if(email.DistributionLists.Contains(list) )
                    {
                        updatedEmail = email;
                        updatedEmail.DistributionLists.Remove(list);
                        updatedEmails.Add(updatedEmail);                    
                    }
                }
    
                emailService.SaveAllEmails(updatedEmails, runState.Domain);         
                list.Users.Clear();
                service.SaveList(list, runState.Domain);
                service.DeleteList(list, runState);
     
    I don't see why it wouldnt work. It compiles fine, we'll see how it runs.
     
    Last edited: Apr 8, 2008
  5. critter783

    critter783 OT Supporter

    Joined:
    Jul 15, 2005
    Messages:
    1,785
    Likes Received:
    0
    This is going to give you the same problem as before. The way to get around it is to change the line

    updatedEmail = email;

    to

    updatedEmail = new IEmail(email);

    The way you have the line written, updatedEmail is just a reference to email. Modifying it will also modify email. Constructing it with new will give you a completely separate copy of email that you can then modify.
     
  6. whup

    whup I wish you had children and.. so that I could step

    Joined:
    Feb 12, 2007
    Messages:
    1,603
    Likes Received:
    0
    Why not use the cascade delete functions in NHibernate? That way you can delete the Distribution List, and the references to the list will be removed also, providing your relationships have been defined properly.

    http://www.hibernate.org/hib_docs/nhibernate/html/manipulatingdata.html

    I might be able to help you with some more code but I actually use ActiveRecord which is an even better wrapper over NHibernate.
     
  7. jdog12

    jdog12 New Member

    Joined:
    Nov 27, 2004
    Messages:
    5,474
    Likes Received:
    0

    :dunno: Tested my code and it works. I'm not actually modifying anything inside of the list. I create a copy of the email in the current iteration, then update that copy instead of the one in the list and save the copy. What you wrote is similar to what I was going to do at first, but for some reason I did this.
     
  8. jdog12

    jdog12 New Member

    Joined:
    Nov 27, 2004
    Messages:
    5,474
    Likes Received:
    0
    I'll have to look in to those cascade delete functions. Never even heard of ActiveRecord, but I'm a newb. what makes it better?
     
  9. whup

    whup I wish you had children and.. so that I could step

    Joined:
    Feb 12, 2007
    Messages:
    1,603
    Likes Received:
    0
    The best part is probably the no XML. You mark up your objects with attributes; defining the database fields and relationships.

    It also provides good helper methods for Find, FindAll, SlicedFindAll (the paging implementation), and then easy overrides for things like Save, Update, Insert, Delete.

    They also provide a pretty elegant solution for handling sessions and transactions.

    Because it's built on NHibernate you still have the power of NHibernate there for you; just ActiveRecord can handle all the menial stuff for you.

    It also has support for validators.

    e.g. class:

    [ActiveRecord]
    public class User : ActiveRecordValidationBase<User>
    {
    private int userId;
    private string userName;

    [PrimaryKey]
    public int UserId
    {
    get { return userId; }
    set { userId = value; }
    }

    [Property, ValidateNonEmpty, ValidateUnique]
    public string UserName
    {
    get { return userName; }
    set { userName = value; }
    }
    }

    Then from that class you can:

    IList<User> users = User.FindAll();
    User user = User.Find(2); // User with UserId of 2

    // Find user with username of whup
    User user = User.FindOne( Expression.Eq("UserName", "whup") );

    http://www.castleproject.org/activerecord/documentation/trunk/index.html

    I think you'd probably get the most advantage from ActiveRecord if you were also using MonoRail, which is another Castle project and has strong ActiveRecord integration for data binding and validation. Otherwise NHibernate is pretty damn sweet.
     
  10. critter783

    critter783 OT Supporter

    Joined:
    Jul 15, 2005
    Messages:
    1,785
    Likes Received:
    0
    My point is that you're *not* creating a copy of the email in your iteration. updatedEmail = email; doesn't copy email into updatedEmail, it just makes updatedEmail a pointer to email. Modifying updatedEmail will also modify email.

    Edit: Sure, what you have will compile and run. I'm only telling you this because at some point, you will get an ArrayAccessOutOfBounds exception if you're modifying a collection over which you're iterating.
     
  11. jdog12

    jdog12 New Member

    Joined:
    Nov 27, 2004
    Messages:
    5,474
    Likes Received:
    0
    If it's just a pointer, then it is modifying a collection that i'm iterating over, but it still works for some reason. Before if only one email referenced that list it was fine, but any more than that and it would break since it was modifying the collection that I was iterating through, but this does not break. If it was just a reference, wouldn't it throw an error once it updates one and tries to move to the next? Why is it not throwing an error the way it is now?
     
  12. critter783

    critter783 OT Supporter

    Joined:
    Jul 15, 2005
    Messages:
    1,785
    Likes Received:
    0
    No exception is raised because the foreach call can't tell that you're modifying the email. The email contains a few strings and a reference to an ICollection. None of the strings are changing, and while the contents of the ICollection are changing, this isn't actually changing the reference stored in the email. In your original post, you're doing nested foreach calls, and modifying the ICollection. This is detectable. In the last code block you posted, you're not modifying the collection over which you're iterating. By that, I mean that the length of the collection doesn't change, and none of the data members of any individual objects are changing value. However, you *are* modifying data that they reference.
    Sorry about the long post. Anyway, my point was to try and help you understand the default behavior of the = operator. It doesn't actually copy the data from one object to another, it just stores the memory address of the right-hand operand in the left-hand operand so you can make a reference to it. If you want to get a true copy of object x and store it in object y, you have to declare y as y = new ClassName(x); Bugs caused from not doing this are hard to find since they create hard-to-define boundary test cases.
     
  13. jdog12

    jdog12 New Member

    Joined:
    Nov 27, 2004
    Messages:
    5,474
    Likes Received:
    0
    Hmm.. I'll have to look in to that then. I avoided what you posted because at the beggining of the semester the class lead said when you create an object that will be stored in the database, do not use the new keyword, instead a method called CreateTransientInstance() will be used. I don't know if this is the case since technically I'm not making a new record, I'm just making a copy of the old one and modifying it. Thanks for clarifying that.
     

Share This Page