Nov 11, 2010

Don't use constants

I don’t really remember when, but inside my current team I have told that I really hate to see classes that are like this:

public class Contants
{    
    public class Settings
    {
        public const ConnectionStringName = "SomeProject.ConnectionString";    
    }
    public class FieldNames
    {
        public const SomeField = "MyField";
    }
}

Almost all of the projects that I saw had this kind of class. Usage of it seams to be obvious – to store reusable strings to remove repetition.

But for me such a classes are lacking of the one thing – intention. Each constant exists inside our application for some reason and Constants class completely removes context that is connected to its value.

Today we had a some kind of “real world” scenario where I can show what to do with constants. I our case we had an email template where system will substitute some user input. So template contains %UserName%, %Reason% and so force string patterns that are going to be replaced with some values. The most obvious storage for these patterns is Constants class, but we don’t want it :)! So what I think we should do is to create a set of extension methods like this:

static class StringExtensions
{
    public static string SubstituteUserName(this string template, string userName)
    {
        return template.Replace("%UserName%", userName);
    }
}

This approach will allow us easily to write unit tests, because method is separated from others and really simple, and it will allow to write code in the following way:

"Hello %UserName%, thanks for your message about %Reason%"
                      .SubstituteUserName(userName)
                      .SubstituteReason(reason);

I think its really readable and looks nice.

The next thing why such an approach works is the way how developer works with the legacy code. In legacy system it is really often for developer to perform text based search to find places where similar tasks were already solved. For example when you need to get some value from the web.config file you can either use ConfigurationManager or WebConfigurationManager or maybe in your project some custom wrapper is implemented. For me the easiest way to find such information is to grab existing key of some value and try find all the references on it. In the good project you will meet only one place of usage :). In other cases it may be two places – Constants class and some domain specific wrapper (in this case why do need this constant separately of its usage?). But if you find that this constant is reused in 100 places and there is no common pattern you will just add the 101th place and forget about it. The reason why that happened is the existence of this Constans class.

So my approach for constants is that they can be only private in some classes that are using them. This allows new developers easy way to understand why those values are here and how to use them.

10 comments:

  1. "real world" example is just lol...

    {{ mustache }} anyone? (https://github.com/jdiamond/Nustache)

    but despite the lousy example constants-storage-classes are still bad anyway :)

    ReplyDelete
  2. Thats why it is in quotes :)

    ReplyDelete
  3. and what about {{ mustache }} anyone? (https://github.com/jdiamond/Nustache) ?

    ReplyDelete
  4. mustache is a logic-less templates which are far better for your "real world" example. (nustache is a .NET fork)

    ReplyDelete
  5. This example is EXAMPLE how can we avoid to have constant containers...

    ReplyDelete
  6. and how this approach could be applied to ConnectionStrings, MyField, etc.?

    the problem with example is that constants are used in many absolutely different use-cases. and the ways to avoid using constant-storage-class (CSC) are different each time.

    besides your example easily allows user to use this CSC but inside the extension class. i mean:

    return template.Replace(CSC.EmailTemplatePatterns.UserName, userName);

    CSCs are widely spread as CSC has one great advantage - it is a single point of failure - all constants are stored in 1 place.

    and the only way to really fight CSCs is to make people not to fear string literals all over the code (hello TDD!).

    ReplyDelete
  7. 2 COTOHA
    I believe that the point was to raise some driving force that will make some of us think a little more, while creating "constants-storage-classes", not a create mandatory rule. Having that we should consider this like a code-smell. Right?

    "and the only way to really fight CSCs is to make people not to fear string literals all over the code (hello TDD!). "

    TDD doesn't save us from this. And having several failing tests because of one constant change is bad and called "fragile test". And also with constant we have single place of change.

    ReplyDelete
  8. Yes the I just wanted to highlight one more place where we should think a little bit further then used to

    ReplyDelete
  9. 2 Restuta
    1. when test fails this doesn't mean it should be redone. in most cases the code should be fixed. this is the case with this particular example.

    2. with constants, that are not grouped into class/namespace you have no single point of change. (did i get you right?)

    2 Sly
    there is no need to actually "think" about using such constants. there are well-known ways dealing with them:
    1. resource files for short strings
    2. templates for long texts
    3. config files for settings
    4. class-definitions for domain-specific constants

    maybe there is something else but i can't remember right now :)

    ReplyDelete
  10. 2 COTOHA
    1. Yep, it doesn't mean it should be redone. But you should _understand_ what should be fixed in code. It is better to achieve with only one failing test for one reason than with bunch of them.

    2. I thinks yes.

    > 1. resource files for short strings
    Boring and often isn't needed, in this case resources becomes constant classes, what is the difference if you have only one language?

    > 4. I was sure that we are talking about this "class-definitions" in this topic.

    ReplyDelete