current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 7 down vote favorite
1

I have a static class:

public static class ConfigurationDetails
{
    public static string SharepointServer { get; set; }
    public static string Username { get; set; }
    public static string Password { get; set; }
    public static string SharepointDomain { get; set; }
    public static string ServiceLibrary { get; set; }
    public static string SqlServer { get; set; }
    public static string SQLUsername { get; set; }
    public static string SQLPassword { get; set; }
    public static string SQLDomain { get; set; }
}

which needs to be updated from the values of a dictionary.

Using if the code is below:

var configValues = ReadConfiguration();

if (configValues.ContainsKey("SharepointServer"))
{
    ConfigurationDetails.SharepointServer = configValues["SharepointServer"];

}
if (configValues.ContainsKey("ServiceLibrary"))
{
    ConfigurationDetails.ServiceLibrary = configValues["ServiceLibrary"];

}
if (configValues.ContainsKey("Username"))
{
    ConfigurationDetails.Username = configValues["Username"];

}
if (configValues.ContainsKey("Password"))
{
    ConfigurationDetails.Password= configValues["Password"];

}

Now this will do the job, but there would be a lot of ifs. I've tried with LINQ, but this is the closest I've gotten:

configValues.Where(kv => kv.Key == "SharepointServer").ToList().ForEach(kv => ConfigurationDetails.SharepointServer = kv.Value);

and it's not doing any good. Any other ideas, please?

share|improve this question
1  
As a side-note: I wouldn't make configuration a static class –  CodesInChaos Nov 5 '13 at 16:21
    
How much control do you have over the ReadConfiguration method? Could it be refactored to return a ConfigurationDetails instance rather than a dictionary? –  Dan Lyons Nov 5 '13 at 18:14

5 Answers 5

up vote 10 down vote accepted

One solution would be to encapsulate the logic into a method. A problem with that is that there is no simple way to pass a property to the method.

If the case where the key is not in the dictionary means that the value is null, then you could write an extension method for IDictionary that returns the value, if it's present, or null:

public static string GetValueOrDefault(this IDictionary<string, string> dict, string key)
{
    string result;
    dict.TryGetValue(key, out result);
    return result;
}

You would then use it like this:

ConfigurationDetails.SharepointServer = configValues.GetValueOrDefault("SharepointServer");

Another option (assuming the names of properties and keys always match) would be to use reflection to set all the properties at the same time. Something like:

var properties = typeof(ConfigurationDetails).Getproperties();
foreach (var property in properties)
{
    string value;
    if (configValues.TryGetValue(property.Name, out value))
    {
        property.SetValue(null, value);
    }
}
share|improve this answer
up vote 3 down vote

First off: I think, LINQ is of little help.

  1. You could do yourself a favour and change your configuration to a Dictionary and the problem would not occur.

  2. You could do it with the ternary operator:

    ConfigurationDetails.SharepointServer=(configValues.ContainsKey("SharepointServer"))?configValues["SharepointServer"]:default;

    To make things easier, you could wrap it up into a method like svick's solution

  3. You could do it like svick

  4. You could do it automagically, if you drop the idea of a static class, which is by the way nasty, and deal with a configuration object. then, you could leverage reflection to set the according fields, depending on the keys in your dictionary.

share|improve this answer
    
Oooh automagic reflection... I like :) and +1 for static class being nasty! –  Mat's Mug Oct 29 '13 at 15:46
up vote 3 down vote

I would carefully think about the class design.

Do you really want your configuration values to be writable from outside the ConfigurationDetails class?

Is ReadConfiguration a really expensive method? If not, why should you read the configuration once instead of reading it every time when user requests it? [In the current design, how would user force a refresh of configuration?]

Do you really want to return NULL for setting value when it's not configured? I prefer to throw a helpful exception which can help me troubleshoot the problem just in case I forgot to specify a key or mistyped it.

I can not answer the above questions for you but this is what you should think about.

Assuming that ReadConfiguration method is cheap, here's how I would write the ConfigurationDetails class. 

public static class ConfigurationDetails
{
    public static string SharepointServer {  get { return GetConfigValue('SharepointServer'; }

    public static string Username {  get { return GetConfigValue("Username"); }

    public static string Password {  get { return GetConfigValue("Password"); }

    /* Fill in the rest */

    private static string GetConfigValue(string key)
    {
        var configValues = ReadConfiguration();

        if(! configValues.ContainsKey(key))
            throw new ConfigurationErrorsException("Could not find configuration for key " + key);

        return configValues[key];
    }
}
share|improve this answer
    
Alternatively, if the configuration changes infrequently (or not at all), you could change ConfigurationDetails to be an instance class and use a Lazy<T> field for the configuration dictionary. In the case where there are infrequent changes, you can have a method to reset (i.e., re-assign) the field. –  Dan Lyons Nov 5 '13 at 18:16
up vote 0 down vote

Compressed code could look like:

string dictValue;
ConfigurationDetails.SharepointServer = 
    configValues.TryGetValue("SharepointServer", out dictValue) 
    ? dictValue : null;

TryGetValue also sets dictValue to null if it fails, thus no init required on declaration.

Anyway, I would not like a static class, but pass the dictionary to a constructor or Configure(IDictionary ...) method, and also validate the input before taking it into the config.

share|improve this answer
up vote 2 down vote

I know it would not be worth answering this question, but I would like to suggest that using a dictionary and expecting optimization would not be a good idea.

I have tried to refactor my conditions from if/else or switch to dictionary, and you won't believe that it has cost me more resources than a simple if/else.

So you could try to resolve this issue by using Strategy Patterns. That can help you out for such issues.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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