Never make public members virtual/abstract - really?

Question

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?

Answer 1

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.

Answer 2

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.

Answer 3

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.

Answer 4

tl;dr– Argument-validation is a run-time check for the part of the method-signature that isn't checked at compile-time. Prohibiting public abstract/virtual methods is a pattern for ensuring that the exact same code-validation is used, consistent with the requirement that the exact same compile-time checks apply to the same signature-group.

---

The rationale for not exposing abstract/virtual methods is that methods should always have the same signature, both formally and informally.

For example:

class A
{
    public virtual void Print(string? text)
    {
        if (String.IsNullOrEmpty(text)) { throw new InvalidArgumentException(); }
        Console.WriteLine(text);
    }
}
class B1 : A
{
    public override void Print(string? text)
    {
        if (String.IsNullOrEmpty(text)) { throw new InvalidArgumentException(); }
        Console.WriteLine(text);
    }
}
class B2 : A
{
    public override void Print(string? text)
    {
        if (text == null) { throw new NullArgumentException(); }
        if (text.Equals(String.Empty)) { throw new InvalidArgumentException(); }
        Console.WriteLine(text);
    }
}
class B3 : A
{
    public override void Print(string? text)
    {
        Console.WriteLine(text ?? "[null]");
    }
}

In the code above, string? text can't be null or empty (""). Anyone calling .Print() should understand, by contract, that they're actually calling

public virtual void Print([non-null, non-empty]string text)

, leaving three possibilities for inheritors:

1. Correctly duplicate the exact same code for validation.

   • Example: B1.Print() duplicates A.Print()'s validation code.

2. Correctly reproduce the same effective-signature, but with inconsistent behavior.

   • Example: B2.Print() has the same effective-signature as A.Print(), but it has inconsistent behavior (returning a different type of exception for a null-argument).

3. Incorrectly change the effective-signature in addition to inconsistent behavior.

   • Example: B3.Print() accepts null/empty strings.

For example, say someone's trying to debug their code:

var list = new List<A>();
list.Add(new A());
list.Add(new B1());
list.Add(new B2());
list.Add(new B3());

foreach (A item in list)
{
    item.Print(null);
}

, where they improperly call .Print(null) four times. It's the exact same error, but:

1. They'll get 2 InvalidArgumentException's (from A.Print() and B1.Print()).

2. They'll get 1 NullArgumentException (from B2.Print()).

3. They'll not get an exception for the fourth mistake, which'll fail silently.

4. They'll also get an effective Print("[null]") despite never having called Print("[null]") and having no reason to expect the method to produce this behavior from its code-contract. (Yeah, it's not too hard to figure out what happened in this simple example, but in larger code projects, tracking down stuff like this can be fun!)

---

Summary of problems.

In short, the problems can include: code-duplication, inconsistent debug-feedback, silent-failures, and unexpected-behavior.

Ultimately inheritors have two options:

1. Copy/paste the validation code from the base-method in every override (whether directly or through memorizing it and retyping it). Require anyone changing any of them to copy/paste the new version everywhere else too.

2. Have the code-base polluted with inconsistent debug-feedback, silent errors, and inconsistent behaviors.

Both options make headaches for maintenance. Option (1) requires maintainers to be sure that they copy/paste any modifications into each virtual/override in the same family while Option (2) can result in all sorts of weird problems to untangle.

---

Better pattern.

class A
{
    public void Print(string? text)
    {
        if (String.IsNullOrEmpty(text)) { throw new InvalidArgumentException(); }
        this.Internal_Print(text);
    }
    protected virtual void Internal_Print(string? text)
    {
        Console.WriteLine(text);
    }
}
class B : A
{
    protected override void Internal_Print(string? text)
    {
        Console.WriteLine(text);
    }
}

This pattern prevents both the need and ability of inheritors to change the parameter-validation. Now we don't have to deal with copy/pasting validation code and the inconsistent/unexpected behaviors that can come from doing anything other than copy/pasting validation code.

---

Discussion: Conceptual argument for locking the implicit part of a signature.

A method-signature effectively has two parts:

1. An explicitly specified part, with compile-time validation.

2. An implicitly specified part, with run-time validation.

The language forces inheritors to maintain consistency with the explicit part, but the implicit part isn't fully captured by the type-system.

So, coders can either:

1. maintain consistency by copy/pasting the same validation code everywhere;

2. maintain consistency by requiring methods to create correct types that precisely describe the expected argument space (e.g., using non-null types when null isn't allowed);

3. break consistency by doing their own thing in each method;

4. maintain consistency through a combination of (1) and (2) using the above pattern.

---

Note: Historical context of string? vs. string.

The above code uses string? for consistency with modern C#. However, before C# fixed null-reference typing ambiguities, this same code would've been written as

public virtual void Print(string text)

, because the nullable-qualifier, ?, used to be implicitly assumed on all reference-types.

So if you're confused as to why we'd make the argument nullable only to then protest nullability, that's why: this is meant to reflect a historically common pattern, but written in more recent language.