4
\$\begingroup\$

Currently I am developing an application which uses a few variables which should be changable in a .ini file. Since I want to make the application to be able to continue if the user "forgets" to correctly implement the variables (read: "retard-proof"), I figured I would have to create an object for each variable which holds a default value and an optional set value. My implementation looks like this at the moment;

IProperty.cs

public interface IProperty
{
    bool IsSet { get; set; }
    string PropertyName { get; set; }
    object GetValue();
}

Property.cs

public class Property<T> : IProperty
{
    public bool IsSet { get; set; }

    public Property()
    {
        IsSet = false;
    }

    public string PropertyName { get; set; }

    public T DefaultValue { get; set; }

    private T _CurrentValue;
    public T CurrentValue
    {
        get
        {
            return _CurrentValue;
        }
        set
        {
            IsSet = true;
            _CurrentValue = value;
        }
    }

    private T GetActualValue()
    {
        return IsSet ? CurrentValue : DefaultValue;
    }

    public object GetValue()
    {
        return (object)GetActualValue();
    }
}

PropertyFactory.cs

public static class PropertyFactory
{
    private static Dictionary<string, IProperty> CreateDefaults()
    {
        Dictionary<string, IProperty> dic = new Dictionary<string, IProperty>();
        List<IProperty> propList = new List<IProperty>();

        #region String properties
        propList.Add(new Property<string>(){ DefaultValue = @"C:\...\...", PropertyName = "log_path"});
        //...                      
        #endregion

        #region Int32 properties
        propList.Add(new Property<int>() { DefaultValue = 1337, PropertyName = "some_value"});         
        //...   
        #endregion

        #region Boolean properties
        propList.Add(new Property<bool>() { DefaultValue = false, PropertyName = "log_enabled" });
        //...
        #endregion

        //Map to the dictionary based on their name
        foreach(IProperty prop in propList)
        {
            dic.Add(prop.PropertyName, prop);
        }

        return dic;
    }

    public static ConcurrentDictionary<string, IProperty> CreateFromFile(string path)
    {
        Dictionary<string, IProperty> dic = CreateDefaults();
        INIFile ini = new INIFile(path);
        Dictionary<string, string> values = ini.GetAllValues();

        foreach(KeyValuePair<string, string> pair in values)
        {
            if (dic.ContainsKey(pair.Key))
            {
                if (dic[pair.Key] is Property<int>)
                {
                    int value;
                    if (!int.TryParse(pair.Value, out value)) continue;
                    ((Property<int>)dic[pair.Key]).CurrentValue = value;
                }

                if (dic[pair.Key] is Property<string>)
                {
                    ((Property<string>)dic[pair.Key]).CurrentValue = pair.Value;
                }

                if (dic[pair.Key] is Property<bool>)
                {
                    bool valueIsValid = (pair.Value == "true" || pair.Value == "false");
                    if (!valueIsValid) continue;
                    ((Property<bool>)dic[pair.Key]).CurrentValue = (pair.Value == "true");

                }
            }
        }
        ConcurrentDictionary<string, IProperty> cdic = new ConcurrentDictionary<string, IProperty>(dic);
        return cdic;
    }
}

INIFile.cs

public class INIFile
{
    private string _path;

    public INIFile(string path)
    {
        _path = path;
    }

    public Dictionary<string, string> GetAllValues()
    {
        string s;
        string name;
        string val;

        Dictionary<string, string> map = new Dictionary<string, string>();
        using(StreamReader sr = new StreamReader(_path)
        {
            while (!sr.EndOfStream)
            {
                while ((s = sr.ReadLine() ?? "").Contains("="))
                {
                    name = s.Split('=')[0];
                    val = s.Split('=')[1];
                    map.Add(name, val);
                }
            }
        }
        return map;
    }
}

And finally, the implementation in the actual project;

public static class Properties
{
    public static ConcurrentDictionary<string, IProperty> PropertyList = PropertyFactory.CreateFromFile(@"C:\...\...\settings.ini");
}

I am not satisfied with the current implementation of Property<T>. It requires alot of (un)boxing and that looks ugly. For example;

if((bool)Properties.PropertyList["log_enabled"].GetValue()) //log...

However, I could not come up with a better implementation to map all the properties in one ConcurrentDictionary<>. Is there a better/nicer way to do this? Also, note that the PropertyList has to be accessed by multiple threads; hence my choice of ConcurrentDictionary<>.

\$\endgroup\$
3
\$\begingroup\$

Naming

Variables named s and sr are unhelpful to a maintenance programmer, prefer longer, more explanatory names.

I also prefer to declare my variables where I'm going to be using them (e.g. s inside your first while loop) so that the declaration is never too far from the utilisation.

Var

Use var when declaring local variables when the right hand side of the declaration makes the type obvious.

e.g.

Dictionary<string, string> map = new Dictionary<string, string>();

should be

var map = new Dictionary<string, string>();

You should also always use var when declaring a loop variable.

foreach(var pair in values)

Lines

Lines like this:

propList.Add(new Property<string>(){ DefaultValue = @"C:\...\...", PropertyName = "log_path"});

Could do with expanding across multiple lines, it eases readability and stops you having to scroll.

Result:

propList.Add(new Property<string>()
{ 
    DefaultValue = @"C:\...\...",
    PropertyName = "log_path"
});

Regions

Regions are controversial outside of methods, but whenever I see a region inside a method it's almost a sure-fire guarantee that the contents of that region should be refactored into its own method. You're missing some lines from your original code, so that would be a judgement call left to you.

Generics

As for your complaint:

if((bool)Properties.PropertyList["log_enabled"].GetValue()) //log...

Why on earth does GetValue() even exist?

Why not just use the generic GetActualValue() in your interface?

public interface IProperty
{
    bool IsSet { get; set; }
    string PropertyName { get; set; }
    T GetValue<T>();
}

Then call GetValue<bool>() and have it return a boolean, no boxing or unboxing needed.

If it's the wrong type, either throw an exception, or sort something out with null values.

\$\endgroup\$
  • \$\begingroup\$ T GetValue<T>() was the obvious missing part of my puzzle. I tried T GetValue(), but that would've meant that I needed IProperty<T>, which did not fit in my design. Thanks for that one! I also tend to forget te use var, I'll take it into account. About the regions; there are like 20 lines in total. It was just meant to get it organized, however, they are not neccessary. Would you create methods like CreateIntDefaults() and others there? \$\endgroup\$ – Dion V. Dec 1 '14 at 19:31
  • 1
    \$\begingroup\$ @DionV.: For the defaults, you might even consider just adding the values in the declaration: var propList = new List<IProperty> { new Property<string>() { DefaultValue = @"C:\...\...", PropertyName = "log_path"}, // etc }; perhaps all in a separate List<IProperty> GetDefaults() method, which returns your full defaults collection. I'd argue that such a method is only doing one thing, and therefore needs no more splitting than that. \$\endgroup\$ – Magus Dec 1 '14 at 23:23

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.