Is using a static class to facilitate access to appsettings acceptable? [closed]

Question

I have some keys in the app.config file of my application that house email settings. The application I am currently developing for work has several places where I need to send off an email (mostly in error checking to let us know something bad happened). To make it easier to grab that setting where I need it (instead of going back to the app.config to look up the text string to use in ConfigurationManager.AppSettings["SomeKey"].ToString() I created a simple static class to house a few readonly properties that return the values I need. Here's the basic structure

internal static Class myClass
{
    internal static string setting1
    {
        get { return ConfigurationManager.AppSettings["SomeKey"].ToString(); }
    }
    internal static string setting2
    {
        get { return ConfigurationManager.AppSettings["SomeOtherKey"].ToString(); }
    }
}

Is the way I am doing this acceptable? I can't find an issue with this but can't help thinking I'm missing something.

Additional Info:

I am aware of the method of simply using the Settings.Settings file. I can't use the Settings.Settings file due to the code policy in place where I work, which would make the way I'm doing this unnecessary.

Edit: By "acceptable" I mean is it commonly done or considered a good idea? I realize the community at large cannot speculate on acceptability at my job.

Answer 1

It's not bad but it can be better with a small tweak. (Ok, maybe not real small.)

If your classes depend on a static class and that class depends on AppSettings then your classes are still coupled to AppSettings. In other words, there's no other way for them to get their settings. That's a challenge if you want to unit test your class. That means that your unit test project has to have the same <appSettings> section. But what if you want two tests that use two different values for a setting? It's impossible. Or what if you have a class that needs settings and in a few years you want to use it in an ASP.NET Core application and there is no web.config? Then it won't work at all.

To avoid that you can do this:

public interface IMySettings
{
    string Setting1 {get;}
    string Setting2 {get;}
}

public class MyConfigurationSettings : IMySettings
{
    public string Setting1
    {
        get { return ConfigurationManager.AppSettings["SomeKey"].ToString(); }
    }
    public string Setting2
    {
        get { return ConfigurationManager.AppSettings["SomeOtherKey"].ToString(); }
    }
}

Then, in the class that needs the setting:

public class ClassThatNeedsSettings
{
    private readonly IMySettings _settings;

    public ClassThatNeedsSettings(IMySettings settings)
    {
        _settings = settings;
    }
}

Then, when ever you create an instance of ClassThatNeedsSettings you pass an instance of a class that implements IMySettings, and the class will use that to retrieve settings. When your application is running you pass in MyConfigurationSettings so that your values come from AppSettings. But ClassThatNeedsSettings never knows that. It only knows that it's using an instance of IMySettings.

This is called "dependency injection." ClassThatNeedsSettings depends on IMySettings so you're "injecting" it into the constructor. That way ClassThatNeedsSettings receives what it needs. It's not responsible for creating it.

If you want to unit test, you can "mock" IMySettings. That is, you can create other classes that implement the interface and use them to pass in whatever values you want to test with. There are even tools like Moq that help you create those classes.

Typically if you use dependency injection you're also going to use a framework like Windsor, Unity, Autofac, or others to manage creating objects for you. It feels a little bit like bait-and-switch introducing that at the very end because it requires more learning and perhaps changing the way an application is configured. But this is why we use it, to prevent one class from having an absolute dependency on another which makes it less flexible and harder to test.

Answer 2

Here's an example of testable code that addresses the commenters' concerns:

interface IEmailSettings {
    string Server { get; }
    string Sender { get; }
    string[] Recipients { get; }
}

If you store your settings in the app.config, use this:

class AppConfigEmailSettings : IEmailSettings {
    public AppConfigEmailSettings() {
        this.Server = ConfigurationManager.AppSettings["server"];
        this.Sender = ConfigurationManager.AppSettings["sender"];
        this.Recipients = ConfigurationManager.AppSettings["recipients"].Split(';');
    }

    public string Server { get; private set; }
    public string Sender { get; private set; }
    public string[] Recipients { get; private set; }
}

If you store your settings in the database, use this:

class DatabaseEmailSettings : IEmailSettings {
    public DatabaseEmailSettings(string connectionString) {
        //code to connect to database and retrieve settings
    }
    //you can use the same fields and properties from AppConfigEmailSettings
}

For testing, you can use something like this:

class MockSettings : IEmailSettings {
    public string Server { get { return "localhost"; } }
    public string Sender { get { return "[email protected]" } }
    public string[] Recipients { get { return new string[] { "[email protected]" }; } }
}

You get the idea. This code is easier to test than yours. Also, if you inject IEmailSettings into the code that sends emails, you can easily change how you store your email settings by changing one line of code in your whole application. That's the line that instantiates the IEmailSettings object to AppConfigEmailSettings or DatabaseEmailSettings or something else.