Do Not Make Every Method Virtual

I’m reacting to Roy Osherove’s recommendation, “make methods virtual by default”, in his excellent The Art of Unit Testing which I reviewed a few days ago.

I’ve heard my friend Jeremy Miller wish that .NET methods were virtual by default as they are in Java. I respect these guys immensely but I strenuously disagree. You should make each member virtual reluctantly and in the full knowledge of the risk you are taking. I hope you’ll at least appreciate my concerns even if you are not convinced.

Why Virtualize All Members By Default

It’s Roy’s first suggestion in his “Design for Testability” appendix [258]. He calls it “handy” and it is handy indeed. The easiest way to stub or hand-mock a class is the “extract and override” approach in which you override a production class to gain access to its innards during testing.

In one such example [71], a TestableProductionClass derives from a ProductionClass so that the implementation of the latter’s GetConcreteDependency() can be replaced with an alternative that returns a stub class instead of the concrete class.

// ProductionClass
protected virtual ILogger GetLogger() { return new ProductionLogger();}

// TestableProductionClass : ProductionClass
protected override GetLogger() { return new TestLogger(); }

What an easy way to swap out the dependency on the production logger … and I don’t need any of that messy IoC machinery.

If every method were virtual, any member could be replaced in this fashion within the test environment.

Making every method virtual makes life easier for the mocking frameworks too. Most do well at fabricating mocks either for interfaces or for unsealed classes with protected virtual methods. I believe it is fair to say that most have a tougher time mocking closed concrete classes.

A class with all virtual methods is easier to proxy too, lending itself to injection techniques such as Ayende demonstrates in his post about injecting INotifyPropertyChanged into POCO objects.

What could possibly be wrong with this?

Invites Violation of Liskov Substitution Principle

The “Liskov Substitution Principle (LSP)” is the “L” in Robert “Uncle Bob” Martin’s SOLID design principles. Formally this principle states that the derived class can’t make a method’s pre-conditions stronger nor post-conditions weaker. In colloquial English, the derived method can’t change the fundamental guarantees of the base method. It may do things differently but it shouldn’t violate our expectations of what goes in, what comes out, or what the method does.

Uncle Bob, in his Agile Principles, Patterns, and Practices in C# , describes LSP as a “prime enabler of the Open Closed Principle (OCP)” [151]. It follows that a violation of LSP is effectively a violation of the more familiar OCP.

Now I have no problme with deliberately making some methods virtual. That is one of the favored techniques for facilitating OCP; it is a mechanism for identifying an opening for extension.

My argument is with opening up the class blindly and totally by making everything virtual. Suddenly nothing in the class is truly “closed for modification.” The “virtual” keyword announces to the world “here is my extension point.” When every method is virtual, the world is invited to change every method.

The lesson of Martin’s LSP chapter is that extending a class by overriding a virtual method is tricky business. The innocent seeming Rectangle is his famous example. What could go wrong in deriving Square from Rectangle and overriding its Length and Width properties? It turns out that plenty can go wrong [140] and that it’s almost impossible for the author of the class to anticipate and guard against the abuse of his class.

Martin is pragmatic. “Accepting compromise instead of pursuing perfection is an engineering trade-off. … However, conformance to LSP should not be surrendered lightly.” [his emphasis, 149] . He continues:

The guarantee that a subclass will always work where its base classes are used is a powerful way to manage complexity. Once it is forsaken, we must consider each subclass individually.

I’m going to come back to this point. Because we give up the guarantee … and open wide the door to big trouble … when we make every member virtual.

Let back up a second and elaborate on the danger.

The Wayward Elevator

I am the maker of Elevator software that runs elevators around the globe. My Elevator class has an Up method. Suppose I make it virtual. How might value-added elevator developers implement an override of a virtual Up in their derived BetterElevator class? They could

• replace it completely; base.Up() not called
• call base.Up(), then invoke custom logic
• invoke custom logic, then call base.Up()
• wrap base.Up() in pre- and post-logic

It’s not an issue when I am doing the deriving. I almost never call base when I derive a test class (TestElevator). If I support construction of a dynamic proxy to address cross-cutting concerns, I expect the proxy to wrap the base method. These scenarios are not worrisome to me. Why?

When writing tests, I have intimate knowledge of Elevator. Elevator’s details are open to me. I wrote it. I know what I’m doing.

If I make Up() accessible to a proxy, I again know what I’m doing … and more importantly, I know how the proxy will manipulate my method. The proxy may behave blindly but it operates automatically and predictably. Whether I decorate the method with attributes or map it or rely on some conventions, I, the author of the class, am choosing precisely how it will be extended.

Unfortunately, I can’t declare Up() to be “virtual for testing” or “virtual for proxying”. It is simply “virtual”, an invitation to extension by unknown developers in unknown ways. I have lost control of my class.

