Problem
The program below basically reads configuration data from an .ini file in ini format. The exemplary configuration file was also added to this post . Let me know how I can improve it, please.
using Sahara.Core.Logging;
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
using System.Linq;
namespace Sahara.Core.Config
{
class ConfigManager
{
private readonly string configFilePath;
private readonly Dictionary<string, string> configItems;
private readonly LogManager logManager;
public ConfigManager(string configFile)
{
configFilePath = configFile;
configItems = new Dictionary<string, string>();
logManager = Sahara.GetServer().GetLogManager();
try
{
if (File.Exists(configFilePath))
{
Stopwatch stopwatch = Stopwatch.StartNew();
configItems = File.ReadLines(configFilePath)
.Where(IsConfigurationLine)
.Select(line => line.Split('='))
.ToDictionary(line => line[0], line => line[1]);
stopwatch.Stop();
logManager.Log("Loaded Config Data [" + stopwatch.ElapsedMilliseconds + "ms]", LogType.Information);
}
else
{
logManager.Log("Using config defaults", LogType.Information);
configItems.Add("database.host", "localhost");
configItems.Add("database.username", "root");
configItems.Add("database.password", "");
configItems.Add("database.name", "database");
configItems.Add("database.port", "3306");
configItems.Add("database.max_connections", "10000");
}
}
catch (Exception exception)
{
if (logManager != null)
{
logManager.Log("Error in loading config: " + exception.Message, LogType.Error);
logManager.Log(exception.StackTrace, LogType.Error);
}
}
}
private bool IsConfigurationLine(string line)
{
return !line.StartsWith("#") && line.Contains("=");
}
public string GetConfigElement(string key)
{
if (!IsInitialized)
throw new NotImplementedException("Config data has not get been implemented.");
if (!configItems.ContainsKey(key))
throw new KeyNotFoundException("Unable to find key " + key + " in config items dictionary.");
return this.configItems[key];
}
public bool IsInitialized
{
get { return configItems != null; }
}
}
}
Config File (.ini):
database.host=127.0.0.1
database.username=root
database.password=*********
database.name=******
database.port=3306
database.max_connections=10000
Solution
This caught my eye:
if (!IsInitialized)
throw new NotImplementedException("Config data has not get been implemented.");
A NotImplementedException is usually a kind of “meta” exception that you typically find in unimplemented method stubs, e.g.
public string GetConfigElement(string key)
{
throw new NotImplementedException();
}
It’s basically ilke a “todo” marker that [intentionally] blows up at runtime:
You might choose to throw a NotImplementedException exception in properties or methods in your own types when the that member is still in development and will only later be implemented in production code. In other words, a NotImplementedException exception should be synonymous with “still in development.”
Throwing that exception as part of the implementation of a method is confusing; in this case the goal is to tell the caller that the current state of the object does not allow using this method yet.
In other words it should be an InvalidOperationException:
The exception that is thrown when a method call is invalid for the object’s current state.
See the remarks section on MSDN for more guidelines on throwing that specific exception.
I find the constructor is doing too many things:
var foo = new ConfigManager(path);
With this single instruction, I’ve just hit the file system, read the contents of some file, and cached a number of configuration settings in an encapsulated dictionary.
The problem is, if I wanted to write a unit test to document how the GetConfigElement works (and then another to document how it fails), I couldn’t, because my test would depend on file I/O.
I’d suggest moving that logic out of the constructor, and delay doing that work.
It would be nice to include an indexer, so client code could do this:
var foo = new ConfigManager(path);
var port = foo["port"];
That said you’re reinventing the wheel here, if this is production code you should probably be using the framework-provided XML configuration, using the System.Configuration namespace; you could store your connection strings in a dedicated <ConnectionStrings> section.
public bool IsInitialized
{
get { return configItems != null; }
}
Well, this will always return true because configItems is initialized in the constructor and it is readonly so you won’t change it anywhere.
Btw. why is this property public ?
This
public string GetConfigElement(string key)
{
if (!IsInitialized)
throw new NotImplementedException("Config data has not get been implemented.");
if (!configItems.ContainsKey(key))
throw new KeyNotFoundException("Unable to find key " + key + " in config items dictionary.");
return this.configItems[key];
}
should better use a call to TryGetValue because calling Contains and if this returns true calling the Item property will result in internal calling another method twice.
The above should be implemented like so
public string GetConfigElement(string key)
{
string value = null;
if (!configItems.TryGetValue(key, out value))
{
throw new KeyNotFoundException("Unable to find key " + key + " in config items dictionary.");
}
return value;
}
Btw, if I remember correctly I have already told you that you should always use braces {}, although they might be optional, which would make your code less error prone.
catch (Exception exception)
{
if (logManager != null)
{
logManager.Log("Error in loading config: " + exception.Message, LogType.Error);
logManager.Log(exception.StackTrace, LogType.Error);
}
}
It seems the code expects to probably get null from the call to Sahara.GetServer().GetLogManager(); ? If yes, how should the logManager log something in the try part ?
Call your types Manager only as a last resort. Most of the time there is a better name like here: Configuration.
Your configuration works in a weird way. You throw an exception about not initialized configuration from a method that cannot be called without initializing the configuration first. You don’t need the IsInitialized stuff at all.
You can only have a configuration that is either loaded from a file or has default values so it’s always initialized.
Now let’s try to reorganize the configuration and get rid of the magic strings that the user needs to use.
You can achieve this with interfaces. Create one for the database configuration:
interface IDatabaseConfiguration
{
string Host { get; set; }
}
and let the Configuration implement it with explicit interface implementation:
class Configuration : IDatabaseConfiguration
{
private readonly Dictionary<string, string> _settings;
public Configuration(string fileName)
{
// ...
}
private string this[string key]
{
get { return _settings[key]; }
set { _settings[key] = value; }
}
public IDatabaseConfiguration Database => this;
string IDatabaseConfiguration.Host
{
get { return this["database.host"]; }
set { this["database.host"] = value; }
}
}
Now you can use it like this:
var config = new Configuration(@"c:config.ini");
var host = config.Database.Host;
If you have more setting groups then just add interfaces for them too and implement them in the same way.
Other issues have been already metioned in other reviews.