A VS MPF bug, about how to write good virtual fucntion

MPF bug: creating the custom ProjectConfig class

 

Old design:

          MPF ConfigProvider.cs:

namespace Microsoft.VisualStudio.Package

{

public class ConfigProvider : IVsCfgProvider2, IVsProjectCfgProvider, IVsExtensibleObject

{

private Dictionary<string, ProjectConfig> configurationsList = new Dictionary<string, ProjectConfig>();

 

85:          protected virtual ProjectConfig CreateProjectConfiguration(string configName)

86:          {

87:                          // if we already created it, return the cached one

88:                          if (configurationsList.ContainsKey(configName))

89:                                          return configurationsList[configName];

90:         

91:                          ProjectConfig requestedConfiguration = new ProjectConfig(this.project, configName);

92:                          configurationsList.Add(configName, requestedConfiguration);

93:         

94:                          return requestedConfiguration;

95:          }

 

Now let’s imagine that developer wants to inherit ProjectConfig class with custom one (for example to customize its DebugLaunch logic) and he/she overrides CreateProjectConfiguration method to return custom MyProjectConfig. At the same time he/she wants to stay configuration caching logic.

 

But… It is imposible due to configurationsList has ‘private’ accessor in base class! :

 

protected override ProjectConfig CreateProjectConfiguration(string configName)

{

                // if we already created it, return the cached one

                if (configurationsList.ContainsKey(configName))

                {

                                return configurationsList[configName];

                }

 

                AsProjectConfig requestedConfiguration = new MyProjectConfig(ProjectMgr, configName);

                configurationsList.Add(configName, requestedConfiguration);

 

                return requestedConfiguration;

}

 

RESOLUTION:

 

Way 1: Change ‘private’ to at least ‘protected’ for :

 

private Dictionary<string, ProjectConfig> configurationsList = new Dictionary<string, ProjectConfig>();

 

It’s simple but it ‘smells bad’.

 

 

Way 2: Rename current ProjectConfig.CreateProjectConfiguration method to GetProjectConfiguration and extract the real CreateProjectConfiguration method which is really creates the ProjectConfig instance and doen’t perform any other operations.

 

85:          protected virtual ProjectConfig GetProjectConfiguration (string configName)

86:          {

87:                          // if we already created it, return the cached one

88:                          if (configurationsList.ContainsKey(configName))

89:                                          return configurationsList[configName];

90:         

91:                          ProjectConfig requestedConfiguration = CreateProjectConfiguration(this.project, configName);

92:                          configurationsList.Add(configName, requestedConfiguration);

93:         

94:                          return requestedConfiguration;

95:          }

96:         

97:          protected virtual ProjectConfig CreateProjectConfiguration(ProjectNode project, string configName)

98:          {

99:                          return new ProjectConfig(this.project, configName);

100:        }

 

That will be a good resolution!

 

P.S. also posted here - http://forums.microsoft.com/MSDN/ShowPost.aspx?PostID=1371036&SiteID=1&mode=1

 

 

Thanks

 

 

 ======Mail discussion:

 

I agree that the comments would have been better saying something like “get a configuration…” instead of “Create….

The pattern really is that you should only call the getter and not the Create.. method. The Create method is your abstract factory method that you should override to handle your own special Project Configuration types.

 

I will fix that the next time I make some changes to the project sources and thanks for the feedback.

 

Ole

 

From:
Sent: Thursday, April 10, 2008 12:42 AM

 

I think the fix is ok except for the comment.

 

The comment I prefer is something like: Get an existing Project Configuration according to the configName if configName already exists in configuration list, otherwise it returns a new one.

 

From: 
Sent: 2008年4月10日 14:13
To: Ole Preisler (VS SDK)

Hi Ole

        

         Sorry to trouble you.

         This bug: MPF bug: creating the custom ProjectConfig class is fixed by you. I have closed it.

         And I have a little doubts with the bug fix.

 

         Now the file ConfigProvider.cs has been changed to：

This comment seems need changed

/// <summary>

/// Creates new Project Configuartion objects based on the configuration name.

/// </summary>

/// <param name="configName">The name of the configuration</param>

/// <returns>An instance of a ProjectConfig object.</returns>

protected ProjectConfig GetProjectConfiguration(string configName)

{

      // if we already created it, return the cached one

      if (configurationsList.ContainsKey(configName))

      {

           return configurationsList[configName];

      }

 

      ProjectConfig requestedConfiguration = CreateProjectConfiguration(configName);

      configurationsList.Add(configName, requestedConfiguration);

 

      return requestedConfiguration;

}

 

protected virtual ProjectConfig CreateProjectConfiguration(string configName)

{

      return new ProjectConfig(this.project, configName);

}

      

     So if we override CreateprojectConfiguration, and creating a custom ProjectConfig class MyProjectConfig.

        

         protected override ProjectConfig CreateProjectConfiguration(string configName)

     {

      MyProjectConfig requestedConfiguration = new MyProjectConfig(ProjectMgr, configName);

      return requestedConfiguration;

     }

 

     If want to create and add it to configurationsList. We have to use GetProjectConfiguration

        

1.          

         ConfigProvider myConfigProvider = new ConfigProvider(myProjectNode);

     myConfigProvider.CreateProjectConfiguration(“myConfig”); // can’t add it to configurationsList

2.        

     ConfigProvider myConfigProvider = new ConfigProvider(myProjectNode);

     myConfigProvider.GetProjectConfiguration(“myConfig”);    // Ok to new and add it to configurationsList

    I think method 2 has some problems in logic. We use Get* method to new Configuration. But it seems that we have no better workaround.