Ayende @ Rahien

My name is Oren Eini
Founder of Hibernating Rhinos LTD and RavenDB.
You can reach me by phone or email:


+972 52-548-6969

, @ Q c

Posts: 6,128 | Comments: 45,551

filter by tags archive

I hate this code

time to read 1 min | 123 words

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.


Jonathan Allen

Why doesn't your settings class support default values and types?

When I read a setting looks more like this:

MemoryCacheExpiration = Settings.ReadTimespan("Raven/MemoryCacheExpiration", new Timespan(0,0,5,0));

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.

Ayende Rahien

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" :-)

Sergey Shumov

You should implement your own configuration section. There is a good tool to help you http://csd.codeplex.com .

Frank Quednau

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(Config.Raven.MemoryCacheExpiration, new Timespan(0,0,5,0));


public struct Config{ public struct Raven{ public const string MemoryCacheExpiration = "Raven/MemoryCacheExpiration"} }


blog escaped my generic signs! I meant;

Settings.Get< int >(key, default);

Harry McIntyre

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.


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

Felipe Fujiy Pessoto

I had a similar problem at my blog settings, I resolved using Enum instead of string and annotated enum values with DefaultValueAttribute:


private enum SettingNames { [DefaultValue(6), Description("MinRequiredPasswordLength")] MinRequiredPasswordLength = 1,

        [DefaultValue(10), Description("PostsPerPage")]
        PostsPerPage = 2,

        [DefaultValue("Blog Name"), Description("BlogName")]
        BlogName = 3,



What about this:

    public class MySettings
        public int MaxPageSize { get; set }

        public ThreadPriority BackgroundTasksPriority { get; set }

        public int MemoryCacheLimitMegabytes { get; set }

        public TimeSpan MemoryCacheExpiration { get; set }

        private void SetDefaults()
            MaxPageSize = 1024;
            BackgroundTasksPriority = ThreadPriority.Normal;
            MemoryCacheLimitMegabytes = GetDefaultMemoryCacheLimitMegabytes();
            MemoryCacheExpiration = TimeSpan.FromMinutes(5);

        private void EnsureValues()
            MaxPageSize = Math.Max(MaxPageSize, 10);

        public Setting(Settings settings)

        private void LoadSettings(settings)
            //TODO: do it by reflection
            throw new NotImplementedException();

    public static string ToFullPath(this string path)
        path = Environment.ExpandEnvironmentVariables(path);
        if (path.StartsWith(@"~\") || path.StartsWith(@"~/"))
            path = Path.Combine(AppDomain.CurrentDomain.BaseDirectory, path.Substring(2));

        return Path.IsPathRooted(path) ? path : Path.Combine(AppDomain.CurrentDomain.BaseDirectory, path);

Sorry ToFullPath is not related

Jeff Paulsen

I 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())

Paul Stovell

I take an opposite approach to configuration - I let the settings be injected into the objects rather than objects asking for settings:




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

Chris Metzl

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.

Adrian Lanning

Similar to mert's code above, here's a simple static method to encapsulate the check for null and type-casting:

        public static T GetConfigValue(string configValue, T defaultValue)
            if (String.IsNullOrEmpty(configValue))
                return defaultValue;

            return ((T)Convert.ChangeType(configValue, typeof(T)));

Could turn it into an extension method off of whatever type your Settings object is to allow code like this:

BackgroundTasksPriority = Settings.GetConfigValue("Raven/BackgroundTasksPriority", ThreadPriority.Normal);

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.

Daniel Marbach

@paul i used a similar approach in the appccelerate bootstrapper: www.appccelerate.com. Look for the extensionconfigurationsectionbehavior


Erik Juhlin

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("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("Raven/BackgroundTasksPriority").OrThrowException();

Erik Juhlin

Line breaks got messed up... :(



+1 Harry McIntyre.

Felice Pollano

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 ?

M. Schopman

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.

Jon Sequeira

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

Comments have been closed on this topic.


No future posts left, oh my!


  1. The design of RavenDB 4.0 (14):
    26 May 2016 - The client side
  2. RavenDB 3.5 whirl wind tour (14):
    25 May 2016 - Got anything to declare, ya smuggler?
  3. Tasks for the new comer (2):
    15 Apr 2016 - Quartz.NET with RavenDB
  4. Code through the looking glass (5):
    18 Mar 2016 - And a linear search to rule them
  5. Find the bug (8):
    29 Feb 2016 - When you can't rely on your own identity
View all series


Main feed Feed Stats
Comments feed   Comments Feed Stats