Eliminar bucles y condiciones repetitivos y codificados en C#

Question

Tengo una clase que compara 2 instancias de los mismos objetos y genera una lista de sus diferencias. Esto se hace recorriendo las colecciones de claves y completando un conjunto de otras colecciones con una lista de lo que ha cambiado (esto puede tener más sentido después de ver el código a continuación). Esto funciona y genera un objeto que me permite saber qué se ha agregado y eliminado exactamente entre el objeto "antiguo" y el "nuevo".
Mi pregunta/preocupación es esta ... es realmente fea, con toneladas de bucles y condiciones. ¿Hay una mejor manera de almacenar/abordar esto, sin tener que depender tanto de un sinfín de grupos de condiciones codificadas?

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

---

El marco dirigida es 3,5.

Answer 1

¿Está utilizando .NET 3.5? Estoy seguro de que LINQ to Objects haría mucho de esto mucho más simple.

Otra cosa en que pensar es que si tienes un montón de código con un patrón común, donde algunas cosas cambian (por ejemplo, "¿qué propiedad estoy comparando?" Entonces ese es un buen candidato para un método genérico teniendo un delegado para representar esa diferencia

EDIT:. bien, ahora sabemos que podemos utilizar LINQ:

. Paso 1: Reducir anidación
en primer lugar me gustaría llevar a cabo un nivel de anidamiento en lugar de :

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

lo haría:

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

primeros resultados similares, que pueden hacer que el código mucho más legible.

Paso 2: Encontrar documentos para eliminar

esto sería mucho más agradable si usted podría simplemente especificar una función clave para Enumerable.Intersect. Puede especificar un comparador de igualdad, pero construir uno de ellos es una molestia, incluso con una biblioteca de utilidad. Ah bueno.

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

Paso 3: Eliminación de los docs
utilizar el bucle foreach existente, o cambiar las propiedades. Si sus propiedades son realmente del tipo List < T>, entonces podría usar RemoveAll.

Paso 4: Actualización y eliminación de usuarios

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 
} 

Una de las opciones para simplificar aún más las cosas serían para implementar un IEqualityComparer usando el usuario (y uno para documentos con DocId).

Answer 2

¿A qué marco se dirigen? (Esto hará una diferencia en la respuesta.)

¿Por qué es esta función anulada?

¿No debería la firma aspecto:

DiffResults results = object.CompareTo(object2); 

Answer 3

Como está utilizando al menos .NET 2.0 recomiendo aplicar iguales y GetHashCode (http://msdn.microsoft.com/en-us/library/7h9bszxx.aspx) en StepDoc. Como una sugerencia para cómo se puede limpiar el código que podría tener algo como esto:

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

con esto:

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

Esto supone oldDocs es una lista de StepDoc.

Answer 4

Si desea ocultar el recorrido de la estructura en forma de árbol puede crear una subclase IEnumerator que oculta las "feas" construcciones de bucle y luego usar interfaz CompareTo:

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

Sin embargo, no estoy seguro de que esto simplifica todo en particular. No me importa ver las estructuras atravesadas anidadas. El código está anidado, pero no es complejo o particularmente difícil de entender.

Answer 5

Si ambos StepDocs y StepUsers implementan IComparable <T>, y se almacenan en colecciones que implementan IList <T>, a continuación, puede utilizar el siguiente método de ayuda para simplificar esta función. Simplemente llámelo dos veces, una vez con StepDocs y una vez con StepUsers. Use beforeRemoveCallback para implementar la lógica especial utilizada para realizar sus actualizaciones de roles. Supongo que las colecciones no contienen duplicados. He omitido las verificaciones de argumentos.

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

Aquí está una muestra para su beforeRemoveCallback código de actualizaciones:

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

Answer 6

El uso de múltiples listas de foreach es fácil. Haga lo siguiente:

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

Funciona de manera similar, ya que es foreach (t cuadro de texto en la col & & cuadro de texto d en el des)