usa - Elimine bucles y condiciones repetitivos y codificados en C#

instrucciones 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 objetivo es 3.5.

Answer 1

¿A qué marco estás apuntando? (Esto marcará la diferencia en la respuesta).

¿Por qué es esto una función vacía?

¿No debería ser la firma como:

DiffResults results = object.CompareTo(object2);

Answer 2

Como está utilizando al menos .NET 2.0, recomiendo implementar Equals y GetHashCode ( http://msdn.microsoft.com/en-us/library/7h9bszxx.aspx ) en StepDoc. Como una pista sobre cómo puede limpiar tu código, podrías 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 este:

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

Esto supone que oldDocs es una lista de StepDoc.

Answer 3

Si ambos StepDocs y StepUsers implementan IComparable <T>, y se almacenan en colecciones que implementan IList <T>, entonces puede usar 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í hay un ejemplo de BeforeRemoveCallback para su código de actualizaciones:

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

Answer 4

Si desea ocultar el recorrido de la estructura similar a un árbol, puede crear una subclase IEnumerator que oculte las construcciones de bucle "feo" y luego use la 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 nada seguro de que esto simplifique algo 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

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

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 que toma un delegado para representar esa diferencia

EDITAR: Bien, ahora sabemos que podemos usar LINQ:

Paso 1: reducir la anidación
Primero eliminaría un nivel de anidación. 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

Los primeros retornos como ese pueden hacer que el código sea mucho más legible.

Paso 2: Encontrar documentos para borrar

Esto sería mucho mejor si pudieras 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: eliminar los documentos
Utilice el bucle foreach existente o cambie las propiedades. Si sus propiedades son en realidad de tipo List <T>, podría usar RemoveAll.

Paso 4: actualizar y eliminar 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 opción para simplificar aún más las cosas sería implementar un IEqualityComparer usando UserId (y uno para documentos con DocId).

Answer 6

Usar múltiples listas en foreach es fácil. Hacer esto:

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 como foreach (TextBox t en col && TextBox d in des)