Back in the 2000s a colleague of mine told me that it is an anti-pattern to make public methods virtual or abstract.
For example, he considered a class like this not well designed:
public abstract class PublicAbstractOrVirtual
{
public abstract void Method1(string argument);
public virtual void Method2(string argument)
{
if (argument == null) throw new ArgumentNullException(nameof(argument));
// default implementation
}
}
He stated that
- the developer of a derived class that implements
Method1
and overridesMethod2
has to repeat the argument validation. - in case the developer of the base class decides to add something around the customizable part of
Method1
orMethod2
later, he cannot do it.
Instead my colleague proposed this approach:
public abstract class ProtectedAbstractOrVirtual
{
public void Method1(string argument)
{
if (argument == null) throw new ArgumentNullException(nameof(argument));
this.Method1Core(argument);
}
public void Method2(string argument)
{
if (argument == null) throw new ArgumentNullException(nameof(argument));
this.Method2Core(argument);
}
protected abstract void Method1Core(string argument);
protected virtual void Method2Core(string argument)
{
// default implementation
}
}
He told me making public methods (or properties) virtual or abstract is as bad as making fields public. By wrapping fields into properties one can intercept any access to that fields later, if needed. The same applies to public virtual/abstract members: wrapping them the way as shown in the ProtectedAbstractOrVirtual
class allows the base class developer to intercept any calls that go to the virtual/abstract methods.
But I don't see this as a design guideline. Even Microsoft doesn't follow it: just have a look at the Stream
class to verify this.
What do you think of that guidline? Does it make any sense, or do you think it's overcomplicating the API?
Best Answer
Saying
is mixing up cause and effect. It makes the assumption that every overrideable method requires a non-customizable argument validation. But it is quite the other way round:
If one wants to design a method in a way it provides some fixed argument validations in all derivations of the class (or - more general - a customizable and a non-customizable part), then it makes sense to make the entry point non-virtual, and instead provide a virtual or abstract method for the customizable part which is called internally.
But there are lots of examples where it makes perfect sense to have a public virtual method, since there is no fixed non-customizable part: look at standard methods like
ToString
orEquals
orGetHashCode
- would it improve the design of theobject
class to have these not public and virtual at the same time? I don't think so.Or, in terms of your own code: when the code in the base class finally and intentionally looks like this
having this separation between
Method1
andMethod1Core
only complicates things for no apparent reason.