3
\$\begingroup\$

The C# code below, using LINQ to Entities, aims to merge all Class entities which have the same SubjectId and the same Students.

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

For an idea of scale, there are about 1600 classes with 30 students each, and counter comes out at just over 300.

I'd really appreciate any suggestions.

\$\endgroup\$
8
\$\begingroup\$

Your code is very difficult to read. (To give you an idea of how difficult it is, I've been looking at this for the past 2 hours so far)

  • You are using traditional for loops when you instead should be using foreach loops.
  • Your "loop variables" are poorly named. Look toward the end of the code. What is a c? What is a d? Leave the shorter names within smaller lambdas, index varialbes and possibly single instances of "utility" objects (e.g., StringBuilder or Regex).
  • You are trying too hard to write everything within a single line. for loops, if conditions, large queries... all of them should span multiple lines. With perhaps an exception to the if (duplicates.Count == 0) continue; line. I'd be okay with that but you should try to keep those to a minimum (it might be possible that you could code it so that line is not needed).
  • You have a long chain of Append()s to the string builder when you really should use a single call to AppendFormat().
  • There are some snippets of your code which would work better either as a separate method or a LINQ query. Use it.
  • LINQ to Entities should have made associations between your objects so your joins could be replaced by using the navigation properties. If you don't have those set up, you should.
  • You also have some useless comments. // For each class... Do we really need a comment to tell us that? Well, maybe since we were using for loops but that should have been avoided in the first place.

You have a lot of performance issues too.

  • You are calling ToList() on every query when you are simply just enumerating over it most of the time.
  • You have a lot of repeated loops on the same collection too. You should be able to do a lot within a single loop.
  • Your query for finding duplicates will be a huge hit. I don't have much to say on it right now but it's already making this O(n^2).
  • I have a feeling that you don't need the calls to LoadProperty(). From what I can tell, the majority of your code is filling in data manually when you probably don't need to. Unfortunately I don't know the entity framework enough to know better (though I am looking into it right now).

Applying what I've explained to your code, I'd start with this (hopefully I didn't break anything).

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

That's my initial analysis so far. I'll update some more once I've gained understanding on what you are doing. As I've said, it is very difficult to read. I'll probably offer more improvements once I've had a chance to look at it more.

\$\endgroup\$
  • \$\begingroup\$ Thank you very much for looking into this. I like some of your ideas and will implement them. In response to a few of your points: (1) The compiler won't let me use foreach because the enumerable collection is modified in each case (though I'm not sure how in some cases), (2) The variable names made sense to me, e.g. c=class, d=duplicate, and I'm working on this project alone, but I accept that they are unclear - sorry, (3) Without ToList() when looping through, for example, PeriodsTimetable, an error is thrown about updating references. Again I'm afraid I don't understand why. \$\endgroup\$ – James Oct 16 '11 at 20:59
  • \$\begingroup\$ For points (1) and (3), you probably would be able to use AsEnumerable() instead of calling ToList(). It would seem that updating objects when enumerating through an entity set is not possible. Using AsEnumerable() would force using LINQ to Objects and hopefully allow this to work without being forced to throw them into a list. \$\endgroup\$ – Jeff Mercado Oct 16 '11 at 21:12
  • \$\begingroup\$ No luck, I'm afraid. The type is already IEnumerable<EntityName> so AsEnumerable() has no effect. I very much like your HashSet<int>, PeriodsTimetable.Any and sb.AppendFormat improvements and am testing them now. Thanks again. \$\endgroup\$ – James Oct 16 '11 at 21:26
  • \$\begingroup\$ Ah, after having thought about it, I could now see why that would cause problems. We're changing the FK so it would alter the collection indirectly since the changes are tracked. I'm still exploring what would be the better way to handle this. I think you might be able to do something like this instead (looking at the period updates): dupPeriod.Class = aClass; instead of changing the id's, it will all be handled for you (and no need to use ToList()). I hope I just didn't butcher that explanation. :) \$\endgroup\$ – Jeff Mercado Oct 17 '11 at 2:02
  • \$\begingroup\$ I have a few remarks because small parts of the code go back to my answer on StackOverflow: stackoverflow.com/questions/7780455/… . 1) LoadProperty (explicite loading) is ugly ideed and the code is a typical case where one would tend to use Include (eager loading). However, it has serious performance problems (see James' comment to my answer about the perf. gain with explicite loading). 2) You cannot move LoadProperty(aClass, o => o.Students) into the main loop because the inner query for dupClasses will fetch classes... \$\endgroup\$ – Slauma Oct 19 '11 at 18:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.