Configuration Handler

Question

I have below coded a class that loads information from a file that I select. The file then loads the content of that file in the INI format.

The INI format usually works when each line contains key=value and then each line is split by the = character in the code. It also contains a method for getting the item by key from the dictionary of stored items previously loaded from the file. I wondered if anyone could help me out in finding any way to improve this?

I think its pretty well coded in my eyes, but of course there is always room for improvement.

namespace Hariak_Emulator.Emulator.Core.Config
{
    using System;
    using System.Collections.Generic;
    using System.IO;
    using System.Linq;
    using NLog;
    using Utilities;

    internal sealed class ConfigManager : IDisposable
    {
        private static readonly ILogger Logger = LogManager.GetCurrentClassLogger();

        private Dictionary<string, string> _configItems;
        private string _configFile;

        public ConfigManager()
        {
            _configItems = new Dictionary<string, string>();
            _configFile = "assets/config/hariak_config.ini";

            LoadConfig();
        }

        private void LoadConfig()
        {
            try
            {
                if (!File.Exists(_configFile) || Utilities.IsFileLocked(new FileInfo(_configFile)))
                {
                    Logger.Error("Either your config file couldn't be found, or its open somewhere else.");

                    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("Error while loading config: " + exception.Message);
                Logger.Fatal(exception.StackTrace);

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

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

        internal string GetConfigValueByKey(string key)
        {
            string value;

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

            return value;
        }

        public void Dispose()
        {
            if (!disposing)
            {
                return;
            }

            _configItems.Clear();
            _configItems = null;
            _configFile = "";
        }
    }
}

Answer 1

IMHO the code looks fine so far.

However, I see few improvements:

Class Design

• The instance variables _configItems and _configFile should be readonly
• There is no need to implement IDisposable because you don't reference resources that must be disposed.
• It makes sense to pass the file as constructor argument to make the class reusable for any ini files.
• Maybe it makes sense to provide also a constructor that receives a stream, so that the class can be used with embedded resource ini files and so on.
• In method LoadConfig you could create the FileInfo object once and use it subsequently (File.Exists and IsFileLocked)

Behavior

• The method IsConfigurationLine could be turned into a more tolerant version when trimming the line string before (to support comments where the '#' is not on the first position)
• If the value contains also '=' characters, your method returns only the part between the '=' that separates the key and the value and the first occurring '=' in the value.
• The class kills the application if loading the configuration file fails. That may be a valid behavior for your application, but it is nothing that should be decided by the method that loads the configuration code. IMHO a better approach is to throw the exception and let the calling code handle the behavior.
• The key is case sensitive. I am not sure if that is desired, otherwise you can simply change that by defining the comparer when initializing the dictionary: _configItems = new Dictionary<string, string>(StringComparer.InvariantCultureIgnoreCase);

Answer 2

• I would argue with the use of "ini" file extension. ini files are relatively standardized ( e.g. look here https://en.wikipedia.org/wiki/INI_file ) but it seems your code will not be able to handle the most common type of comments (starting with ";"), and sections ("[section]") will be ignored. Probably you want to make up your own file extension (.cfg or .config would probably be more appropriate since it is less defined).

• I also feel this is kind of reinventing the wheel, with XML or JSON or something you should be able to implement this with only a few lines of code - meaning less bugs (though at the expense of more overhead in the configuration file and a few more CPU cycles)

• You are mixing "Config" and "Configuration". Microsoft / C# naming conventions suggest never to abbreviate, so it would be more correct to call it ConfigurationManager than ConfigManager. (The reason for never abbreviating is to ensure consistency). (Some of the Config/Configuration could be removed completely since it's redundant to have it repeated in the class AND so many properties and methods.)

Answer 3

You have absolutely no reason to implement IDisposable and the dispose pattern. Can you explain why you believe you need that?

    public void Dispose()
    {
        if (!disposing)
        {
            return;
        }

        _configItems.Clear();
        _configItems = null;
        _configFile = "";
    }

Neither _configItems or _configFile need to be "disposed". Please don't go down the "former C++ programmer struggling to adjust to a managed language" path. This happens far too often and leads to horrible code.

https://blog.stephencleary.com/2009/08/first-rule-of-implementing-idisposable.html

Answer 4

Don't catch exception, try to catch specifically the exceptions you're expecting to see. In this case you'll want to catch things here https://msdn.microsoft.com/en-us/library/dd383503(v=vs.110).aspx#Exceptions and whatever Utilities.IsFileLocked might throw. You might also need some exceptions around your LINQ statements afterwards in case the .ini file isn't valid, e.g.

key=value
key without value=
key=value with an = sign in it

In which case the second line wouldn't have a line[1]. Similarly you don't currently handle the third line properly.

Also consider your access levels. The constructor is public but because the class is internal the constructor can only be accessed internally too. I'd also recommend sorting your methods in such a way that private functions are at the bottom but that's a personal preference/situational thing.

The following points are low priority but worth thinking about.

As others have mentioned you don't need the word Config in your methods (and be consistent about it, don't use both Config and Configuration). Think about how the calling code would read, e.g. configManager.GetConfigValueByKey. Since you can get autocomplete hints with argument names now if the method signature was internal string GetValue(string key) then it's more readable. Similarly IsConfigurationLine isn't the best name in my opinion. Since you're then converting into a dictionary that uses KeyValuePairs, IsKeyValuePair might be better.

GetConfigValueByKey could return null if the TryGetValue fails but this might be what you want.

_configFile doesn't need to be stored if you pass it straight into LoadConfig and as others have mentioned, its value should be passed into the constructor.

Your error message for failing to read the file could be improved, it says or its open somewhere. Firstly that should be it's as its is possessive. Secondly that isn't the only reason a file might be locked. It also doesn't include the filename itself. A better error message might be: $"Failed to open {_configFile}. It either does not exist or is locked.". Similarly the Missing configuration key message could be better depending on your expected usage. I would use $"Key not found: {key} or something along those lines. To me, "missing" implies something more than simply not finding something. Also note the string interpolation using $ there if you're using C#6.

@JanDotNet also mentioned the class killing the application if loading fails. I'd add that it also shouldn't interact with the console. In future the class might be used for a non-console application and throwing exceptions with the relevant messages would make it easier to move them from the console to a dialog window or web callback.

Finally, C# projects have an app.config file that can be used for configuration like this, with a pre-built configuration manager. It would be better to use that unless you need to support .ini files specifically.