Объединить элементы, которые имеют те же дети


Код C# ниже, используя LINQ к объектам, призван объединить всех классов сущностей, которые имеют тот же SubjectId и те же студенты.

var sb = new StringBuilder();
var counter = 0;

using (var ctx = new Ctx()) 
{
  var allClasses = ctx.Classes.Where(o => o.SubjectId > 0).OrderBy(o => o.Id).ToList();
  foreach (var c in allClasses)
    ctx.LoadProperty(c, o => o.Students);

  // For each class...
  for (var i = 0; i < allClasses.Count; i++) 
  {
      var c = allClasses[i];
      var duplicates = allClasses
             .Where(o => o.SubjectId == c.SubjectId && o.Id != c.Id &&
                         o.Students.OrderBy(s => s.Id).Select(s => s.Id)
                              .SequenceEqual(c.Students.OrderBy(s => s.Id)
                                                  .Select(s => s.Id)))
             .ToList();

      // Does it have any duplicates?
      if (duplicates.Count == 0) 
          continue;

      ctx.LoadProperty(c, o => o.PeriodsTimetable);
      // For each duplicate...
      for (var j = 0; j < duplicates.Count; j++) 
      {
          var d = duplicates[j];
          // If the duplicate class has a timetable slot that's not also allocated to 
          // the class we are keeping, then allocate it (otherwise MySql will delete it).
          ctx.LoadProperty(d, o => o.PeriodsTimetable);
          var dt = d.PeriodsTimetable.ToList();
          for (var k = 0; k < dt.Count; k++) 
          {
              var dtp = dt[k];
              var tpIsDuplicate = false;
              foreach (var ctp in c.PeriodsTimetable) 
              {
                  if (dtp.TeacherId == ctp.TeacherId && 
                      dtp.PeriodTime == ctp.PeriodTime) 
                  {
                      tpIsDuplicate = true;
                  }
              }

              if (!tpIsDuplicate) 
                  dtp.ClassId = c.Id;
      }

      // Now update all other entities which reference the duplicate to instead      
      // reference the class we are keeping.
      var dPeriods = ctx.Periods.Where(o => o.ClassId == d.Id).ToList();
      for (var k = 0; k < dPeriods.Count; k++) 
          dPeriods[k].ClassId = c.Id;

      var dAssessments = ctx.Assessments.Where(o => o.ClassId == d.Id).ToList();
      for (var k = 0; k < dAssessments.Count; k++) 
          dAssessments[k].ClassId = c.Id;

      var dSeatingPlans = ctx.SeatingPlans.Where(o => o.ClassId == d.Id).ToList();
      for (var k = 0; k < dSeatingPlans.Count; k++) 
          dSeatingPlans[k].ClassId = c.Id;

      var dMerits = ctx.Merits.Where(o => o.ClassId == d.Id).ToList();
      for (var k = 0; k < dMerits.Count; k++) 
          dMerits[k].ClassId = c.Id;

      sb.Append(d.LongName).Append(" merged with ").Append(c.LongName).Append(".<br />");
      // Finally, delete the duplicate
      ctx.Classes.DeleteObject(d);
      allClasses.Remove(d);
      counter++;
    }
  }
  ctx.SaveChanges();
  lblOutput.Text = "<b>Merged " + counter + " duplicate classes.</b><br /><br />" + sb.ToString();
}

Для представления масштаба, насчитывается около 1600 классах 30 учеников в каждом, и счетчик выходит чуть более 300.

Я бы очень признателен за любые предложения.



1219
3
задан 16 октября 2011 в 07:10 Источник Поделиться
Комментарии
1 ответ

