FxCop 1.35 Overridable Methods In Constructors question

jspurlin posted on Monday, January 29, 2007

The FxCop rules web site states that this rule should not be excluded. I am curious what your take is on this rule.

I am using FxCop to help clean up and optimize my code, but I also pay close attention to the project tracker examples.

One example of this message is the following:

"'Role.Role(SafeDataReader)' 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: Role.Role(SafeDataReader)BusinessBase.MarkOld(
                   ):Void"

malloc1024 replied on Monday, January 29, 2007

You should not call a virtual method in a constructor because it is possible that the derived constructor wasn’t executed before the call.  If you set a variable in your derived constructor and your virtual method uses this variable, you will end up with a wrong result.

jspurlin replied on Monday, January 29, 2007

Yes. That is exactly what the website states, and I understand that.

ajj3085 replied on Tuesday, January 30, 2007

Then it seems it wouldn't be a good idea to ignore the message. 

jspurlin replied on Tuesday, January 30, 2007

Great! So here is the scenario.  I am using the design patterns used in Project Tracker (which I have used for myself).

Here is the Fetch Method for the Roles Collection:

private void DataPortal_Fetch(Criteria criteria)
    {
      RaiseListChangedEvents = false;
      using (SqlConnection cn = new SqlConnection(Database.PTrackerConnection))
      {
        cn.Open();
        using (SqlCommand cm = cn.CreateCommand())
        {
          cm.CommandType = CommandType.StoredProcedure;
          cm.CommandText = "getRoles";

          using (SafeDataReader dr = new SafeDataReader(cm.ExecuteReader()))
            while (dr.Read())
              this.Add(Role.GetRole(dr));
        }
      }
      RaiseListChangedEvents = true;
    }

It is an Instance Method which sends the datareader to the Role Class.  In the Role Class the datareader is used to build a new role while the datareader is reading and pass each new role back to the collection:

//You cannot initialize properties here: this is a static method
//and Instance variables are not directly accessible.
internal static Role
      GetRole(Csla.Data.SafeDataReader dr)
    {
      return new Role(dr);
    }

//You can initialize instance variables here.  But this is a constructor, too.
private Role(Csla.Data.SafeDataReader dr)
    {
      MarkAsChild(); 
      _id = dr.GetInt32("id");
      _idSet = true;
      _name = dr.GetString("name");
      dr.GetBytes("lastChanged", 0, _timestamp, 0, 8);
      MarkOld(); //virtual method called by the base class.
    }

FxCop 1.35 states:

"'Role.Role(SafeDataReader)' 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: Role.Role(SafeDataReader)BusinessBase.MarkOld(
                   ):Void"

What would the recommended work-around be for this?

jspurlin replied on Tuesday, January 30, 2007

Here is what I can do to avoid this error, but I haven't tested it yet and would like suggestions if you have any:

internal static Role
      GetRole(Csla.Data.SafeDataReader dr)
    {
      //return new Role(dr);
      Role _role = new Role(dr);
      _role.MarkOld();
      return _role;
    }

private Role(Csla.Data.SafeDataReader dr)
    {
      MarkAsChild();
      _id = dr.GetInt32("id");
      _idSet = true;
      _name = dr.GetString("name");
      dr.GetBytes("lastChanged", 0, _timestamp, 0, 8);
      //MarkOld();
    }

ajj3085 replied on Tuesday, January 30, 2007

Hi,

This is the pattern I use, and it doesn't get that warning.  Another advantage of this method is that the load logic is outside the constructor completely, which means I can reuse it should the need arise.

public sealed class MyChild : BB<MyChild> {

    internal static MyChild GetRole( IDataReader dr ) {
          MyChild result;

          result = new MyChild();
          result.LoadSelf( dr );

          return result;
    }

    private void LoadSelf( IDataReader dr ) {
       // Load here
       MarkOld();
     }
}

jspurlin replied on Tuesday, January 30, 2007

Thank you, ajj.  That's the elegance I was looking for. May I ask how you handle this with a editable collection of roles using the DataPortal_Fetch(Criteria criteria) method?

ajj3085 replied on Tuesday, January 30, 2007

The MyChildCollection class implements the same pattern, except that it loops through the data reader... something like this:

while( dr.Read() ) {
    Add( MyChild.Get( dr ) );
}

That would be in a LoadSelf method on the collection.

HTH
Andy