4
\$\begingroup\$

I'm developing an app which stores some different user settings in app.config such as connection strings, paths for saving reports, default printer e.t.c For retrieving them I made a static class StoredSettingsTools which contains an Enum of settings types and a public method which calls (depending of setting type) some private method and get the settings from app.config:

public static class StoredSettingsTools
{

    public enum StoredSettingsType
    {
        DBConnection,
        Printer,
        ReportsFolder
    };

    private static ConnectionStringsSection GetConfigurationConnectionStringsSection()
    {
        ExeConfigurationFileMap configMap = new ExeConfigurationFileMap();
        Configuration config = ConfigurationManager.OpenExeConfiguration(System.Reflection.Assembly.GetEntryAssembly().Location);
        ConnectionStringsSection conectionSettings = (ConnectionStringsSection)config.SectionGroups.Get("UserSettingsGroup")
            .SectionGroups.Get("StoredConnectionSettings")
            .Sections.Get("defaultConnectionString");        
        return conectionSettings;
    }

    private static Hashtable GetConfigurationPrinterSection()
    {
        ExeConfigurationFileMap configMap = new ExeConfigurationFileMap();
        Configuration config = ConfigurationManager.OpenExeConfiguration(System.Reflection.Assembly.GetEntryAssembly().Location);
        ConfigurationSectionGroup userSettingsGroup = config.SectionGroups["UserSettingsGroup"];
        Hashtable sectionSettings = ConfigurationManager.GetSection(userSettingsGroup.Sections.Get("printerSettings").SectionInformation.SectionName) as Hashtable;
        return sectionSettings;
    }

    private static Hashtable GetConfigurationReportsFolderSection()
    {
        ExeConfigurationFileMap configMap = new ExeConfigurationFileMap();
        Configuration config = ConfigurationManager.OpenExeConfiguration(System.Reflection.Assembly.GetEntryAssembly().Location);
        ConfigurationSectionGroup userSettingsGroup = config.SectionGroups["UserSettingsGroup"];
        Hashtable sectionSettings = ConfigurationManager.GetSection(userSettingsGroup.Sections.Get("reportsFolderSettings").SectionInformation.SectionName) as Hashtable;
        return sectionSettings;
    }

    public static Hashtable GetStoredSettings(StoredSettingsType sectionType)
    {
        Hashtable table = new Hashtable();
        switch (sectionType)
        {
            case StoredSettingsType.DBConnection:
                {
                    ConfigurationSection section = GetConfigurationConnectionStringsSection();
                    ConnectionStringSettingsCollection col = ((ConnectionStringsSection)section).ConnectionStrings;

                    foreach (ConnectionStringSettings settings in col)
                    {
                        table.Add(settings.Name, settings.ConnectionString);
                    }

                    return table;
                }
            case StoredSettingsType.Printer:
                {
                    table = GetConfigurationPrinterSection();
                    return table;
                }

            case StoredSettingsType.ReportsFolder:
                {
                    table = GetConfigurationReportsFolderSection();
                    return table;
                }

        }
        return null;
    }
}

Is anything wrong with this code? Could it be more object-oriented?

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review. I hope you get some great answers! \$\endgroup\$ – Phrancis Mar 11 '16 at 4:01
2
\$\begingroup\$
  • You don't use ExeConfigurationFileMap configMap in any of your methods just get rid of it. If you do copy&pasta you should always check if some cleaning needs to be done.

  • You are calling this

    Configuration config = ConfigurationManager.OpenExeConfiguration(System.Reflection.Assembly.GetEntryAssembly().Location);
    ConfigurationSectionGroup userSettingsGroup = config.SectionGroups["UserSettingsGroup"];  
    

    in every GetXXX method. Extract it to a separate method.

  • Something like

    table = GetConfigurationPrinterSection();
    return table;  
    

    doesn't buy you anything. Just return directly which makes it possible to reduce the scope of table to only the first case.

  • The GetConfigurationConnectionStringsSection() method already returns a ConnectionStringsSection so there is no need to read it as ConfigurationSection only to cast it to a ConnectionStringsSection. Changing this would also make the ConnectionStringSettingsCollection col superfluous.

  • To make your life a little bit easier you could use the var type at places where it is obvious from the right hand side of an assignment which type is meant.

  • The lines where you read the e.g printerSettings are a little bit long.

If we would change the GetXXX methods like mentioned we would get

private static ConfigurationSectionGroup FetchConfigurationSectionGroup(string groupName)
{
    Configuration config = ConfigurationManager.OpenExeConfiguration(System.Reflection.Assembly.GetEntryAssembly().Location);
    return config.SectionGroups[groupName];
}  
private static string FetchSectionName(ConfigurationSectionGroup group, string sectionName)
{
    return group.Sections.Get(sectionName).SectionInformation.SectionName;
}  
private static Hashtable GetConfigurationPrinterSection()
{
    ConfigurationSectionGroup userSettingsGroup = FetchConfigurationSectionGroup("UserSettingsGroup");
    Hashtable sectionSettings = ConfigurationManager.GetSection(FetchSectionName(userSettingsGroup,"printerSettings")) as Hashtable;
    return sectionSettings;
}

