4

I have a normal class called BaseView with a virtual method DisplayView. This method calls GetHeader and GetBody virtual methods to get contents for the page. I would then create a class that inherits from BaseView and override the methods that needs to display content differently than the way the base class does it.

My issue is that, although this works great, when running a code analysis I'm warned not to call the virtual functions directly.

Should I create another class layer on top of the base class that override the virtual functions and only inherit from that?

What are the disadvantages of using the virtual methods directly?

EDIT: the warning is:

CA2214 : Microsoft.Usage : xxx contains a call chain that results in a call to a virtual method defined by the class. Review the following call stack for unintended consequences

Cœur
  • 37,241
  • 25
  • 195
  • 267
Corne Beukes
  • 1,759
  • 4
  • 14
  • 18

1 Answers1

4

I think the issue is that DisplayView is virtual, and it's calling virtual methods. In most cases virtual methods are called by final methods as a means of changing behaviour, for example in the strategy pattern. If a final method calls a virtual method, the compiler knows that the virtual method will always be called in all deriving classes, and therefore it is valid for the virtual method to exist.

The fact that you're calling virtual from virtual means that your design can be called into question: if DisplayView is virtual, another implementation may override it. The current implementation calls the virtual GetHeader but a deriving class may not. Therefore it cannot guarantee that GetHeader is not dead code.

This is probably what FxCop is drawing your attention to. It wants to know that if you define a virtual method (GetHeader in this case) in a base class that all deriving implementations will use it.

I would focus on making DisplayView final, or appraise your design in that light.

Joe
  • 46,419
  • 33
  • 155
  • 245
  • I spent a long time using FxCop, and there is some very interesting thinking in the rules, most of which I reluctantly agree with! – Joe Dec 14 '11 at 11:31
  • but the [msdn doc on CA2214](http://msdn.microsoft.com/en-us/library/ms182331.aspx) says that it should be fine if the virtual method is not called from the constructor, or at least that is what i understood from that – Vamsi Dec 14 '11 at 11:32
  • Interesting. The question does not mention that this is called from the constructor. Is it? – Joe Dec 14 '11 at 11:35
  • Yes, the OP mentions that he is calling the virtual methods `GetHeader` and `GetBody` from another **virtual method** `DisplayView`, so i don't understand how this rule applies here – Vamsi Dec 14 '11 at 11:40
  • The intended behaviour is that the virtual GetHeader method may return a simple string containing the name of the view. In some cases, a more elaborate header may be needed, and then the derived class will override GetHeader. The virtual method DisplayView (called from the derived class) will then call the overridden method for GetHeader. – Corne Beukes Dec 14 '11 at 11:50
  • @Joe The methods aren't called from the constructor. – Corne Beukes Dec 14 '11 at 11:53
  • It is also intended behaviour that a specific derived class may not call GetHeader, for a view that does not require a header. – Corne Beukes Dec 14 '11 at 12:00
  • Well you can disable individual rules if you have a good reason, or further complicate your design. Your call! – Joe Dec 14 '11 at 12:14
  • Thank you all for the feedback. I will refactor my class hierarchy. With hindsight, GetHeader doesn't belong in the virtual method for DisplayView, since it isn't standard behaviour anymore to always display a header. I will create overridden versions of DisplayView for views that have headers and those that don't. I think my design flaw is that I presume default behaviours in my virtual methods that should exist in derived classes. – Corne Beukes Dec 14 '11 at 12:18
  • Yeah, I love the code analysis feature in Visual Studio. It is horrifying when you have an app that works perfectly, but then get 2000 warnings. The best part is working that list down to zero warnings. – Corne Beukes Dec 14 '11 at 12:26