Ваш код очень трудно читать. (Чтобы дать вам представление о том, как это сложно, я смотрю на это за последние 2 часа)


  • Вы используете традиционные для петли, когда вы должны быть с помощью оператора foreach петли.

  • Ваш "переменные цикла" слабо имени. Смотреть ближе к концу кода. Что такое с? Что такое д? Оставлять более короткие имена в небольших лямбды, индекс varialbes и, возможно, единичные случаи "полезность" объектов (например, класса StringBuilder или регулярное выражение).

  • Вы пытаетесь слишком трудно писать все в одну строку. для петель, если условия, большие запросы... все они должны занимать несколько строк. Возможно, исключение если (дубликатов.Счетчик == 0) и далее; линия. Я был бы не против , но нужно постараться свести их к минимуму (возможно, вы могли бы код так, чтобы линии не требуется).

  • У вас есть длинная цепочка присоеденить()s в строковом разработчике, когда вы действительно должны использовать один вызов AppendFormat().

  • Есть некоторые фрагменты кода, который будет работать лучше либо в качестве отдельного метода или запроса LINQ. Использовать его.

  • LINQ к объектам должны были сделать ассоциации между объектами, так что ваши соединения могут быть заменены с помощью свойств навигации. Если у вас нет их, вы должны.

  • У вас также есть некоторые бесполезные комментарии. // Для каждого класса... мы действительно нуждаемся в комментарий, чтобы сказать нам об этом? Ну, может быть, так как мы были используя для петли, но это должно было избежать в первую очередь.

У вас есть много проблем с производительностью тоже.


  • Вы звоните вызова метода toList() на каждый запрос, когда вы просто перечисляете на него большую часть времени.

  • У вас много повторяющихся петель на одной и той же коллекции тоже. Вы должны быть в состоянии сделать много за один цикл.

  • Ваш запрос на поиск дубликатов будет огромный успех. У меня нет много, чтобы сказать об этом прямо сейчас, но он уже делает это за o(п^2).

  • У меня ощущение, что вы не нужны призывы к LoadProperty(). Насколько я могу судить, большинство кода является заполнение данных вручную, когда вы, вероятно, не нужно. К сожалению, я не знаю сущности достаточно основы, чтобы знать лучше (хотя я смотрю на это прямо сейчас).

Применяя то, что я объяснил свой код, я бы начал с этого (надеюсь, я ничего не сломал).

var sb = new StringBuilder();
var counter = 0;
using (var ctx = new SchoolModel())
{
var allClasses = ctx.Classes
.Where(o => o.SubjectId > 0)
.OrderBy(o => o.Id)
.ToList();

foreach (var aClass in allClasses)
{
ctx.LoadProperty(aClass, o => o.Students);

// a properly crafted query could remove this completely and not use LINQ to Objects
var students = new HashSet<int>(aClass.Students.Select(s => s.Id));
var dupClasses = allClasses
.Where(o => o.SubjectId == aClass.SubjectId
&& o.Id != aClass.Id
&& students.SetEquals(o.Students.Select(s => s.Id)))
.ToList();

if (dupClasses.Count == 0) continue;

ctx.LoadProperty(aClass, o => o.PeriodsTimetable);

foreach (var dupClass in dupClasses)
{
// If the duplicate class has a timetable slot that's not also allocated to the
// class we are keeping, then allocate it (otherwise MySql will delete it).
ctx.LoadProperty(dupClass, o => o.PeriodsTimetable);

foreach (var dupTimetable in dupClass.PeriodsTimetable)
{
var isDuplicate = aClass.PeriodsTimetable
.Any(ctp => dupTimetable.TeacherId == ctp.TeacherId
&& dupTimetable.PeriodTime == ctp.PeriodTime);
if (!isDuplicate)
dupTimetable.ClassId = aClass.Id;
}

// Now update all other entities which reference the duplicate to instead reference
// the class we are keeping.
foreach (var dupPeriod in dupClass.Periods)
dupPeriod.ClassId = aClass.Id;
foreach (var dupAssessment in dupClass.Assessments)
dupAssessment.ClassId = aClass.Id;
foreach (var dupSeatingPlan in dupClass.SeatingPlans)
dupSeatingPlan.ClassId = aClass.Id;
foreach (var dupMerit in dupClass.Merits)
dupMerit.ClassId = aClass.Id;

sb.AppendFormat("{0} merged with {1}.<br />", dupClass.LongName, aClass.LongName);

// Finally, delete the duplicate
ctx.Classes.DeleteObject(dupClass);

// not sure this is really needed
allClasses.Remove(dupClass);
counter++;
}
}
ctx.SaveChanges();
Output = "<b>Merged " + counter + " duplicate classes.</b><br /><br />" + sb.ToString();
}

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

8
ответ дан 16 октября 2011 в 08:10 Источник Поделиться