Basic Config Class

I have recently made some major changes to my configuration file and wondered if I could get it reviewed, and be told what I can improve on it. My designer pattern and personal preference has changed a lot since I originally wrote this class, and I have basically re-wrote it. I just wanted to know if anything is poor code practice here, or if I am missing something that could be improved a whole lot more than how I have coded it in the class below.

Yes I do know that there is a config system in app.config and one for .NET. I do not wish to use them.

The reason for this is that I believe that they are slow. If I am incorrect about this, please say so below and I will consider and other suggestions for config systems. Also, the config system must be outside the source, as this will be on public release closed source to do with a public project.

```
namespace Apple.Application.Base.Config
{
using log4net;
using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;

internal class AppleConfig
{
///
/// Holds a list of configuration keys and values.
///
private readonly Dictionary _configItems = null;

///
/// Holds file information about the configuration file.
///
private readonly FileInfo _configFile = null;

///
/// Log4net logger.
///
private readonly ILog _log = null;

///
/// Checks if the class has been initialized already.
///
private bool _initialized;

///
/// AppleConfig constructor.
///
/// Configuration file path
public AppleConfig(string filePath)
{
this._configItems = new Dictionary();
this._configFile = new FileInfo(filePath);
this._log = LogManager.GetLogger(typeof(AppleConfig));
}

///
/// Initializes AppleConfig
///
public void Initialize

Thread-safety

Note that Initialize() is not thread-safe. Someone could call it the second time while it's still processing the file, and your safeguard: if (this._initialized) return; will not be enabled yet.

This may or may not be a problem, it's just good to bear this in mind - let thread safety, or the lack thereof, be a deliberate decision.

Code style

Redundant comments

Plenty of them, eg.:

/// 
    /// AppleConfig constructor.
    /// 

I can see it's a constructor. This sort of comments doesn't add any value whatsoever, it's just noise / fluff that inflates the codebase.

Comments aren't a goal in and of itself, quite the contrary. If you can't come up with a meaningful useful comment, just leave it.

Empty tags

/// 
    /// 

What's the point? Let's keep the file clean. It's a detail of course, but keeping code files nice and neat kind of sends a good message.

As @Dunk pointed out, sometimes maintaining such cruft may be required by documentation generator tool. If that's the case, tough luck, but if it isn't - no point.

Redundant parantheses and conditions

return (!string.IsNullOrWhiteSpace(Line) && !Line.StartsWith("#") && Line.Contains("="));

The enclosing parantheses serve no purpose I could think of.

Plus, can line really be null? Under what conditions would File.ReadLines return a null element? I don't think this ever happens. So we don't need !IsNullOrWhiteSpace, because Contains("=") ensures that already.

Thus it could be simplified to:

return !line.StartsWith("#") && line.Contains("=");

Error handling

We give up on the first line that causes an exception, and yet we still "pretend" that config was initialized. This doesn't seem very maintainable to me.

Shouldn't this code just ignore lines that caused errors, but still try to parse as much info as possible?

As for communicating the actual error to the caller - if we don't want them to deal with an Exception - we could expose an event (say, OnError) to which the caller could subscribe, or have Initialize() return some object indicating either a success or a failure (containing some details of what went wrong).

And this bit, the heart of your mechanism.

foreach (string line in File.ReadLines(this._configFile.ToString()).Where(IsConfigurationLine))
            {
                var splittedLine = line.Split('=');

                const int keyIndex = 0;
                const int valueIndex = 1;

                this._configItems[splittedLine[keyIndex]] = splittedLine[valueIndex];
            }

(Ab)use of ToString() where a better alternative exists

I'd suggest that you use _configFile.FullName instead of _configFile.ToString(). Yes, ToString happens to return the same value, but FullName is more descriptive and more right. ToString is for debugging purposes, if its value happens to be useful in the actual code, I'd treat it as circumstantial.

Slight misuse of constants (imho)

What's the point of redefining both constants in every iteration of the loop?

const int keyIndex = 0;
const int valueIndex = 1;

Move them outside of the loop, or - better still - let's just ditch them.

The problem is not 0 and 1 here, what's hardly readable is this line: this._configItems[splittedLine[keyIndex]] = splittedLine[valueIndex]. We could fix it though:

String key = splittedLine[0];
                String value = splittedLine[1];

                this._configItems[key] = value;

Isn't that cleaner? And it's implicitly obvious that 0 is a key index and 1 a value index.

Field initializers

I don't understand the point of initializing member fields to null. Does C# enforce that for readonly members (I can't remember)? If it does, I retract this comment.

private readonly FileInfo _configFile = null;
    private readonly ILog _log = null;

If not, I'd get rid off this, it's noisy.

Others

Mixing functional and imperative approach

The same piece of code once again. Now, I'm not saying this is wrong by itself:

foreach (string line in File.ReadLines(this._configFile.ToString()).Where(IsConfigurationLine))
            {
                var splittedLine = line.Split('=');

                const int keyIndex = 0;
                const int valueIndex = 1;

                this._configItems[splittedLine[keyIndex]] = splittedLine[valueIndex];
            }

It's a hybrid solution, so to speak. But if you are using LINQ already, why not go all the way with it?

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

(It's off the top of my head, not tested, but you get the idea).

Edge cases

What to do with empty keys? You know that a line consisting of just "=" would be seen as a valid config entry, with an empt

StackExchange Code Review Q#108967, answer score: 9