I knew that Up() sent the elevator ascending. But I can’t stop someone from re-implementing Up() so that it descends instead. Maybe base.Up() triggers the doors to close. The developer might call base.Up() too late, sending the elevator in motion before the doors have closed. The developer could replace my base.Up with something that juggled the sequence of door closing and elevator motion methods, interleaving custom behaviors, yielding upward motion that failed to satisfy some other elevator guarantees.

Any of these or a hundred other implementations of Up() could alter Up-ness in ways that are subtly incorrect or catastrophically wrong. Everyone one of those implementations requires that the developer understand intimate details of the Elevator class, details that he would not have to know if the method were sealed.

“Up” is definitely not closed to modification. It is dangerously open. As an Elevator man, I should work hard … perhaps writing a lot of Elevator.Up certification tests … to ensure that essential pre-conditions, post-conditions, and possible side-effects are all correct even for derived classes.

The burden on my development has gone up as well, not through conscious design but by default. This is unthinking design, fiat design. My code fails “open” instead of “failing” closed. I better be very good and very conscientious about manually sealing what the language would make virtual on its own.

I’m not that good and I’m definitely not conscientious.

Is This Paternalism?

Am I being absurdly protective. Sure there are risks. Programmers are adults and we should treat them as such.

I hear this a lot. I hear about how Microsoft tends to infantilize the developer, tries to shield them from the bumps and bruises that are essential to mastering the profession. There comes a time when you let the kid have sharp scissors and run around with them if he must. A few blind kids is a price worth paying.

I agree … as long as I’m not paying the medical bills.

I Write Frameworks, You Write Applications

I can’t tell you how to build your applications. When you own the class … and you do as an application developer … your team is answerable to you. You don’t come to me with your medical bills.

But I write frameworks for you to use. You’ve licensed my product and you’re paying me for support. When the elevator goes down instead of up; when the doors close suddenly and injure a rider; when the elevator simply stops … you don’t say “I wonder what I did?” You say “that elevator we bought is a piece of crap.”

You paid for support. You call me. And I am pleased to give it.

It’s not free to you. It’s not free to me either. I have to find what’s wrong as quickly as possible and get your elevator moving safely again. Unfortunately, if you’ve written BetterElevator (as you are supposed to do … it’s a framework remember) and you can change everything about my Elevator, I face an extremely challenging support call.

I have no idea what you’ve done. I don’t know what you’ve done to elevator and I can’t guess. You can tell me … you will tell me … that you haven’t touched “Up”. Perhaps you didn’t. Instead you’ve overridden another method on another of my classes that Up requires.

Maybe you don’t write applications. Maybe you write an open source framework. Fantastic. You don’t have true customers. If someone has a problem, you diagnose it and maybe you fix it … at your leisure. That someone has no recourse … and knows that going in.

That’s often why businesses won’t use open source. As more than one manager has told me, “I want a neck to wring.”

I think I’m entitled to fear for my throat. But that’s not my real motivation. I’m in the business of providing good service for a product that makes certain behavioral guarantees. I can’t deliver good service if I can’t make those guarantees and I can’t make those guarantees if every method of every class is up for grabs.

I’m not sure you can either.

.NET Framework Design Guidelines

I’m not the only guy with a healthy suspicion of virtual methods. I admit my degree of suspicion is higher than the that of the application architect’s. The application architect probably knows and controls the developer who derives from his class. The unknown developer who derives from my class controls me.

The designers of .NET understand this too well. That’s why in the .NET Framework Design Guidelines, Krzysztof Cwalina and Brad Abrams write

Virtual members … are costly to design, test, and maintain because any call to a virtual member can be overridden in unpredictable ways and can execute arbitrary code. .. [M]uch more effort is usually required to clearly define the contract of virtual members, so the cost of designing and documenting them is higher.  [201]

If all members are virtual by default,

• I waste effort manually sealing most of them,
• I test and document and support more methods than I have resources to support
• I add unwanted complexity

and what do I gain for my pains?

What Should We Do?

I want to blame someone. I’m going to blame the language authors. Maybe methods are automatically virtual only in test environments. We should be able to mark methods for proxying and compile time injection (as in static Aspect Oriented Programming). Otherwise, members are closed unless explicitly made virtual through the conscious effort of a conscientious programmer.

Meanwhile, I’m prepared to be pragmatic. I like Roy’s recommendation [260] that dependencies should be defined in virtual properties (or methods) without logic.  Auto-properties and similar logic-less methods are safer to make virtual. The Template Design Pattern is a controlled approach to extensibility that may assist testability as well. Interface-based designs help too.

That’s as far as I dare go as a framework developer. Application developers may have more rope to hang … er … more latitude.