20

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 overrides Method2 has to repeat the argument validation.
  • in case the developer of the base class decides to add something around the customizable part of Method1 or Method2 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?

  • 5
    Making methods virtual allows optional overriding. Your method should probably be public, because it might not be overridden. Making methods abstract forces you to override them; they should probably be protected, because they're not particularly useful in a public context. – Robert Harvey Sep 4 at 14:50
  • 4
    Actually, protected is most useful when you want to expose private members of the abstract class to derived classes. In any case, I'm not particularly concerned about your friend's opinion; choose the access modifier that makes the most sense for your particular situation. – Robert Harvey Sep 4 at 14:53
  • 4
    Your colleague was advocating for the Template Method Pattern. There can be use cases for both ways, depending on how interdependent the two methods are. – Greg Burghardt Sep 4 at 15:42
  • 8
    @GregBurghardt: sounds like the OP's colleague suggests always to use the template method pattern, regardless if its required or not. That's a typical overuse of patterns - if one has a hammer, sooner or later every problem starts to look like a nail ;-) – Doc Brown Sep 4 at 16:16
  • 3
    @PeterPerot: I had never a problem to start with simple DTOs with just public fields, and when such a DTO turned out to require members with business logic, refactor them to classes with properties. Surely things are different when one works as a library vendor and has to care for not changing public APIs, then even turning a public field into a public property of the same name can cause issues. – Doc Brown Sep 5 at 10:20
30

Saying

that it is an anti-pattern to make public methods virtual or abstract because of the developer of a derived class that implements Method1 and overrides Method2 has to repeat the argument validation

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 or Equals or GetHashCode- would it improve the design of the object 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

 public void Method1(string argument)
 {
    // nothing to validate here, all strings including null allowed
    this.Method1Core(argument);
 }

having this separation between Method1 and Method1Core only complicates things for no apparent reason.

  • 1
    In case of the ToString() method, Microsoft had better made it non-virtual and introduced a virtual template method ToStringCore(). Why: because of this: ToString() - note to inheritors. They state that ToString() should not return null. They could have coerced this demand by implementing ToString() => ToStringCore() ?? string.Empty. – Peter Perot Sep 4 at 14:33
  • 9
    @PeterPerot: that guideline you linked to also recommends not to return string.Empty, did you notice? And it recommends a lot of other things which cannot be enforced in code by introducing something like a ToStringCore method. So this technique is probably not the right tool for ToString. – Doc Brown Sep 4 at 14:41
  • 3
    @Theraot: surely one can find some reasons or arguments why ToString and Equals or GetHashcode could have been designed differently, but today they are as they are (at least I think their design is good enough for make a good example). – Doc Brown Sep 4 at 15:35
  • 1
    @PeterPerot "I saw a lot of code like anyObjectIGot.ToString() instead of anyObjectIGot?.ToString()" - how is this relevant? Your ToStringCore() approach would prevent a null string being returned, but it still would throw a NullReferenceException if the object is null. – IMil Sep 4 at 23:34
  • 1
    @PeterPerot I was not trying to convey an argument from authority. It is not so much that Microsoft uses public virtual, but that there are cases in which public virtual is ok. We could argue for an empty non-customizable part as making the code future proof... But that does not work. Going back and changing it could break derived types. Thus, it earns nothing. – Theraot Sep 5 at 12:34
6

Doing it the way your colleague suggests does provide more flexibility to the implementor of the base class. But with it also comes more complexity which is typically not justified by the presumed benefits.

Mind that the increased flexibility for the base class implementer comes at the expense of less flexibility to the overriding party. They get some imposed behavior that they may not particularly care for. To them things have just gotten more rigid. This can be justified and helpful but this all depends on the scenario.

The naming convention to implement this (that I know of) is to reserve the good name for the public interface and to prefix the internal method's name with "Do".

One useful case is when the action performed needs some setting up and some closing down. Like open a stream and close it after the overrider is done. In general, same sort of initialization and finalization. It is a valid pattern to use but it would be pointless to mandate its use in all abstract and virtual scenarios.

  • 1
    The Do method prefix is one option. Microsoft often uses the Core method postfix. – Peter Perot Sep 5 at 11:25
  • @Peter Perot. I never saw the Core prefix in any Microsoft material but this may be because I have not been paying much attention lately. I suspect they started doing this recently just to promote the Core moniker, to make a name for .NET Core. – Martin Maat Sep 5 at 13:24
  • Nope, it's an old hat: BindingList. In addition I found the recommendation somewhere, maybe their Framework Design Guidelines or something similar. And: it's a postfix. ;-) – Peter Perot Sep 5 at 13:36
  • Less flexibility for the derived class is the point. The base class is an abstraction boundary. The base class tells the consumer what its public API does, and it defines the API is requires in order to accomplish those goals. If a derived class can override a public method in the base class, you run an increased risk of violating the Liskov Substitution Principle. – Adrian McCarthy Sep 5 at 20:16
2

In C++, this is called the non-virtual interface pattern (NVI). (Once upon a time, it was called the Template Method. That was confusing, but some of the older articles have that terminology.) NVI is promoted by Herb Sutter, who has written about it at least a few times. I think one of the earliest is here.

If I recall correctly, the premise is that a derived class should not change what the base class does but how it does it.

For example, a Shape might have a Move method to relocate the shape. A concrete implementations (e.g., like Square and Circles) shouldn't directly override Move, because Shape defines what Moving means (at a conceptual level). A Square might have difference implementation details than a Circle in terms of how the position is represented internally, so they will have to override some method to provide the Move functionality.

In simple examples, this often boils down to a public Move that just delegates all the work to a private virtual ReallyDoTheMove, so it seems like a lot of overhead for no benefit.

But this one-to-one correspondence is not a requirement. For example, you could add an Animate method to Shape's public API, and it might implement that by calling ReallyDoTheMove in a loop. You end up with two public non-virtual methods APIs that both rely on the one private abstract method. Your Circles and Squares don't need to do any extra work, nor can the override Animate.

The base class defines the public interface used by consumers, and it defines an interface of primitive operations that it needs to implement those public methods. The derived types are responsible for providing implementations of those primitive operations.

I'm not aware of any differences between C# and C++ that would change this aspect of class design.

  • Good find! Now I remember that I found exactly that post your 2nd link points to (or a copy of it), back in the 2000s. I remember that I was searching for more evidence of my colleague's claim, and that I didn't find anything in the C# context, but C++. This. Is. It! :-) But, back in C# land, it seems this pattern is not used much. Maybe people realized that adding base functionality later can break derived classes, too, and that the strict use of the TMP or NVIP instead of public virtual methods does not always make sense. – Peter Perot Sep 5 at 21:06
  • Note to self: good to know this pattern has a name: NVIP. – Peter Perot Sep 5 at 21:09

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.