Simple Configuration Manager

below I have coded a simple configuration manager that loads config elements from a file straight into a dictionary, and also has a function for getting an element by its key in the dictionary. I just wondered if there is anything I can do to improve this..

using log4net;
using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;

namespace Apple_Server.Application.Base.Core.Config
{
    class ConfigManager
    {
        private readonly Dictionary configValues = null;
        private readonly FileInfo configFile = null;
        private readonly ILog _log = null;
        private bool _initialized = false;

        public ConfigManager(string filePath)
        {
            this.configValues = new Dictionary();
            this.configFile = new FileInfo(filePath);
            this._log = LogManager.GetLogger(typeof(ConfigManager));
        }

        public void Initialize()
        {
            if (!this._initialized)
            {
                try
                {
                    foreach (string line in File.ReadLines(this.configFile.ToString()).Where(s => !string.IsNullOrWhiteSpace(s) && !s.StartsWith("#") && s.Contains("=")))
                    {
                        this.configValues[line.Split('=')[0]] = line.Split('=')[1];
                    }
                }
                catch (FileNotFoundException)
                {
                    this._log.Error("Unable to find the config file.\nPress any key to exit.");
                    Console.ReadKey(true);

                    Environment.Exit(Environment.ExitCode);
                }
                catch (Exception)
                {
                    Console.WriteLine("opppppps");
                }
            }

            this._initialized = true;
        }

        public string GetConfigElement(string Key)
        {
            return configValues[Key];
        }
    }
}

You're not consistent with your private fields:

private readonly Dictionary configValues = null;
private readonly FileInfo configFile = null;
private readonly ILog _log = null;
private bool _initialized = false;

Several things:

• If you're going to use an _underscore prefix, then don't use a redundant this qualifier to refer to the field names later: it defeats the purpose.

• If you're not going to use an _underscore prefix for private fields, then don't use one for any private field. Be consistent. Either you this or you don't.

• The 3 readonly fields are initialized in the constructor. Why are they also statically initialized then?

• Why bother initializing a field to its default value? Reference types are null, and a bool is false by default. The static initialization is redundant.

private readonly Dictionary _configValues;
    private readonly FileInfo _configFile;
    private readonly ILog _log;
    private bool _initialized;

    public ConfigManager(string filePath)
    {
        _configValues = new Dictionary();
        _configFile = new FileInfo(filePath);
        _log = LogManager.GetLogger(typeof(ConfigManager));
    }

The first condition in the Initialize method could be inverted to eliminate a whole level of nesting:

if (_initialized)
{
    return;
}

This has the added benefit of avoiding a redundant assignment, since you don't need to set _initialized to true when it's already true.

Don't do this:

Environment.Exit(Environment.ExitCode);

The configuration manager shouldn't be allowed to terminate its client. That's a big, huge no-no right there.

Instead, throw some ConfigurationUnavailableException and let the client code deal with it - if it wants to terminate when configuration is unavailable, let it do so. If it wants to work off default values, allow it to do so. Don't just let that one component pull the plug because it's not happy with the execution path!

This isn't useful either:

catch (Exception)
{
    Console.WriteLine("opppppps");
}

I'd let System.Exception bubble up here, and catch a more generic System.IOException after dealing with the FileNotFoundException - perhaps the file was found, but the logged-in user can't read-access it?

Ideally you would just catch IOException and log the exception type instead of... wait a minute. That's not a logger, it's a console wrapper! How confusing!

this._log.Error("Unable to find the config file.\nPress any key to exit.");
Console.ReadKey(true);

Does the user-friendly message get output to the Console? If so, then what do you need ILog for? Your class is already coupled with System.Console! Or it's a logger, but you're assuming the output is configured to go to the console? Logging shouldn't interact with your code in any way, and seeing "press any key to exit" in a log file is a bit awkward.

I think I'd do this instead:

try
{
    // load configuration
}
catch(Exception exception)
{
    _log.Error(exception); // assuming overload that takes an exception.
    throw new ConfigurationUnavailableException(exception);
}

So we get a log trace of the actual exception, and bubble it up to the caller via a new ConfigurationUnavailableException that can be caught and dealt with at the call site, which would be responsible for the "press any key to continue" part.

StackExchange Code Review Q#108918, answer score: 6