Remove repetitive, hard coded loops and conditions in C#

Question

I have a class that compares 2 instances of the same objects, and generates a list of their differences. This is done by looping through the key collections and filling a set of other collections with a list of what has changed (this may make more sense after viewing the code below). This works, and generates an object that lets me know what exactly has been added and removed between the "old" object and the "new" one.
My question/concern is this...it is really ugly, with tons of loops and conditions. Is there a better way to store/approach this, without having to rely so heavily on endless groups of hard-coded conditions?

    public void DiffSteps()
    {
        try
        {
            //Confirm that there are 2 populated objects to compare
            if (NewStep.Id != Guid.Empty && SavedStep.Id != Guid.Empty)
            {
                //<TODO> Find a good way to compare quickly if the objects are exactly the same...hash?

                //Compare the StepDoc collections:
                OldDocs = SavedStep.StepDocs;
                NewDocs = NewStep.StepDocs;
                Collection<StepDoc> docstoDelete = new Collection<StepDoc>();

                foreach (StepDoc oldDoc in OldDocs)
                {
                    bool delete = false;
                    foreach (StepDoc newDoc in NewDocs)
                    {
                        if (newDoc.DocId == oldDoc.DocId)
                        {
                            delete = true;
                        }
                    }
                    if (delete)
                        docstoDelete.Add(oldDoc);
                }

                foreach (StepDoc doc in docstoDelete)
                {
                    OldDocs.Remove(doc);
                    NewDocs.Remove(doc);
                }


                //Same loop(s) for StepUsers...omitted for brevity

                //This is a collection of users to delete; it is the collection
                //of users that has not changed.  So, this collection also needs to be checked 
                //to see if the permisssions (or any other future properties) have changed.
                foreach (StepUser user in userstoDelete)
                {
                    //Compare the two
                    StepUser oldUser = null;
                    StepUser newUser = null;

                    foreach(StepUser oldie in OldUsers)
                    {
                        if (user.UserId == oldie.UserId)
                            oldUser = oldie;
                    }

                    foreach (StepUser newie in NewUsers)
                    {
                        if (user.UserId == newie.UserId)
                            newUser = newie;
                    }

                    if(oldUser != null && newUser != null)
                    {
                        if (oldUser.Role != newUser.Role)
                            UpdatedRoles.Add(newUser.Name, newUser.Role);
                    }

                    OldUsers.Remove(user);
                    NewUsers.Remove(user);
                }

            } 
        }
        catch(Exception ex)
        {
            string errorMessage =
                String.Format("Error generating diff between Step objects {0} and {1}", NewStep.Id, SavedStep.Id);
            log.Error(errorMessage,ex);
            throw;
        }
    }

---

The targeted framework is 3.5.

Answer 1

Are you using .NET 3.5? I'm sure LINQ to Objects would make a lot of this much simpler.

Another thing to think about is that if you've got a lot of code with a common pattern, where just a few things change (e.g. "which property am I comparing?" then that's a good candidate for a generic method taking a delegate to represent that difference.

EDIT: Okay, now we know we can use LINQ:

Step 1: Reduce nesting
Firstly I'd take out one level of nesting. Instead of:

if (NewStep.Id != Guid.Empty && SavedStep.Id != Guid.Empty)
{
    // Body
}

I'd do:

if (NewStep.Id != Guid.Empty && SavedStep.Id != Guid.Empty)
{
    return;
}
// Body

Early returns like that can make code much more readable.

Step 2: Finding docs to delete

This would be much nicer if you could simply specify a key function to Enumerable.Intersect. You can specify an equality comparer, but building one of those is a pain, even with a utility library. Ah well.

var oldDocIds = OldDocs.Select(doc => doc.DocId);
var newDocIds = NewDocs.Select(doc => doc.DocId);
var deletedIds = oldDocIds.Intersect(newDocIds).ToDictionary(x => x);
var deletedDocs = oldDocIds.Where(doc => deletedIds.Contains(doc.DocId));

Step 3: Removing the docs
Either use the existing foreach loop, or change the properties. If your properties are actually of type List<T> then you could use RemoveAll.

Step 4: Updating and removing users

foreach (StepUser deleted in usersToDelete)
{
    // Should use SingleOfDefault here if there should only be one
    // matching entry in each of NewUsers/OldUsers. The
    // code below matches your existing loop.
    StepUser oldUser = OldUsers.LastOrDefault(u => u.UserId == deleted.UserId);
    StepUser newUser = NewUsers.LastOrDefault(u => u.UserId == deleted.UserId);

    // Existing code here using oldUser and newUser
}

One option to simplify things even further would be to implement an IEqualityComparer using UserId (and one for docs with DocId).

Answer 2

As you are using at least .NET 2.0 I recommend implement Equals and GetHashCode ( http://msdn.microsoft.com/en-us/library/7h9bszxx.aspx ) on StepDoc. As a hint to how it can clean up your code you could have something like this:

 Collection<StepDoc> docstoDelete = new Collection<StepDoc>();
foreach (StepDoc oldDoc in OldDocs)
                    {
                        bool delete = false;
                        foreach (StepDoc newDoc in NewDocs)
                        {
                            if (newDoc.DocId == oldDoc.DocId)
                            {
                                delete = true;
                            }
                        }
                        if (delete) docstoDelete.Add(oldDoc);
                    }
                    foreach (StepDoc doc in docstoDelete)
                    {
                        OldDocs.Remove(doc);
                        NewDocs.Remove(doc);
                    }

with this:

oldDocs.FindAll(newDocs.Contains).ForEach(delegate(StepDoc doc) {
                        oldDocs.Remove(doc);
                        newDocs.Remove(doc);
                    });

This assumes oldDocs is a List of StepDoc.