8
\$\begingroup\$

I have this custom IComparer I use to sort a list of SurveyResponse objects, but it seems really inefficient, both in terms of runtime performance, and code elegance and maintainability. I thought of implementing IComparable for each of these components, and each CompareTo knowing its "parent" to compare, but the sort order for this report I'm making won't necessarily be the same as other sort orders, so that may not work.

It looks like the sort of problem that should be solvable using recursion, but maybe I'm just over-analyzing it?

Are there any best practices that would make this faster and more maintainable?

private class CommentComparer : IComparer<SurveyResponse>
{
  public int Compare(SurveyResponse x, SurveyResponse y)
  {
    // Sort by Application Name...
    int result = x.Question.Survey.Feature.Application.Name.CompareTo(
      y.Question.Survey.Feature.Application.Name);
    if (result != 0) { return result; }
    // ...then Feature Name...
    result = x.Question.Survey.Feature.Name.CompareTo(
      y.Question.Survey.Feature.Name);
    if (result != 0) { return result; }
    // ...then SurveyTime...
    result = x.Question.Survey.TimeTaken.CompareTo(y.Question.Survey.TimeTaken);
    if (result != 0) { return result; }
    // ...then ID...
    result = x.Question.Survey.ID.CompareTo(y.Question.Survey.ID);
    if (result != 0) { return result; }
    // ...then Position.
    return x.Question.Position.CompareTo(y.Question.Position);
  }
}
\$\endgroup\$
8
\$\begingroup\$

You can use types from ComparerExtensions to make your code more maintainable:

IComparer<SurveyResponse> comparer = KeyComparer<SurveyResponse>
    .OrderBy(sr => sr.Question.Survey.Feature.Application.Name)
    .ThenBy(sr => sr.Question.Survey.Feature.Name)
    .ThenBy(sr => sr.Question.Survey.TimeTaken)
    .ThenBy(sr => sr.Question.Survey.ID)
    .ThenBy(sr => sr.Question.Position);

But I don't think you can actually make your comparer more efficient, it should be already very fast (it's just a few simple instructions). Are you sure your performance problem is caused by this comparer?

\$\endgroup\$
  • 1
    \$\begingroup\$ I've been pulled off that project, but any performance delay was probably more likely caused by the fact that the SurveyResponse objects are NHibernate-mapped, lazily-initialized objects. I probably need to explicitly pre-fill all the data I might need to compare when originally populating the list. I didn't get a chance to eliminate that delay yet, and thought I'd check on the performance, here, while reviewing the ugliness of the code, too. Thanks! :) \$\endgroup\$ – mo. Jun 5 '12 at 18:39
  • \$\begingroup\$ Very elegant! But this code might actually have worse performance than the OP's code simply because the OP's has short-circuiting. That is, if any of the CompareTos finds a difference, it returns immediately. Does this code do the same? I would guess not. \$\endgroup\$ – Pat Jun 8 '12 at 23:45
  • \$\begingroup\$ @Pat It does use short-circuiting (see CompoundComparer<T>.Compare() and notice the condition in for). But the performance might be actually worse, because of delegate invocations. \$\endgroup\$ – svick Jun 9 '12 at 0:04
3
\$\begingroup\$

+1 to @svick of course. Here is another solution without NList, LINQ, etc., maybe somebody find it interesting. Creating a separate IComparer for every comparison also could work, for example:

private class ApplicationNameComparer : IComparer<SurveyResponse> {
    public int Compare(SurveyResponse x, SurveyResponse y) {
        return x.Question.Survey.Feature.Application.Name.
            CompareTo(y.Question.Survey.Feature.Application.Name);
    }
}

then put them into a list:

var comparers = new List<IComparer<SurveyResponse>>();
comparers.Add(new ApplicationNameComparer());
comparers.Add(new FeatureNameComparer());
comparers.Add(new SurveyTimeComparer());
comparers.Add(new IdComparer());
comparers.Add(new PositionComparer());

finally iterate through the list:

foreach (var comparer in comparers) {
    int result = comparer.Compare(x, y);        
    if (result != 0) {
        return result;
    }
}
return 0;

It removes the logic repetition of if (result != 0) { return result; } but it uses more classes. On the other hand every class has single responsibility and they do something more complex together.

References:

\$\endgroup\$
  • \$\begingroup\$ Thanks for your insight. I can't vote up on CR yet, but I would if I could. I think your solution would be more suitable if this wasn't a quick one-off problem I needed to solve :) \$\endgroup\$ – mo. Jun 5 '12 at 18:34
  • \$\begingroup\$ @mo. You can now :-) \$\endgroup\$ – svick Jun 5 '12 at 18:53
  • \$\begingroup\$ I don't like this solution much because it's even more repetition than the original code. \$\endgroup\$ – svick Jun 5 '12 at 18:55
  • \$\begingroup\$ @svick: Thanks for the edit! What kind of repetition do you think? \$\endgroup\$ – palacsint Jun 5 '12 at 19:11
  • 1
    \$\begingroup\$ As the creator of NList, I can confirm that @palacsint's solution is how NList is implemented internally. \$\endgroup\$ – Travis Parks Jun 13 '12 at 20:40
2
\$\begingroup\$

As the creator of NList, I can confirm that @palacsint's solution is how NList is implemented internally.

Another way to implement it is using the chain-of-command pattern.

public class CompoundComparer<T> : IComparer<T>
{
    private readonly IComparer<T> first;
    private readonly IComparer<T> second;

    public CompoundComparer(IComparer<T> first, IComparer<T> second)
    {
        this.first = first;
        this.second = second;
    }

    public int Compare(T x, Ty)
    {
        int result = first(x, y);
        if (result != 0)
        {
            return result;
        }
        return second(x, y);
    }
}

Once you've created this class, you can compose them together to build more complex comparisons.

IComparer<T> comparer = new CompoundComparer<T>(
    comparison1,
    new CompoundComparison(
        comparison2,
        comparison3));

Some helper functions can make this easier to read/write. At that point, though, it is probably just easier to steal code from NList. I wrote it so you don't have to.

\$\endgroup\$

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.