Удалить повторяющиеся, жестко закодированные циклы и условия в C #

Question

У меня есть класс, который сравнивает 2 экземпляра одного и того же объекта и генерирует список их различий. Это можно сделать, просматривая коллекции ключей и заполняя набор других коллекций списком того, что изменилось (это может иметь больше смысла после просмотра кода ниже). Это работает и генерирует объект, который позволяет мне узнать, что именно было добавлено и удалено между «старым» и «новым» объектами.
Мой вопрос / проблема заключается в следующем ... это действительно безобразно, с кучей циклов и условий. Есть ли лучший способ сохранить / приблизить это, не полагаясь так сильно на бесконечные группы жестко закодированных условий?

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

---

Целевая структура - 3,5.

Answer 1

Вы используете .NET 3.5? Я уверен, что LINQ to Objects сделает многое из этого намного проще.

Еще одна вещь, о которой стоит подумать, это то, что если у вас много кода с общим шаблоном, где меняется только несколько вещей (например, «какое свойство я сравниваю?»), То это хороший кандидат на использование универсального метода делегат, представляющий эту разницу.

РЕДАКТИРОВАТЬ: Хорошо, теперь мы знаем, что мы можем использовать LINQ:

Шаг 1. Сокращение вложенности
Во-первых, я бы выбрал один уровень вложенности. Вместо:

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

Я бы сделал:

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

Раннее возвращение, подобное этому, может сделать код намного более читабельным.

Шаг 2. Поиск документов для удаления

Было бы намного лучше, если бы вы могли просто указать ключевую функцию для Enumerable.Intersect. Вы можете указать средство сравнения на равенство, но его создание затруднительно даже при использовании служебной библиотеки. Ах, хорошо.

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

Шаг 3: Извлечение документов
Либо используйте существующий цикл foreach, либо измените свойства. Если ваши свойства на самом деле имеют тип List , тогда вы можете использовать RemoveAll.

Шаг 4. Обновление и удаление пользователей

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
}

Один вариант еще более упростить процесс - реализовать IEqualityComparer с использованием UserId (и один для документов с DocId).

Answer 2

Поскольку вы используете хотя бы .NET 2.0, я рекомендую реализовать Equals и GetHashCode (http://msdn.microsoft.com/en-us/library/7h9bszxx.aspx) в StepDoc. Как подсказка, как он может очистить ваш код, вы можете получить что-то вроде этого:

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

с этим:

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

Предполагается, что oldDocs является списком StepDoc.

Answer 3

Если и StepDocs, и StepUsers реализуют IComparable , и они хранятся в коллекциях, которые реализуют IList , то вы можете использовать следующий вспомогательный метод для упрощения этой функции. Просто позвоните дважды, один раз с StepDocs и один раз с StepUsers. Используйте beforeRemoveCallback для реализации специальной логики, используемой для обновления вашей роли. Я предполагаю, что коллекции не содержат дубликатов. Я пропустил проверку аргументов.

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

Вот пример beforeRemoveCallback для вашего кода обновлений:

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

Answer 4

Использование нескольких списков в foreach легко. Сделайте это:

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

работает аналогично foreach (TextBox t в столбце && TextBox d в дес)

Answer 5

Если вы хотите скрыть обход древовидной структуры, вы можете создать подкласс IEnumerator, который скрывает «некрасивые» циклические конструкции, а затем использовать интерфейс 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
    }
}

Однако я совсем не уверен, что это что-то особенно упрощает. Я не против увидеть вложенные структуры обхода. Код является вложенным, но не сложным или особенно трудным для понимания.

Answer 6

На какие рамки вы ориентируетесь? (Это изменит ответ.)

Почему это пустая функция?

Разве подпись не должна выглядеть так:

DiffResults results = object.CompareTo(object2);