Ayende @ Rahien

Refunds available at head office

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.

image

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.

Tags:

Posted By: Ayende Rahien

Published at

Originally posted at

Comments

Jonathan Allen
05/03/2012 09:10 AM by
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.

Palesz
05/03/2012 09:23 AM by
Palesz

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
05/03/2012 09:29 AM by
Ayende Rahien

Palesz, Because it isn't just default settings. It is settings that change other settings as well.

Jon
05/03/2012 09:33 AM by
Jon

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

njy
05/03/2012 09:41 AM by
njy

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

njy
05/03/2012 09:43 AM by
njy

that, btw, is what Oren does just 2 lines after with "Raven/MemoryCacheLimitMegabytes" :-)

Sergey Shumov
05/03/2012 10:24 AM by
Sergey Shumov

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

Frank Quednau
05/03/2012 10:44 AM by
Frank Quednau

wrt clear timespan definitions, there are very nice static methods on TimeSpan which state what is happening: TimeSpan.FromMinutes(2);

mert
05/03/2012 10:48 AM by
mert

Well this also seems pretty obvious to me as;

int timeout = Settings.Get(Config.Raven.MemoryCacheExpiration, new Timespan(0,0,5,0));

where;

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

mert
05/03/2012 10:50 AM by
mert

blog escaped my generic signs! I meant;

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

Harry McIntyre
05/03/2012 11:21 AM by
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.

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

Felipe Fujiy Pessoto
05/03/2012 11:50 AM by
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:

http://fujiyblog.codeplex.com/SourceControl/changeset/view/f03b98a5a273#src%2fFujiyBlog.Core%2fEntityFramework%2fSettingRepository.cs

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

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

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

....

Andres
05/03/2012 01:31 PM by
Andres

What about this:

    public class MySettings
    {
        [SettingsKey("Raven/MaxPageSize")]
        public int MaxPageSize { get; set }

        [SettingsKey("Raven/BackgroundTasksPriority")]
        public ThreadPriority BackgroundTasksPriority { get; set }

        [SettingsKey("Raven/MemoryCacheLimitMegabytes")]
        public int MemoryCacheLimitMegabytes { get; set }

        [SettingsKey("Raven/MemoryCacheExpiration")]
        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)
        {
            SetDefaults();
            LoadSettings(settings);
            EnsureValues();
        }

        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);
    }
Andres
05/03/2012 01:32 PM by
Andres

Sorry ToFullPath is not related

Jeff Paulsen
05/03/2012 02:07 PM by
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.

Chris
05/03/2012 02:11 PM by
Chris

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
05/03/2012 02:16 PM by
Paul Stovell

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

Toni
05/03/2012 02:27 PM by
Toni

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
05/03/2012 02:44 PM by
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
05/03/2012 04:25 PM by
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
05/03/2012 04:44 PM by
Daniel Marbach

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

Daniel

Erik Juhlin
05/03/2012 08:01 PM by
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
05/03/2012 08:06 PM by
Erik Juhlin

Line breaks got messed up... :(

https://gist.github.com/2588735

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));
Settings.GetInt32("Raven/MemoryCacheLimitMegaBytes").OrDefault();
Settings.GetEnum("Raven/BackgroundTasksPriority").OrThrowException();
Afif
05/04/2012 04:00 AM by
Afif

+1 Harry McIntyre.

Felice Pollano
05/04/2012 10:41 AM by
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
05/04/2012 08:25 PM by
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
05/08/2012 07:50 PM by
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.

Comments have been closed on this topic.