private static Hashtable GetConfigurationReportsFolderSection()
{
    ConfigurationSectionGroup userSettingsGroup = FetchConfigurationSectionGroup("UserSettingsGroup");
    Hashtable sectionSettings = ConfigurationManager.GetSection(FetchSectionName(userSettingsGroup, "reportsFolderSettings")) as Hashtable;
    return sectionSettings;
}  

but as you see we still have some code duplication here, so a method FetchSetting(string groupName, string sectionName) would help us to remove that duplication like so

private static Hashtable FetchSetting(string groupName, string sectionName)
{
    ConfigurationSectionGroup userSettingsGroup = FetchConfigurationSectionGroup(groupName);
    return ConfigurationManager.GetSection(FetchSectionName(userSettingsGroup, sectionName)) as Hashtable;
}
private static ConfigurationSectionGroup FetchConfigurationSectionGroup(string groupName)
{
    Configuration config = ConfigurationManager.OpenExeConfiguration(System.Reflection.Assembly.GetEntryAssembly().Location);
    return config.SectionGroups[groupName];
}
private static string FetchSectionName(ConfigurationSectionGroup sectionGroup, string sectionName)
{
    return sectionGroup.Sections.Get(sectionName).SectionInformation.SectionName;
}
private static Hashtable GetConfigurationPrinterSection()
{
    return FetchSetting("UserSettingsGroup", "printerSettings");
}

private static Hashtable GetConfigurationReportsFolderSection()
{
    return FetchSetting("UserSettingsGroup", "reportsFolderSettings");
}  

which looks quite nice and tidy.

But wait, what about the ConnectionStringsSection ? If we remove everything which isn't necessary we will end with

private static ConnectionStringsSection GetConfigurationConnectionStringsSection()
{
    return (ConnectionStringsSection)FetchConfigurationSectionGroup("UserSettingsGroup")
        .SectionGroups.Get("StoredConnectionSettings")
        .Sections.Get("defaultConnectionString");
}  

but because the HashTable table will only be used for that value we should generate the HashTable there which will lead to

private static Hashtable GetConfigurationConnectionStringsSection()
{
    var section= (ConnectionStringsSection)FetchConfigurationSectionGroup("UserSettingsGroup")
        .SectionGroups.Get("StoredConnectionSettings")
        .Sections.Get("defaultConnectionString");

    Hashtable table = new Hashtable();
    ConnectionStringSettingsCollection col = section.ConnectionStrings;

    foreach (ConnectionStringSettings settings in section.ConnectionStrings)
    {
        table.Add(settings.Name, settings.ConnectionString);
    }

    return table;
}  

leaving the former GetStoredSettings() like this

public static Hashtable GetStoredSettings(StoredSettingsType sectionType)
{

    switch (sectionType)
    {
        case StoredSettingsType.DBConnection:
            {
                return GetConfigurationConnectionStringsSection();
            }

        case StoredSettingsType.Printer:
            {
                return GetConfigurationPrinterSection();
            }

        case StoredSettingsType.ReportsFolder:
            {
                return GetConfigurationReportsFolderSection(); ;
            }
        default:
            {
                return null;
            }
    }
}  

which looks strange with the given name, so let us rename the GetXXX methods which will lead to this

private static Hashtable FetchConnectionStringsSetting()
{
    var section= (ConnectionStringsSection)FetchConfigurationSectionGroup("UserSettingsGroup")
        .SectionGroups.Get("StoredConnectionSettings")
        .Sections.Get("defaultConnectionString");

    Hashtable table = new Hashtable();
    ConnectionStringSettingsCollection col = section.ConnectionStrings;

    foreach (ConnectionStringSettings settings in section.ConnectionStrings)
    {
        table.Add(settings.Name, settings.ConnectionString);
    }

    return table;
}
private static Hashtable FetchPrinterSetting()
{
    return FetchSetting("UserSettingsGroup", "printerSettings");
}

private static Hashtable FetchReportsFolderSetting()
{
    return FetchSetting("UserSettingsGroup", "reportsFolderSettings");
}
private static Hashtable FetchSetting(string groupName, string sectionName)
{
    ConfigurationSectionGroup userSettingsGroup = FetchConfigurationSectionGroup(groupName);
    return ConfigurationManager.GetSection(FetchSectionName(userSettingsGroup, sectionName)) as Hashtable;
}
private static ConfigurationSectionGroup FetchConfigurationSectionGroup(string groupName)
{
    Configuration config = ConfigurationManager.OpenExeConfiguration(System.Reflection.Assembly.GetEntryAssembly().Location);
    return config.SectionGroups[groupName];
}
private static string FetchSectionName(ConfigurationSectionGroup group, string sectionName)
{
    return group.Sections.Get(sectionName).SectionInformation.SectionName;
}
public static Hashtable GetStoredSettings(StoredSettingsType sectionType)
{

    switch (sectionType)
    {
        case StoredSettingsType.DBConnection:
            {
                return FetchConnectionStringsSetting();
            }

        case StoredSettingsType.Printer:
            {
                return FetchPrinterSetting();
            }

        case StoredSettingsType.ReportsFolder:
            {
                return FetchReportsFolderSetting(); ;
            }
        default:
            {
                return null;
            }
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.