CODEWITHSUNDEEP
c#design-patterns
jitendragarg
jitendragarg

Reputation: 957

How to improve class object initialization in C#?

I have a class in an existing app that is initialized every time an event is fired, using XMLConfigManager objXMLConfig = new XMLConfigManager();.

Obviously it makes no sense to do so, and there are better ways to have a static class. But the same class reads the system registry to find the appropriate database to access. Below is the code from the constructor of the class.

    public XMLConfigManager()
    {
        RegistryKey objRKConfigGroup = Registry.LocalMachine.OpenSubKey("Software\\......");
        strFilePath = (string)objRKConfigGroup.GetValue("XMLFilePath");
        objRKConfigGroup.Close();
        string strApplicationName = (string)CacheHandler.GetCache().GetCachedElement("APPLICATION");
        this.ApplicationName = strApplicationName;
    }

I am trying to find ways to improve this, so that we don't go to registry all the time. Instead we can refresh the IIS cache to update the registry value if it ever changes (maybe once in a year, if database IP changes, or new version is released).

Now the question is:

Is static class a good choice? Or should I use singleton? Or is there any other better option I have missed?

Secondly, will it require me to change the XMLConfigManager objXMLConfig = new XMLConfigManager(); line in every place (about 2k+ event handlers in total).

P.S. I am considering the fact that reducing registry access events should be faster. I hope I am right about the idea. Also, if the question belongs to a better stack exchange section, do let me know and I will move it there.

Edit:

More details: The constructor is trying to read just a single registry key. The registry key stores the default file path (saved during application installation), so that it can find the actual configuration file ConfigData.xml.

The remaining code in this class is to read the ConfigData.xml file for different xml elements. An example function:

        public string GetConfigItem( string strConfigGroupID, string strConfigItemID, out bool bIsCacheable )   {

        string  strValue;           //string which holds the Configuration item id

        ///- Get the XML Dom document which contains configuration data
        XmlDocument objXmlDocument  = OpenXML();

        ///- Get the Configuration Group Node by using XPath syntax.            
        XmlNode objXmlAppNode           = objXmlDocument.DocumentElement.SelectSingleNode( "Nsp:Application[@Name='" + this.strAppName + "']", xmlNsp );
        XmlNode objXmlConfigGroupNode   = objXmlAppNode.SelectSingleNode( ".//Nsp:ConfigGroup[@ID='" + strConfigGroupID + "']", xmlNsp );

        ///- Get the Configuration Item Node for the specified Group by using XPath syntax.
        XmlNode objXmlConfigItemNode    = objXmlAppNode.SelectSingleNode( ".//Nsp:ConfigGroup[@ID='" + strConfigGroupID + "']/ConfigItem[@ID='" + strConfigItemID + "']", xmlNsp );

        ///- Read the value attribute from Configuration Item node.
        strValue    = objXmlConfigItemNode.Attributes[ "Value" ].Value;

        ///- get "IsCacheable" value of configuration group, whether config data can be cacheable or not.
        bIsCacheable    = Convert.ToBoolean( objXmlConfigGroupNode.Attributes[ "IsCacheable" ].Value );

        ///- Return the Configuration Item value.
        return  strValue;
    }

The other classes use the following code to access this class.

XMLConfigManager objConfig = new XMLConfigManager();
String prodDBConnectionString = objConfig.GetConfigItem ("ConnectionStrings", "ProductionDB", isCacheable);

The second line in above code changes parameters based on what the application need to read (temp folder path, connection strings, report paths, email server settings, etc).

I want to do something that simply replaces this class as is, without requiring much changes in all the event handlers this class is called from. I was initially planning to go the route of singleton. But, any better solution is appreciated (even replacing the whole requirement of registry).

Upvotes: 1

Views: 74

Answers (2)

Moerwald
Moerwald

Reputation: 11254

Is static class a good choice?

Static classes have the disadvantage that they'll add dependencies to your calling class that make it more complex to test your code via unit tests.

Or should I use singleton? Or is there any other better option I have missed?

A singleton might be ok if consider following things:

  • there is not single class in your codebase that can be the owner of the class
  • you need lazy initialisation
  • there is no alternative way to provide global access to the singleton

So using a singleton might be ok for your case. I would create a IRegistrationCache. The code behind takes care about caching, so that the registry key is only read one time and value is stored in the object. Then I would extend the XMLConfigManager with an additional IRegistrationCache CTOR parameter -> makes your class more testable. In this case you've to extend your 2k+ CTOR calls with an addtional call to singleton and add it via the CTOR. To get rid of that I would create an factory object returning an IRegistrationCache. This step hides additional initialisation logic in the factory class -> if you extend your CTOR you only have to extend it in one place not in 2k+ places. The different classes implementing the events need to be extended to receive the factory object (which should be hided behind an interface).

Upvotes: 1

Sufyan Jabr
Sufyan Jabr

Reputation: 809

From what i understood, every object you create might be different according to the appropriate database to access.

what i recommend is to move the part which get's the database path outside, create a static global dictionary and use "strApplicationName " as a key, and pass it as a parameter to the constructor.

your code will be as below:

define dictionary:

static Dictionary<string, XMLConfigManager> dic= new Dictionary<string, XMLConfigManager>();

Get the database path, and pass it to the object:

  RegistryKey objRKConfigGroup = Registry.LocalMachine.OpenSubKey("Software\\......");
    strFilePath = (string)objRKConfigGroup.GetValue("XMLFilePath");

Load existing or create new

XMLConfigManager tmp;
 if(dic.ContainsKey(strFilePath)){
 tmp= dic[strFilePath];
}
else{
XMLConfigManager objXMLConfig = new XMLConfigManager(strFilePath);
dic.add(objXMLConfig );
tmp = objXMLConfig;
}

Upvotes: 1

Related Questions