3
\$\begingroup\$

Today I coded a class to get settings from a file and to load them in to a dictionary.

Please post and feedback, improvements and changes, no matter how big or small, as long as they are actual improvement and not disadvantages.

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

namespace Sahara.Application.Base.Core.Settings
{
    class Settings
    {
        /// <summary>
        /// Holds a list of settings.
        /// </summary>
        private Dictionary<string, string> allSettings;

        /// <summary>
        /// log4net instance
        /// </summary>
        private ILog myLogger;

        /// <summary>
        /// syncRoot object
        /// </summary>
        private object syncRoot;

        /// <summary>
        /// Creates a new instance of Settings
        /// </summary>
        public Settings()
        {
            allSettings = new Dictionary<string, string>();
            myLogger = LogManager.GetLogger(typeof(Settings));
            syncRoot = new object();

            if (!loadSettings("settings.ini"))
                throw new Exception("Failed to load settings.");
        }

        /// <summary>
        /// Loads the settings from the settings file
        /// </summary>
        /// <param name="file">Settings file path</param>
        /// <returns>bool loadSettings</returns>
        private bool loadSettings(string file)
        {
            try
            {
                allSettings.Clear();

                lock (syncRoot)
                {
                    allSettings = File.ReadLines(file)
                    .Where(isValidLine)
                    .Select(line => line.Split('='))
                    .ToDictionary(line => line[0], line => line[1]);
                }

                return true;
            }
            catch (Exception exception)
            {
                myLogger.Error(exception);
                return false;
            }
        }

        /// <summary>
        /// Checks if the line is a comment.
        /// </summary>
        /// <param name="line">Line to check</param>
        /// <returns>bool isValidLine</returns>
        private bool isValidLine(string line)
        {
            return !line.StartsWith("#") && line.Contains("=");
        }

        /// <summary>
        /// Returns a setting with the specified key.
        /// </summary>
        /// <param name="key">Key to search for</param>
        /// <returns>Setting</returns>
        public string getSetting(string key)
        {
            if (allSettings == null)
                throw new NotImplementedException("Config data has not get been implemented.");

            lock (syncRoot)
                return this.allSettings[key];
        }
    }
}
\$\endgroup\$
1
\$\begingroup\$

Comments :

Locking is unnecessary :Threads will create individual instances.

Logging is unnecessary : Propagate all exceptions to the caller which makes him aware of the state of the instance being created.

Improvements

Make the class testable.

Delegate file reading to a separate object.

It is safe to use Dictionary.TryGetValue(string key, out string value).

Example:

public class FileSettings :ISettings
{
    private readonly IFileToDictionary _fileReader;
    private Dictionary<string,string> _settings;

    public FileSettings (IFileToDictionary fileToDictionary)
    {
        if (fileToDictionary == null)
            throw new ArgumentNullException ("fileToDictionary");

        _fileReader = fileToDictionary;
        Init ();
    }
    #region ISettings implementation
    public string Get (string key)
    {
        string value;
        _settings.TryGetValue (key, out value);
        return value;
    }

    public Dictionary<string, string> Settings {
        get {
            return _settings;
        }
    }
    #endregion

    #region Private Methods
    private void Init()
    {
        _settings = _fileReader.Read ();    
    }
    #endregion

}

Unit tests

[TestFixture]
public class FileSettingTests
{

    private FileSettings _sut;
    private MockFileToDictionary _fileToDictionary;
    [SetUp]
    public void SetUp()
    {
        _fileToDictionary = new MockFileToDictionary ();
    }

    [Test]
    public void Ctr_LoadSettings()
    {
        _fileToDictionary.Init (new System.Collections.Generic.Dictionary<string, string>(){
            {"color","red"}
        });
        _sut = new FileSettings (_fileToDictionary);
        Assert.That (_sut.Settings, Is.Not.Null, "Should not null if agreed file exists");
    }

    [Test]
    public void ShouldLoadValidData()
    {
        _fileToDictionary.Init (new System.Collections.Generic.Dictionary<string, string>(){
            {"color","red"}
        });
        _sut = new FileSettings (_fileToDictionary);
        var expectedValue = "red";

        var actual = _sut.Get ("color");

        Assert.That (expectedValue, Is.EqualTo(actual), string.Format ("Should match expected {0}", expectedValue));
    }

}
\$\endgroup\$
1
\$\begingroup\$

Pointless locking

You lock in two places:

  1. When loading the configuration file, during construction. This lock will never be contested: no two threads will ever wait on this lock, because every object has its own, different, private lock

  2. When getting a value from the dictionary. This lock may be contested in theory by multiple threads trying to get values, but there is no reason to lock for that. It would simply slow down high throughput of requests, with no benefits

You can safely remove the locking completely.

Exception handling

loadSettings catches exceptions during file loading, logs and returns a boolean. It would be simpler to not catch exceptions here, let them be thrown, and return void.

Since an instance of this class is unusable if an error happens during file loading, consider not catching any exceptions at all, let them be thrown, and let the caller handle the situation.

That will also allow you to remove the null check in the getSetting method, as the dictionary will never be null. Which is nice, as it's always easier to understand a class when you know it can never be in an invalid state.

Further improving loadSettings

Clearing the settings dictionary in this method is pointless. It would be better to create the dictionary here and return it.

\$\endgroup\$

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.