Loading settings from a file to a dictionary 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
    {
        /// &lt;summary&gt;
        /// Holds a list of settings.
        /// &lt;/summary&gt;
        private Dictionary&lt;string, string&gt; allSettings;

        /// &lt;summary&gt;
        /// log4net instance
        /// &lt;/summary&gt;
        private ILog myLogger;

        /// &lt;summary&gt;
        /// syncRoot object
        /// &lt;/summary&gt;
        private object syncRoot;

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

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

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

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

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

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

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

            lock (syncRoot)
                return this.allSettings[key];
        }
    }
}
Pointless locking

You lock in two places:


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
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.