Configuration class in C#, reading an INI file

Question

This class will load a file with INI configuration elements and cache them, it then has a method to get an item from the cache if it needs to.

internal class ConfigHandler
{
    private static readonly ILogger Logger = LogManager.GetCurrentClassLogger();

    private Dictionary<string, string> _configItems;

    public ConfigHandler()
    {
        _configItems = new Dictionary<string, string>();
        LoadConfig("resources/config/pitaya.ini");
    }

    internal void LoadConfig(string configFile)
    {
        try
        {
            if (!File.Exists(configFile))
            {
                Logger.Error("Your config file couldn't be found.");

                Console.ReadKey(true);
                Environment.Exit(0);
            }

            if (CoreUtilities.IsFileLocked(new FileInfo(configFile)))
            {
                Logger.Error("Your config file is already open somewhere.");

                Console.ReadKey(true);
                Environment.Exit(0);
            }

            _configItems = File.ReadLines(configFile).Where(IsConfigurationLine).Select(line => line.Split('=')).ToDictionary(line => line[0], line => line[1]);
        }
        catch (Exception exception)
        {
            Logger.Fatal(exception, "Error while loading config.");

            Console.ReadKey(true);
            Environment.Exit(0);
        }
    }

    private static bool IsConfigurationLine(string line)
    {
        return !line.StartsWith("#") && line.Contains("=");
    }

    internal string GetConfigValueByKey(string key)
    {
        if (!_configItems.TryGetValue(key, out var value))
        {
            Logger.Warn("Missing configuration key `" + key + "`");
        }

        return value;
    }
}

Answer 1

Since you didn't specify a particular aspect to code review for, I would review the code and comment and whatever I see fit.

Inconsistency seems to be a repeating issue in your code - inconsistent code confuse people.

In particular:

Why do you use Pascal casing for one field and then use _ prefix for another?

private static readonly ILogger Logger = LogManager.GetCurrentClassLogger();
private Dictionary<string, string> _configItems;

Your code tricked me to think Logger is a property, and turn out it is a field.

Why do you choose to have a public constructor for an internal class? It won't be available across assembly boundary anyway.

internal class ConfigHandler
{
    // ...
    public ConfigHandler()
    {

Why do you use Logger to encapsulate the Console for output and then directly do Console.ReadKey() later?

        Logger.Fatal(exception, "Error while loading config.");
        Console.ReadKey(true);
        Environment.Exit(0);

Coupling seems to be yet another repeating issue in your code - it limits the reusability.

Your code made a lot of assumptions on how it is going to be used.

For example:

You seems to believe that's the code will only be executed in a context where you do have a Console? What if you don't? Like when it is ported to become a service, or a GUI application?

Environment.Exit() is another. Why don't you let your caller to choose what to do when the file is invalid/not found?

The path is yet another. What if it is used in another environment without that particular path?

The general principle is as follow. The less policy you put in your code, the more reusable it would be. Of course, more effort on the caller side to specify them. For a utility class like this, it would worth the effort to make it reusable.

Answer 2

General

• This class can't be reused because the path to the ini file is hardcoded. Having a parameter string path and using it to load the configuration would be a better way.

LoadConfig()

• I don't like the Console.ReadKey(bool) and I don't see why the method should be able to terminate the process. In addition passing 0 to the Exit() method indicates that the process completed successfully which seems wrong.

• The line where you assign the key/values to the dictionary is too long and therefor hard to read.

• IMO the caller of the code should do the logging and you should let the exceptions bubble up the line.

• Checking if the file is locked will pretence that it is safe to read the file, but it could happen that just after the check the file is opened and locked.

GetConfigValueByKey()

• It seems strange that a not found key only will be logged. A better way would be to just use the bool TryGetXxx style.

---

How I would do this:

• I would add an overloaded public constructor which takes a string fileName parameter. I would then make the parameterless constructor internal and let it call the public constructor with the "resources/config/pitaya.ini" to simulate the former functionality.

• I would validate the fileName parameter and would throw the expected exceptions.

• I would just initialize the dictionary in the constructor and if an exception occurs I would let it bubble up.

Like so

public class ConfigHandler
{
    private readonly Dictionary<string, string> _configItems = new Dictionary<string, string>();

    internal ConfigHandler() 
        : this("resources/config/pitaya.ini")
    { }

    public ConfigHandler(string fileName)
    {
        if (fileName == null)
        {
            throw new ArgumentNullException(nameof(fileName));
        }

        if (string.IsNullOrWhiteSpace(fileName))
        {
            throw new ArgumentException($"The parameter {nameof(fileName)} may not be empty or whitespace", nameof(fileName));
        }

        if (File.Exists(fileName) == false)
        {
            throw new FileNotFoundException("File could not be found.", fileName);
        }

        _configItems = File.ReadLines(fileName)
                           .Where(IsConfigurationLine)
                           .Select(line => line.Split('='))
                           .ToDictionary(line => line[0], line => line[1]);
    }
           

    private static bool IsConfigurationLine(string line)
    {
        return !line.StartsWith("#") && line.Contains("=");
    }

    public bool TryGetValue(string key, out string value)
    {
        if (key == null)
        {
            throw new ArgumentNullException(nameof(key));
        }
        
        return _configItems.TryGetValue(key, out value);
    }
}