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;
        }
    }

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

What framework are you targeting? (This will make a difference in the answer.)

Why is this a void function?

Shouldn't the signature look like:

DiffResults results = object.CompareTo(object2);

Answer 3

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.

Answer 4

If you want to hide the traversal of the tree-like structure you could create an IEnumerator subclass that hides the "ugly" looping constructs and then use CompareTo interface:

MyTraverser t =new Traverser(oldDocs, newDocs);

foreach (object oldOne in t)
{
    if (oldOne.CompareTo(t.CurrentNewOne) != 0)
    {
        // use RTTI to figure out what to do with the object
    }
}

However, I'm not at all sure that this particularly simplifies anything. I don't mind seeing the nested traversal structures. The code is nested, but not complex or particularly difficult to understand.

Answer 5

If both StepDocs and StepUsers implement IComparable<T>, and they are stored in collections that implement IList<T>, then you can use the following helper method to simplify this function. Just call it twice, once with StepDocs, and once with StepUsers. Use the beforeRemoveCallback to implement the special logic used to do your role updates. I'm assuming the collections don't contain duplicates. I've left out argument checks.

public delegate void BeforeRemoveMatchCallback<T>(T item1, T item2);

public static void RemoveMatches<T>(
                IList<T> list1, IList<T> list2, 
                BeforeRemoveMatchCallback<T> beforeRemoveCallback) 
  where T : IComparable<T>
{
  // looping backwards lets us safely modify the collection "in flight" 
  // without requiring a temporary collection (as required by a foreach
  // solution)
  for(int i = list1.Count - 1; i >= 0; i--)
  {
    for(int j = list2.Count - 1; j >= 0; j--)
    {
      if(list1[i].CompareTo(list2[j]) == 0)
      {
         // do any cleanup stuff in this function, like your role assignments
         if(beforeRemoveCallback != null)
           beforeRemoveCallback(list[i], list[j]);

         list1.RemoveAt(i);
         list2.RemoveAt(j);
         break;
      }
    }
  }
}

Here is a sample beforeRemoveCallback for your updates code:

BeforeRemoveMatchCallback<StepUsers> callback = 
delegate(StepUsers oldUser, StepUsers newUser)
{
  if(oldUser.Role != newUser.Role)
    UpdatedRoles.Add(newUser.Name, newUser.Role);
};

Answer 6

The targeted framework is 3.5.

Answer 7

Using Multiple lists in foreach its damn easy

Do this: foreach (TextBox t in col) { foreach (TextBox d in des) //here des and col are list having textboxes { ///here remove first element then and break it RemoveAt(0); break; } }

It works similar as it is foreach (TextBox t in col && TextBox d in des)