Should derived classes hide the Default and Create static members inherited from Comparer?

Question

I am writing an IComparer<T> implementation by deriving from the Comparer<T> class, as recommended by MSDN. For example:

public class MyComparer : Comparer<MyClass>
{
    private readonly Helper _helper;

    public MyComparer(Helper helper)
    {
         if (helper == null)
             throw new ArgumentNullException(nameof(helper));

        _helper = helper;
    }

    public override int Compare(MyClass x, MyClass y)
    {
        // perform comparison using _helper
    }
}

However, under this approach, the MyComparer class inherits the Default and Create static members from the Comparer<T> class. This is undesirable, since the implementation of the said members is unrelated to my derived class, and may lead to misleading behaviour:

// calls MyClass.CompareTo or throws InvalidOperationException
MyComparer.Default.Compare(new MyClass(), new MyClass());

My comparer cannot have a default instance due to the required Helper argument, nor can it initialize itself from a Comparison<T>, so I cannot hide the inherited static members with meaningful implementations.

What is the recommended practice for such situations? I'm considering three options:

• Implement IComparer<T> manually, rather than deriving from Comparer<T>, so as to avoid inheriting the said static members

• Leave the inherited static members in place, and assume consumers will know not to use them

• Hide the inherited static members with new implementations that throw InvalidOperationException:

  public static new Comparer<MyClass> Default
  {
      get { throw new InvalidOperationException(); }
  }
  
  public static new Comparer<MyClass> Create(Comparison<MyClass> comparison)
  {
      throw new InvalidOperationException();
  }
  

Answer 1

Don't inherit from Comparer<T>. This is a classic misuse of inheritance for code sharing. Inheritance is supposed to be used to implement the Liskov Substitution Principle (LSP). Inheriting for code reuse is a hack because, as you found it, it exposes "junk" in your public API surface.

This is not an LSP violation because no base type contracts are being broken. Yet, it is a misuse of inheritance. The problem is that internals are being exposed in a way that API users might mistakenly rely on. It also hinders implementation changes in the future because removing the base class might break users.

Whether you can tolerate that dirtiness depends on the quality standards that you have for your public API surface. If you don't care about that then go ahead and adhere to DRY while not adhering to LSP. If a billion lines of code depend on your class you certainly don't want to expose a dirty base class. The issue here becomes a trade-off between encapsulation (consumers shouldn't need to know about the comparer's implementation) and saving work when creating the class.

You bring up the DRY principle. I'm not sure this is an instance of a violation of DRY. Dry tries to prevent duplicated code becoming inconsistent and it tries to prevent duplicates maintenance effort. Since the duplicate code here can never change (null ordering is contractual) I don't see this is a meaningful DRY violation. Rather, it's simply about saving work when creating the implementation.

Implementing IComparer<T> is easy enough, so do that. I don't see the need to implement IComparer anymore. The default implementation does not much. If you care about null inputs you will have to replicate that logic anyway in your own compare method. The code reuse you achieve is next to nothing.

I'm thinking of implementing my own ComparerBase

That would be a case of the same problem. Maybe you can instead make a static helper method that implements the boilerplate null and type handling. That static helper would not be exposed to API users. This is "composition over inheritance".

Hiding the static members is really confusing. Depending on subtle changes at the call site different methods will be called. Also, none of them are useful.

I'm not too concerned with the fact that static methods are now available through a different type name. These methods are not really inherited. They are just available as a C# feature. I believe this is for versioning resiliency. It is never recommended and various tools generate warnings around this. I would not be too concerned about that. For example, every Stream "inherits" certain static members such as Stream.Null and Stream.Synchronized or whatever they are called. Nobody considers this a problem.

Answer 2

In my opinion, do nothing about it, i.e. your own option:

Leave the inherited static members in place, and assume consumers will know not to use them

Your class also inherits from System.Object, hence possesses stuff like

// static method overload inherited from System.Object:
MyComparer.Equals(new MyClass(), new MyClass());
// also inherited from System.Object:
MyComparer.ReferenceEquals(new MyClass(), new MyClass());

An you can never avoid that, since object is a base class of any type you write.

You must assume that developers that consume your code, understand how static members (properties, methods, etc.) work in C#, also in the context of inheritance.

Good developer tools (IDEs) should complain about int.ReferenceEquals, MyComparer.ReferenceEquals, MyComparer.Default and so on, because these are misleading ways to write the calls.

Hiding members with new is almost always a bad idea. It confuses developers much more, in my experience. Avoid the new modifier (on type members) when possible.

Unlike usr (see other answer) I think Comparer<> is a great base class to use.