Intro
Right off the bat, I know the title is a bit clickbaity, but treating a virtual method as a code smell is actually something I have been doing for a while without even noticing, until just recently when I had a discussion and a light bulb lit over my head, so I thought it could result in an interesting blog post.
Note that the asterisk in the title is not a typo, but rather an acknowledgment that the train of thought I'll share in this post is heavily influenced by the types of projects I've been working on, so it doesn't apply in all contexts.
For my context, which is more often line of business applications, not as much library or framework code, I've been noticing this trend.
And just before getting into it, a friendly reminder that a code smell isn't necessarily something that's wrong, it's just something that should be given some extra attention to understand if it makes sense or not.
Where is this coming from
So, where does this "virtual as a code smell" come from?
Recently I noticed after reviewing a couple of pull requests, as soon as I notice the virtual keyword being used, I give it a second look (and maybe also a third or fourth), eventually resulting in asking, "is this really needed?".
For starters, using inheritance is already something that should be well thought out (you know, composition over inheritance and that sort of stuff), so that by itself makes using virtual a very localized occurrence. On this note, I appreciate that Kotlin's classes are final by default, cause I always forget to seal them in C# (some related thoughts by Mark Seemann).
In addition to not inheriting that often, when I do, it's rare that I make a method virtual. From my recent memory, when implementing class hierarchies, it mostly fell on something like:
- There is an evident "is-a" relationship that warrants the hierarchy (e.g. the error type I discussed in a previous post)
- Implementing the template method pattern, resorting to abstract methods
Off the top of my head, I can't really remember the last time I marked a method as virtual. I remember overriding virtual methods, all of them from libraries or frameworks (e.g. EF Core SaveChanges methods), but marking them as virtual? Nope, can't remember.
And to better grasp why I don't think it's needed most of the time, introducing a common counterargument might be of assistance.
Extensibility and the open/closed principle
The first reaction to my question "why is this virtual needed?" was "in case the inheriting classes want to override it". That "in case" triggered me (probably more than it should), not only because in that context it didn't really make sense (which was probably the biggest cause for my overreaction), but also because we're getting into the futurology realm, where we don't have an actual use case, but doing things "just in case".
When I pushed on the subject, the follow-up reasoning was mentioning the open/closed principle. This is sadly a recurrent misconception about the principle in particular (it doesn't help that this principle, like a couple of others in SOLID, is a bit too vague) and object oriented design in general (side note, Dan North has some interesting thoughts on SOLID in general).
This sort of thinking, is assuming that extensibility points should be provided by way of inheritance, which I'd argue isn't the way to go. There are multiple ways to provide extensibility points, it can be something as simple as passing a function as an argument (ASP.NET Core's request pipeline is a good example of this).
Getting back to the futurology subject and the open/closed principle, my take on it, particularly in modern times, where the pace of change is so fast, is that we should consider it in terms of the current requirements, not trying to guess the future.
As an example, which I used in my "OOPs, I did it again" presentation, if there's a requirement to conditionally apply multiple rules to a given use case, it might make sense to create a way to represent a rule and make it easy to implement multiple times (e.g. create an interface and implement it for each specific rule). Again, in this case we're not too heavy on the guess work, we have an actual present requirement and maybe are aware of further future requirements in terms of implementing more rules, we're not overcomplicating code "just in case".
The asterisk
Before wrapping up, I think it's worth adding another note about the asterisk in the title.
As I mentioned, these thoughts are coming from the context of developing line of business applications. Framework and library code has different characteristics from application code, so your mileage may vary, but even then, it doesn't jump off to me as wise to just make everything unsealed and virtual as a strategy for extensibility.
Extensibility should be thought out, not just opening up everything, as that might result in unforeseen consequences.
As Mark Seemann mentions in this post on sealed by default, sealing an unsealed class is a breaking change, unsealing a sealed class isn't. Same applies to virtual methods. Start out with the thing as constrained as possible and open up extensibility points as needed, explicitly and with concrete use cases in mind, not "just in case". If the extensibility point presents itself as making a method virtual, then, by all means.
Outro
That's it for this one. I'm fully aware this isn't a topic where we're all going to agree, particularly considering the discussion on Mark Seemann's tweet, and even more on Kotlin's final by default decision (now that's a loooong discussion 😛), but I felt like sharing by two cents, particularly after the recent aha moment 🙂.
Links in the post:
- Composition over inheritance - Wikipedia article
- Is-a - Wikipedia article
- Template method pattern - Wikipedia article
- CUPID – the back story
- Inheritance - Kotlin docs
- Pendulum swing: sealed by default - Mark Seemann
- Mark Seemann's tweet on sealed by default
- Kotlin classes final by default forum discussion
- OOPs, I did it again
- More explicit domain error handling and fewer exceptions with Either and Error types [ASPF02O|E038]
Thanks for stopping by, cyaz!
Oldest comments (2)
Nice article and completely agree that virtual is a smell outside of libraries and such, and even in them if inheritance is used improperly. However, I also consider
final,sealed, and the like to be a smell, but sealed as default (in languages that doesn't provide a way to override the seal) is an even greater stink.Outside specific design requirements, marking something as sealed provides no benefit and pointlessly removes a tool that could be used later by consumers of the product.
Say I need to interop with your program, or with data files it outputs. So I reference your application in mine and attempt to reuse as much logic and infrastructure as possible. However, since my project doesn't have the same purpose as yours, I need to tweak a tiny bit of logic. Now, I could have simply inherited from one of your classes, tweaked a single method, and passed it along to your logic, happy as could be...
Only, I can't, because the devs of your project were seal happy. Instead, if I'm lucky, the stars align, and the language allows, then I can spin some reflection magic to bypass your seal. If I'm not lucky, I have to recreate a bunch of logic and code.
I guess all of that is ok if it offset by some benefit, but there is no benefit to sealing general things other than a vague idea that it might discourage over use of inheritance.
As with anything else in the coding world, if you mark something as sealed, then you should have a specific reason for doing so. If there isn't a specific need for the thing to be sealed, then don't seal it. No user or consumer is ever helped by a thing being sealed, but there is a chance they might be hampered by it.
That's a valid point.
Don't agree that sealed by default doesn't provide value though. The value is enforcing as much as possible that the code is used as it was intended to, minimizing the surprise factor when doing something it wasn't originally designed to do.
But of course, this has the problem of not allowing simple tweaks, as you mentioned, which could have no unintended side effects, but fix a given problem.
As usual, trade-offs 🙂.