I hate this code
I really hate this code, it is so stupid it makes my head hurt, and it have so much important factors in it. In particular, there is a lot of logic in here.
You might not see it as such, but a lot of this is actually quite important, default values, config parsing, decisions.
This is important. And it is all handled in a a few methods that goes on forever and hide important details in the tediousness of parameter unpacking.
This approach works if you have 5 parameters, not when you have 50.
Comments
Why doesn't your settings class support default values and types?
When I read a setting looks more like this:
Also, don't store timespan configs as seconds, as someone will always assume that they are in milliseconds or minutes. Instead use strings that are compatible with TimeSpan.Parse. For example, "0:5" is unquestionably 5 minutes.
Why don't you have a default settings file, that is overridable, and you fall back to this default settings file when you don't find the value in the real settings file.
With this approach you could easily see your default settings in that file, and your code would be much more readable also:
var backgroundTasksPriority = Settings["...] ? DefaultSettings["..."]; BackgroundTasksPriority = (ThreadPriority)Enum.Parse(..., backgroundTasksPriority);
Clean, and simple.
Palesz, Because it isn't just default settings. It is settings that change other settings as well.
I have tried to address this issue (in C#) with a generic configuration loader that can give you strongly typed objects, from config file. https://github.com/twistedtwig/CustomConfigurations
@Jonathan Allen: i like your "is unquestionably minutes" :-) ... it's the opposite of your previous line "someone will always assume that they are in milliseconds or minutes" :-)
My approach? When there may be doubts, just be clear! Use a setting name like "Raven/MemoryCacheExpirationInSec" or "Raven/MemoryCacheExpirationInMilliSecs" or "Raven/MemoryCacheExpirationInMs" or something like that.
that, btw, is what Oren does just 2 lines after with "Raven/MemoryCacheLimitMegabytes" :-)
You should implement your own configuration section. There is a good tool to help you http://csd.codeplex.com .
wrt clear timespan definitions, there are very nice static methods on TimeSpan which state what is happening: TimeSpan.FromMinutes(2);
Well this also seems pretty obvious to me as;
int timeout = Settings.Get<int>(Config.Raven.MemoryCacheExpiration, new Timespan(0,0,5,0));
where;
public struct Config{ public struct Raven{ public const string MemoryCacheExpiration = "Raven/MemoryCacheExpiration"} }
blog escaped my generic signs! I meant;
Settings.Get< int >(key, default);
One nasty issue with this, is that you are mixing concerns from all over the app into this Init file - everything needs to access it to find out the settings.
I would engineer this to bits - break each setting into a class inheriting from 'Setting' (a base class which knows how to read a setting), and use the class name as a convention to access the settings repository.
e.g.
void Main() { new MaxMemoryConsumptionInMbSetting().Value.Dump(); }
abstract class Setting { public string Value { get { return Settings[this.GetType().Name.Replace("Setting", "")]; } } }
class MaxMemoryConsumptionInMbSetting : Setting { public new int Value { get { return int.Parse(base.Value); } } //can have custom parse logic here, maybe a default, maybe return a class instance. }
static IDictionary<string, string> Settings = new Dictionary<string, string>(){ {"MaxMemoryConsumptionInMb", "213"} }; //your settings repository, be it ConfigurationManager.AppSettings or
I had a similar problem at my blog settings, I resolved using Enum instead of string and annotated enum values with DefaultValueAttribute:
http://fujiyblog.codeplex.com/SourceControl/changeset/view/f03b98a5a273#src%2fFujiyBlog.Core%2fEntityFramework%2fSettingRepository.cs
private enum SettingNames { [DefaultValue(6), Description("MinRequiredPasswordLength")] MinRequiredPasswordLength = 1,
....
What about this:
Sorry
ToFullPath
is not relatedI think Andres has the right idea, but I would be inclined to have a ordered collection of tuples - each with settings key, a lambda for the default, a lambda for the load (mostly just type casting), and a lambda for any coercion that happens after setting. The basic idea that this is done in three passes - all defaults, all loads, then all coercion. Having & maintaining an explicit order that all this happens in is a good thing once you have settings coercion that depends on other settings.
The thing that sticks out to me here is the repetition. That smell indicates that something is missing, and causes the semantics of the program to be lost in the syntax. What happens if a setting comes back from the xml file as an empty string instead of null? Now you've got to fix it for each setting. That won't be fun.
I find the silent adjustment of the MaxPageSize setting to be strange as well. If you set it to less than ten, you wouldn't be able to tell why your setting wasn't being used without looking through the code. It seems that there should at least be a log entry or perhaps even an exception being raised there.
I would also be interested in knowing why some settings have hard coded defaults, but others require a function to build the default (like GetDefaultMemoryCacheLimitMegabytes())
I take an opposite approach to configuration - I let the settings be injected into the objects rather than objects asking for settings:
http://www.paulstovell.com/convention-configuration
Paul
I got bored of dealing with different types, default values, optional values, conversions etc. and I wrote small type safe wrapper around the default .net configurationmanager https://github.com/tparvi/appsettings
As I'm lazy I also added support for attributes so that I don't actually have to write code to read or write those settings https://github.com/tparvi/appsettings/wiki/Attribute-usage-examples
I understand the issue with this kind of grinding code. Possibly a better solution would be a parameter parser that could handle several different types of parameters? I've written such things in the past.
Similar to mert's code above, here's a simple static method to encapsulate the check for null and type-casting:
<pre> public static T GetConfigValue<T>(string configValue, T defaultValue) { if (String.IsNullOrEmpty(configValue)) return defaultValue; return ((T)Convert.ChangeType(configValue, typeof(T))); } </pre>Could turn it into an extension method off of whatever type your Settings object is to allow code like this:
Would not recommend adding a Func for coercion as I think its clearer to do it on a separate line as you do in your MaxPageSize example.
@paul i used a similar approach in the appccelerate bootstrapper: www.appccelerate.com. Look for the extensionconfigurationsectionbehavior
Daniel
My idea of how to simplify and make the code more readable: https://gist.github.com/2588735 With it you can write: Settings.GetInt32("Raven/MaxPageSize").Or(1024); Settings.GetEnum<ThreadPriority>("Raven/BackgroundTasksPriority").Or(ThreadPriority.Normal); Settings.GetInt32("Raven/MemoryCacheLimitMegaBytes").Or(GetDefaultMemoryCacheLimitMegaBytes); Settings.GetTimeSpanFromSeconds("Raven/MemoryCacheExpiration").Or(TimeSpan.FromMinutes(5)); or Settings.GetInt32("Raven/MemoryCacheLimitMegaBytes").OrDefault(); Settings.GetEnum<ThreadPriority>("Raven/BackgroundTasksPriority").OrThrowException();
Line breaks got messed up... :(
https://gist.github.com/2588735
+1 Harry McIntyre.
Apart it is boring, it throws if the string does not represent an integer. Is this the desired behavior? Wouldn't be better to default even if we write some rubbish, maybe warning in the log about the offending configuration line ?
Just keep it and love the positive side of it. It is perfectly readable now and creating abstractions just introduces more complexity. It also works with 50 parameters, but now you have them all in one place instead of 50 seperate locations in a way that is probably more esthetic but unreadable.
I took a swing at this problem in a similar way to what Andres suggested, with a little MEF wrapped around it to avoid an extra assembly dependency in lower-level components: http://jonsequitur.com/2012/02/15/kill-your-config-files/
An interesting secondary effect of this approach is that you can then export your application's configuration "contract" using a little reflection, and generate a configuration template, e.g. an Azure .csdef file.
Comment preview