Suppression de boucles répétitives codées en dur et de conditions dans C#

Question

J'ai une classe qui compare 2 instances des mêmes objets et génère une liste de leurs différences. Ceci est fait en faisant une boucle dans les collections de clés et en remplissant un ensemble d'autres collections avec une liste de ce qui a changé (cela peut sembler plus logique après avoir vu le code ci-dessous). Cela fonctionne, et génère un objet qui me permet de savoir exactement ce qui a été ajouté et enlevé entre "l'ancien" objet et le "nouveau".
Ma question/préoccupation est-ce ... c'est vraiment moche, avec des tonnes de boucles et de conditions. Y a-t-il une meilleure façon de stocker/d'approcher cela, sans avoir à compter si fortement sur des groupes interminables de conditions codées en dur?

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

---

Le cadre ciblé est de 3,5.

Answer 1

Utilisez-vous .NET 3.5? Je suis sûr que LINQ to Objects rendrait beaucoup de ce plus simple. Une autre chose à penser est que si vous avez beaucoup de code avec un modèle commun, où juste quelques choses changent (par exemple "quelle propriété je compare?" Alors c'est un bon candidat pour une méthode générique prendre un délégué pour représenter cette différence

EDIT: d'accord. nous savons que nous pouvons utiliser LINQ:

Étape 1:. Réduire la nidification
Tout d'abord je prendrais un à niveau d'imbrication au lieu de :

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

Je ferais:

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

retours précoces comme ça peut rendre le code beaucoup plus lisible.

Étape 2: Recherche de documents à supprimer

Ce serait beaucoup plus agréable si vous pouvez simplement spécifier une fonction clé de Enumerable.Intersect. Vous pouvez spécifier un comparateur d'égalité, mais construire un de ceux-là est une douleur, même avec une bibliothèque utilitaire. Et bien.

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

Étape 3: Retrait de la documentation
soit utiliser la boucle foreach existante, ou modifier les propriétés. Si vos propriétés sont réellement de type Liste < T> alors vous pouvez utiliser RemoveAll.

Étape 4: Mise à jour et supprimer des utilisateurs

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 
} 

Une option pour simplifier les choses encore plus loin serait de mettre en œuvre un IEqualityComparer en utilisant UserId (et un pour docs avec DocId).

Answer 2

Quel cadre ciblez-vous? (Cela fera une différence dans la réponse.)

Pourquoi est-ce une fonction vide?

Si pas la signature ressembler à:

DiffResults results = object.CompareTo(object2); 

Answer 3

Comme vous utilisez au moins 2.0 .NET Je recommande equals et GetHashCode (http://msdn.microsoft.com/en-us/library/7h9bszxx.aspx) sur StepDoc. Comme un indice sur la façon dont il peut nettoyer votre code pourrait avoir quelque chose comme ceci:

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

avec ceci:

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

Cela suppose Olddocs est une liste de StepDoc.

Answer 4

Si vous voulez cacher la traversal de la structure arborescente, vous pouvez créer une sous-classe IEnumerator qui cache les constructions en boucle « laid » et utiliser l'interface 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 
    } 
} 

Cependant, je ne suis pas à coup sûr que cela simplifie tout particulièrement. Cela ne me dérange pas de voir les structures de traversal imbriquées. Le code est imbriqué, mais pas complexe ou particulièrement difficile à comprendre.

Answer 5

Si les deux StepDocs et StepUsers mettre en œuvre IComparable <T>, et ils sont stockés dans des collections qui mettent en œuvre IList <T>, vous pouvez utiliser la méthode d'assistance suivante pour simplifier cette fonction. Il suffit de l'appeler deux fois, une fois avec StepDocs, et une fois avec StepUsers. Utilisez beforeRemoveCallback pour implémenter la logique spéciale utilisée pour effectuer les mises à jour de rôle. Je suppose que les collections ne contiennent pas de doublons. J'ai laissé de côté les vérifications d'argument.

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

Voici un exemple beforeRemoveCallback votre code mises à jour:

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

Answer 6

En utilisant plusieurs listes dans le foreach est facile. Pour ce faire:

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

Il fonctionne de la même comme il est foreach (TextBox t col & & TextBox d dans des)