CA2214 Do not call overridable methods in constructors
-
The way I tend to code this type of code is to have an instance method call the override versions, like this:
public abstract class MyBase
{
public MyBase()
{
Initialize();
}private void Initialize()
{
OnInitialize();
}protected virtual void OnInitialize()
{
}
}Why do I do this you may ask? Well, I tend to have a separation of design time code, and runtime code, and this allows me to easily separate things out.
*pre-emptive celebratory nipple tassle jiggle* - Sean Ewington
"Mind bleach! Send me mind bleach!" - Nagy Vilmos
CodeStash - Online Snippet Management | My blog | MoXAML PowerToys | Mole 2010 - debugging made easier
-
Maybe I just need sleep to recover from recent festivities - but I can't think of a way to fix this code to comply with the FX Cop warning. The only thing that comes to mind is to create protected setters (or setter methods) in the base class and call those from the derived constructors...
public abstract class BaseClass
{
private int id;// CA2214 Do not call overridable methods in constructors protected BaseClass() { Initialize(out id); // Initialize derived class with it's own specific data and get common data back here for initialization } public int Id { get { return id; } } protected abstract void Initialize(out int id);}
public class ConcreteA : BaseClass
{
public ConcreteA()
: base()
{ }protected override void Initialize(out int id) { ConcreteAIntializationData data = NativeMethods.ConcreteAInitialize(); id = data.Id; }}
public class ConcreteB : BaseClass
{
private string name;public ConcreteB() : base() { } protected override void Initialize(out int id) { ConcreteBIntializationData data = NativeMethods.ConcreteBInitialize(); id = data.Id; name = data.name; }}
public static class NativeMethods
{
// In the real code this is an external C++ dll call
public static ConcreteAIntializationData ConcreteAInitialize()
{
return new ConcreteAIntializationData();
}
// In the real code this is an external C++ dll call
public static ConcreteBIntializationData ConcreteBInitialize()
{
return new ConcreteBIntializationData();
}
}
// In the real code this is a C++ structure
public class ConcreteAIntializationData
{
private int id;// In the real code this struct is filled by unmanaged code. public ConcreteAIntializationData() { id = 1; } public int Id { get { return id; } }}
public class ConcreteBIntializationData
{
private int id;
public string name;// In the real code this struct is filled by unmanaged code. public ConcreteBIntializationData() { id = 2; name = "Name"; } public int Id { get { return id; } } public string Name { get { return name; } }}
Dave
Binging is like googling, it just feels dirtier. Please take your VB.The reason this is a warning is because you can get yourself into some really nasty messes by doing this, if you don't understand it. The virtual method that you override in the subclasses is run with an incompletely initialised object – in particular its own constructor has not yet run so you don't have access to any instance variables, even those set in a constructor or in field initialisers (as they're compiled into the constructor too). You've obviously simplified your example a lot and in this case you can provide a protected setter for Id and create the specific data objects in the subclass constructors. I'd say that's a better approach if it's possible.
-
Does that dodge the warning? If so, that's pretty rubbish on FXCop's part since that's 'bad' for exactly the same reason.
Yes it does, and there's nothing wrong with doing this. The problem with FxCop is that it's very arbitrary with its rules, many of which are excessively anal.
*pre-emptive celebratory nipple tassle jiggle* - Sean Ewington
"Mind bleach! Send me mind bleach!" - Nagy Vilmos
CodeStash - Online Snippet Management | My blog | MoXAML PowerToys | Mole 2010 - debugging made easier
-
Yes it does, and there's nothing wrong with doing this. The problem with FxCop is that it's very arbitrary with its rules, many of which are excessively anal.
*pre-emptive celebratory nipple tassle jiggle* - Sean Ewington
"Mind bleach! Send me mind bleach!" - Nagy Vilmos
CodeStash - Online Snippet Management | My blog | MoXAML PowerToys | Mole 2010 - debugging made easier
I'll check this later - I'm pretty sure on VS2012 when I tried this yesterday before posting it just added something into the warning about a call chain or similar but still CA2214
Dave
Binging is like googling, it just feels dirtier. Please take your VB.NET out of our nice case sensitive forum. Astonish us. Be exceptional. (Pete O'Hanlon)
BTW, in software, hope and pray is not a viable strategy. (Luc Pattyn) -
Cheers Alan!
Alan N wrote:
don't do anything naughty
:laugh: The overridden method is purely setting instance fields unique to that class to their initial values (these are not changed anywhere after instanciation - that part of the derived classes is immutable) and returning initialization values that are common to all derived classes to the base's constructor. It seems like I should be OK to suppress the warning in this scenario.
Dave
Binging is like googling, it just feels dirtier. Please take your VB.NET out of our nice case sensitive forum. Astonish us. Be exceptional. (Pete O'Hanlon)
BTW, in software, hope and pray is not a viable strategy. (Luc Pattyn)DaveyM69 wrote:
The overridden method is purely setting instance fields unique to that class to their initial values (these are not changed anywhere after instanciation - that part of the derived classes is immutable) and returning initialization values that are common to all derived classes to the base's constructor.
As I read that... Yes that can lead to problems. And that is what the constructor is supposed to do - iniatialize the class variables. Also why would the base ctor care about child initialization values when all the base ctor should be doing is initializing the base ctor?
-
DaveyM69 wrote:
The overridden method is purely setting instance fields unique to that class to their initial values (these are not changed anywhere after instanciation - that part of the derived classes is immutable) and returning initialization values that are common to all derived classes to the base's constructor.
As I read that... Yes that can lead to problems. And that is what the constructor is supposed to do - iniatialize the class variables. Also why would the base ctor care about child initialization values when all the base ctor should be doing is initializing the base ctor?
jschell wrote:
why would the base ctor care about child initialization values when all the base ctor should be doing is initializing the base ctor
The child needs to self initialize with values retrieved from an unmanaged function call (the function is different for each child type) - then pass the common values to the base so it can initialize itself. What I was trying to do was force a method to be called when the base constructor runs that 1. calls the unmanaged function (hence it needing to be in called from the child as only the child should know what unmanaged function to call, and therefore needs to be an abstract method) 2. the child initializes it's unique fields 3. passes the common values back into the base constructor (hence the out parameters) Perhaps this code will make it clearer...
public abstract class Base
{
private int commonValue1;
private int commonValue2;
private int id;protected Base(int id) { this.id = id; Initialize(out commonValue1, out commonValue2); // CA2214 } public int CommonValue1 { get { return commonValue1; } } public int CommonValue2 { get { return commonValue2; } } public int Id { get { return id; } } protected abstract void Initialize(out int commonValue1, out int commonValue2);}
public class ChildA : Base
{
public ChildA(int id)
: base(id)
{ }protected override void Initialize(out int commonValue1, out int commonValue2) { DataA data; NativeMethods.UnmanagedFunctionGetChildAData(Id, out data); commonValue1 = data.Value1; commonValue2 = data.Value2; }}
public class ChildB : Base
{
private int childBValue;public ChildB(int id) : base(id) { } public int ChildBValue { get { return childBValue; } } protected override void Initialize(out int commonValue1, out int commonValue2) { DataB data; NativeMethods.UnmanagedFunctionGetChildBData(Id, out data); commonValue1 = data.Value1; commonValue2 = data.Value2; childBValue = data.Value3; }}
public class ChildC : Base
{
private int childCValue1;
private int childCValue2;public ChildC(int id) : base(id) { } public int ChildCValue1 { get { return childCValue1; } } public int ChildCValue2 { get { ret -
Yes it does, and there's nothing wrong with doing this. The problem with FxCop is that it's very arbitrary with its rules, many of which are excessively anal.
*pre-emptive celebratory nipple tassle jiggle* - Sean Ewington
"Mind bleach! Send me mind bleach!" - Nagy Vilmos
CodeStash - Online Snippet Management | My blog | MoXAML PowerToys | Mole 2010 - debugging made easier
In VS2012 you still get the warning
contains a call chain that results in a call to a virtual method defined by the class
Dave
Binging is like googling, it just feels dirtier. Please take your VB.NET out of our nice case sensitive forum. Astonish us. Be exceptional. (Pete O'Hanlon)
BTW, in software, hope and pray is not a viable strategy. (Luc Pattyn) -
The reason this is a warning is because you can get yourself into some really nasty messes by doing this, if you don't understand it. The virtual method that you override in the subclasses is run with an incompletely initialised object – in particular its own constructor has not yet run so you don't have access to any instance variables, even those set in a constructor or in field initialisers (as they're compiled into the constructor too). You've obviously simplified your example a lot and in this case you can provide a protected setter for Id and create the specific data objects in the subclass constructors. I'd say that's a better approach if it's possible.
Yeah, that's quite possible and may be what I end up doing. I'd like to mark the relevant fields (at least in the base) read only ideally which is primarily why I'm trying to manipulate from the constructor, along with the fact that this is all to do with construction of the objects so it is the logical place!
Dave
Binging is like googling, it just feels dirtier. Please take your VB.NET out of our nice case sensitive forum. Astonish us. Be exceptional. (Pete O'Hanlon)
BTW, in software, hope and pray is not a viable strategy. (Luc Pattyn) -
jschell wrote:
why would the base ctor care about child initialization values when all the base ctor should be doing is initializing the base ctor
The child needs to self initialize with values retrieved from an unmanaged function call (the function is different for each child type) - then pass the common values to the base so it can initialize itself. What I was trying to do was force a method to be called when the base constructor runs that 1. calls the unmanaged function (hence it needing to be in called from the child as only the child should know what unmanaged function to call, and therefore needs to be an abstract method) 2. the child initializes it's unique fields 3. passes the common values back into the base constructor (hence the out parameters) Perhaps this code will make it clearer...
public abstract class Base
{
private int commonValue1;
private int commonValue2;
private int id;protected Base(int id) { this.id = id; Initialize(out commonValue1, out commonValue2); // CA2214 } public int CommonValue1 { get { return commonValue1; } } public int CommonValue2 { get { return commonValue2; } } public int Id { get { return id; } } protected abstract void Initialize(out int commonValue1, out int commonValue2);}
public class ChildA : Base
{
public ChildA(int id)
: base(id)
{ }protected override void Initialize(out int commonValue1, out int commonValue2) { DataA data; NativeMethods.UnmanagedFunctionGetChildAData(Id, out data); commonValue1 = data.Value1; commonValue2 = data.Value2; }}
public class ChildB : Base
{
private int childBValue;public ChildB(int id) : base(id) { } public int ChildBValue { get { return childBValue; } } protected override void Initialize(out int commonValue1, out int commonValue2) { DataB data; NativeMethods.UnmanagedFunctionGetChildBData(Id, out data); commonValue1 = data.Value1; commonValue2 = data.Value2; childBValue = data.Value3; }}
public class ChildC : Base
{
private int childCValue1;
private int childCValue2;public ChildC(int id) : base(id) { } public int ChildCValue1 { get { return childCValue1; } } public int ChildCValue2 { get { retI use a similar pattern, but instead of private member variables in the abstract base class and passing them as out parameters, I use protected member variables and call a parameterless function (the derived class may access them in other overridable functions). Visual Studio 2010 gives that CA2214 warning, too. I feel comfortable with this pattern, and ignore the warning.
-
jschell wrote:
why would the base ctor care about child initialization values when all the base ctor should be doing is initializing the base ctor
The child needs to self initialize with values retrieved from an unmanaged function call (the function is different for each child type) - then pass the common values to the base so it can initialize itself. What I was trying to do was force a method to be called when the base constructor runs that 1. calls the unmanaged function (hence it needing to be in called from the child as only the child should know what unmanaged function to call, and therefore needs to be an abstract method) 2. the child initializes it's unique fields 3. passes the common values back into the base constructor (hence the out parameters) Perhaps this code will make it clearer...
public abstract class Base
{
private int commonValue1;
private int commonValue2;
private int id;protected Base(int id) { this.id = id; Initialize(out commonValue1, out commonValue2); // CA2214 } public int CommonValue1 { get { return commonValue1; } } public int CommonValue2 { get { return commonValue2; } } public int Id { get { return id; } } protected abstract void Initialize(out int commonValue1, out int commonValue2);}
public class ChildA : Base
{
public ChildA(int id)
: base(id)
{ }protected override void Initialize(out int commonValue1, out int commonValue2) { DataA data; NativeMethods.UnmanagedFunctionGetChildAData(Id, out data); commonValue1 = data.Value1; commonValue2 = data.Value2; }}
public class ChildB : Base
{
private int childBValue;public ChildB(int id) : base(id) { } public int ChildBValue { get { return childBValue; } } protected override void Initialize(out int commonValue1, out int commonValue2) { DataB data; NativeMethods.UnmanagedFunctionGetChildBData(Id, out data); commonValue1 = data.Value1; commonValue2 = data.Value2; childBValue = data.Value3; }}
public class ChildC : Base
{
private int childCValue1;
private int childCValue2;public ChildC(int id) : base(id) { } public int ChildCValue1 { get { return childCValue1; } } public int ChildCValue2 { get { ret -
DaveyM69 wrote:
Perhaps this code will make it clearer...
And what do you think is the problem with making the based members protected? Or providing a protected setter for those members?
jschell wrote:
based members protected
Non private fields are generally not considered a great idea.
jschell wrote:
protected setter for those members
No problem with that, but it implies to inheriting classes that the fields that are protected by the property can be changed after initialization which is not the case - easy to protect against in code and so long as I document this well then that's OK.
Dave
Binging is like googling, it just feels dirtier. Please take your VB.NET out of our nice case sensitive forum. Astonish us. Be exceptional. (Pete O'Hanlon)
BTW, in software, hope and pray is not a viable strategy. (Luc Pattyn) -
jschell wrote:
based members protected
Non private fields are generally not considered a great idea.
jschell wrote:
protected setter for those members
No problem with that, but it implies to inheriting classes that the fields that are protected by the property can be changed after initialization which is not the case - easy to protect against in code and so long as I document this well then that's OK.
Dave
Binging is like googling, it just feels dirtier. Please take your VB.NET out of our nice case sensitive forum. Astonish us. Be exceptional. (Pete O'Hanlon)
BTW, in software, hope and pray is not a viable strategy. (Luc Pattyn)DaveyM69 wrote:
Non private fields are generally not considered a great idea.
But calling child methods in a ctor is?
DaveyM69 wrote:
it implies to inheriting classes that the fields that are protected by the property can be changed after initialization
A child can misuse the contract with a parent in any number of ways. There is no way to prevent that.