4
\$\begingroup\$

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;
    }
}
\$\endgroup\$
  • \$\begingroup\$ What's the reason for using writing your own parser instead of using the built-in .NET solution? \$\endgroup\$ – Voo Jan 24 '18 at 9:02
  • 1
    \$\begingroup\$ INI files are pretty old (at least Windows 3 if not older) and an inflexible way of storing configuration. .net has used xml for over 10 years, and since .net core has been using json for configuration settings. Unless there is a great need for this (or it's a learning exercise), I would use the modern libraries. \$\endgroup\$ – Neil Jan 24 '18 at 9:14
3
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ Pascal case for static readonly fields, public or not, is pretty standard. \$\endgroup\$ – Johnbot Jan 24 '18 at 8:15
  • 2
    \$\begingroup\$ Different naming guidelines for static vs. member fields is quite widespread and mostly a matter of taste. The same is true for public on internal variables, Eric wrote about that one in length (I agree with him that public is the better choice fwiw). \$\endgroup\$ – Voo Jan 24 '18 at 9:01
  • \$\begingroup\$ I named my fields and properties that way because as far as I know (or knew) that was the official naming convention guidlines of the language. As for "Why do you use Logger to encapsulate the Console for output" I do this as I wish to shutdown after any key is pressed, the application cannot run without a valid configuration so it would be pointless to continue running. \$\endgroup\$ – ropuxil Jan 24 '18 at 15:00
  • \$\begingroup\$ I thank you for all the other comments, they've helped me improve the code which was my main aim. The reason I also made it only for console is I would only be using it on console. I have no desire in changing what it runs on, it was made specifically for this console application and not to run on any kind of .NET application (WPF, WinForms, etc). \$\endgroup\$ – ropuxil Jan 24 '18 at 15:01
3
\$\begingroup\$

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);
    }
}
\$\endgroup\$
  • \$\begingroup\$ Thank you for writing this, its helped me understand exception handling much better, among other things that you've included. The only advice I didn't take was making 'TryGetValue' a bool, I made it a string as if it was a bool I would have to declare variables for the returned value, my current code just passes the void as it would pass a string. \$\endgroup\$ – ropuxil Jan 24 '18 at 15:07

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.