6
\$\begingroup\$

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
    {
        /// <summary>
        /// Holds a list of configuration keys and values.
        /// </summary>
        private readonly Dictionary<string, string> _configItems = null;

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

        /// <summary>
        /// Log4net logger.
        /// </summary>
        private readonly ILog _log = null;

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


        /// <summary>
        /// AppleConfig constructor.
        /// </summary>
        /// <param name="filePath">Configuration file path</param>
        public AppleConfig(string filePath)
        {
            this._configItems = new Dictionary<string, string>();
            this._configFile = new FileInfo(filePath);
            this._log = LogManager.GetLogger(typeof(AppleConfig));
        }

        /// <summary>
        /// Initializes AppleConfig
        /// </summary>
        public void Initialize()
        {
            if (this._initialized)
                return;

            try
            {
                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];
                }
            }
            catch (Exception exception)
            {
                _log.Error(exception); // assuming overload that takes an exception.
                //Not sure what or if even to throw something here...
            }
            finally
            {
                this._initialized = true;
            }
        }

        /// <summary>
        /// Checks if a string is a valid config item.
        /// </summary>
        /// <param name="Line">String to check</param>
        /// <returns></returns>
        private bool IsConfigurationLine(string Line)
        {
            return (!string.IsNullOrWhiteSpace(Line) && !Line.StartsWith("#") && Line.Contains("="));
        }

        /// <summary>
        /// Returns a config item by its key in the dictionary.
        /// </summary>
        /// <param name="Key"></param>
        /// <returns></returns>
        public string GetConfigItem(string Key)
        {
            return this._configItems[Key];
        }
    }
}
\$\endgroup\$
3
  • \$\begingroup\$ New question new accout? ;-) It looks much better now. If you just took Mat'sMug review into account too and removed all those this' it would be ever better. Parameters like Line and Key should be lower case. Usually only public fields/properties begin with an upper case letter. \$\endgroup\$
    – t3chb0t
    Commented Oct 28, 2015 at 5:35
  • \$\begingroup\$ I have finally ditched the this keywords after a long goodbye, thanks for that advice. I have also took your advice about lowercase parameters inside voids. \$\endgroup\$
    – Liam Hardy
    Commented Oct 28, 2015 at 17:08
  • 1
    \$\begingroup\$ Why don't you wish to use them? They're perfectly good, well tested solutions that work great in all but a handful of situations. \$\endgroup\$
    – RubberDuck
    Commented Oct 29, 2015 at 0:00

2 Answers 2

9
\$\begingroup\$

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

    /// <summary>
    /// AppleConfig constructor.
    /// </summary>

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

    /// <param name="Key"></param>
    /// <returns></returns>

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 empty key and an empty value. Wouldn't we want to reject such entries?

What about keys that repeat in the file? Currently every entry would be just overwritten by the latest encountered value.

What about lines that contain more than one "="? aa=bb=cc will be parsed to (key) "aa" (value) "bb". Wouldn't we want the value to be "bb==cc"?

This is just food for thought, I'm not saying the code is wrong.

\$\endgroup\$
6
  • 3
    \$\begingroup\$ You went above and beyond in your review, so good job overall. However, if there's any chance that you will ever have a desire/need to automatically extract documentation from the code then you'll want to ensure that every class declaration, method, field, parameter and property has xml doc comments. Even if trivial. The couple of auto doc extraction tools I've used have very "unexpected" behavior when they come across missing xml doc comments. Thus, you'll be doing a lot of needless (and not always simple) hand-editing if you choose to leave the xml comments out. \$\endgroup\$
    – Dunk
    Commented Oct 28, 2015 at 15:21
  • \$\begingroup\$ @Dunk I admit I haven't used such tools, and based on what you're saying I don't regret it :) It's a valid point for sure, for those who have to deal with a documentation generator \$\endgroup\$ Commented Oct 28, 2015 at 16:13
  • 1
    \$\begingroup\$ I'll take the hit for that _configItems[splittedLine[keyIndex]] = splittedLine[valueIndex] ;-) It was my advice to do it this way and I find it perfectly clear. Whether it should be this or this._configItems[key] = value; I find it's a matter of taste. The most important thing is that it's readable and understandable without thinking what it might do. Did you see the line before? I think just anything is better then this configValues[line.Split('=')[0]] = line.Split('=')[1]; ;-] \$\endgroup\$
    – t3chb0t
    Commented Oct 28, 2015 at 16:13
  • \$\begingroup\$ @t3chb0t well your alternative is definitely an improvement over the original (which I didn't see), this goes without saying. I'd just take it one step further. Note that de-inlining (is that a word?) splittedLine[...] references didn't result in more LOC in this case, because at the same time I got rid of these consts. I agree, at the end of the day it's a matter of taste how much you are willing to inline. personally I prefer to err on the side of readability (the way I see it), but there's no silver bullet of course \$\endgroup\$ Commented Oct 28, 2015 at 16:16
  • \$\begingroup\$ Thank you to Konrad Morawski for an amazing review, although I am worried about the mention of not thread safe Initialize? would I need to go above and beyond and use locking. Please let me know, thanks. \$\endgroup\$
    – Liam Hardy
    Commented Oct 28, 2015 at 17:07
3
\$\begingroup\$

What happens if a caller of this class forget to call Initialize before the GetConfigItem() is called ? The caller receives an ArgumentNullExpection.

To prevent this it would be better to make Initialize private and call it from the constructor. If we then let Initialize return a bool this returned value could be assigned to the _initialized variable which therefor could be made readonly.

Next we should check in the GetConfigItem() method if _initialized == true and if not we just return null. Having a property IsInitialized would be good too, so a caller could check this property if he get null from that method.

Based on the NET naming guidelines method arguments should be named using camelCase casing.

Usually a variable with an underscore prefix is meant to be a class variable, so no need to use this.

/// <summary>
/// Returns a config item by its key in the dictionary.
/// </summary>
/// <param name="Key"></param>
/// <returns></returns>  

this documentation exposes to much of the implementation to the caller. Why should the caller should know that you are using a Dictionary<TKey, TValue> ? He shouldn't care and know about this.

If you pass the filename to the Initialize() method as a parameter the _configFile can be removed but only if you don't plan to add e.g a public Refresh() method which would reload the file to reflect any changes happend to the config file.

\$\endgroup\$
8
  • \$\begingroup\$ Thanks for your response, would just re-calling Initialize be okay to refresh config? \$\endgroup\$
    – Liam Hardy
    Commented Oct 28, 2015 at 5:56
  • 1
    \$\begingroup\$ IMO having a method Refresh or RefreshSetting would be more clear. Under the hood this method could just call Initialize. \$\endgroup\$
    – Heslacher
    Commented Oct 28, 2015 at 5:58
  • \$\begingroup\$ Exacly what I meant, calling Initialize inside that. Thank you for your response though it has helped a lot, one part I don't understand totally is the part about underscore prefix? Do you mean just don't give so much information on summarys? \$\endgroup\$
    – Liam Hardy
    Commented Oct 28, 2015 at 6:00
  • 1
    \$\begingroup\$ Okay thankyou, I will stop using this. keyword with underscore private class variables also. \$\endgroup\$
    – Liam Hardy
    Commented Oct 28, 2015 at 6:07
  • 2
    \$\begingroup\$ Wouldn't calling Initialize from the constructor amount to doing work and accessing the file system and throwing exceptions in the constructor? \$\endgroup\$ Commented Oct 28, 2015 at 14:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.