Nick Hodges

Delphi Pretty Good Practices #5 – Don’t Use Literals

09 Jun

If there is a bedrock, bottom line, everyone-should-follow-it-all-the-time rule in programming it is “The DRY Principle” – Don’t Repeat Yourself.  This simple rule states that you should write code once and only once, and that you shouldn’t allow the same code to be used all over the place.  Or, as the Wikipedia article deftly states: "Every piece of knowledge must have a single, unambiguous, authoritative representation within a system."  Put more simply, it means that you shouldn’t have the same thing repeated all over your code, but that you should create a single identifier and use that in the many places where it might be needed.

As a practical matter, the DRY principle in its most basic form tells us that we should, as a matter of course, not use literals in our code, but instead should declare constants (or resourcestring types as we’ll discuss in a minute) and use those instead. That way, if you need to change the value for something, you can do it in a single place instead of having to make multiple, error-prone changes throughout your existing code.

For example:  Say you have a system that has an arbitrary number of required repetitions, say 17.  You might end up writing a whole bunch of code like this:


for i := 1 to 17 do
begin
  ProcessStuff;
  DoSomeMoreStuff;
end;

Now imagine that you have that kind of code all over the place, and then your boss comes around and says "Hey, we need to repeat all that stuff 18 times now, not just seventeen.” Well, if you haven’t followed the DRY Principle, you could very well end up with a lot of code to change.  And don’t use search and replace, because what if you change the 17 that is part of a variable name or something?  Things could get ugly fast.

Of course, the thing to do is to declare a constant:


const
  NumberOfRepetitions = 17;

and declare your loops as


for i := 1 to NumberOfRepetitions  do
begin
  ProcessStuff;
  DoSomeMoreStuff;
end;

and now when your boss switches things up, you have but one simple change to make, and all is well.

Now this is a pretty simple, basic thing to do, but I’m constantly surprised at how often I find myself forgetting to do it.  (For instance, if you look through the code for TextScrubber, you’ll probably notice that I need to apply the DRY Principle to the TVersionInfo constructor in uTextScrubberTypes.pas.) You might be surprised how many literals you use in your code. I often take advantage of the syntax highlighting feature to scan code for specific colors for strings and numbers and replace them with constant values as much as possible. For instance, some code from TextScrubber used to look like this:


procedure TStraightTextMainForm.InitializeMainFormInformation;
var
  IniFile: TIniFile;
begin
  IniFile := TIniFile.Create(IniFileName);
  try
    TextScrubberOptions.ClickChoice := TClickChoice(
      IniFile.ReadInteger('Options', cClickChoice, 0));
      TextScrubberOptions.ShouldTrim := IniFile.ReadBool(cOptions,
      'ShouldTrimText', False);
  finally
    IniFile.Free;
  end;
end;

with a bunch of string literals.  Instead, now, I’ve declared two constants in the uTextScrubberConsts.pas unit


const
  cOptions = 'Options';
  cClickChoice = 'ClickChoice';
  cShouldTrimText = 'ShouldTrimText';
  cVersionLangCodePage = '040904E4';

and the new code looks like this:


procedure TStraightTextMainForm.InitializeMainFormInformation;
var
  IniFile: TIniFile;
begin
  IniFile := TIniFile.Create(IniFileName);
  try
    TextScrubberOptions.ClickChoice := TClickChoice(
      IniFile.ReadInteger(cOptions,   cClickChoice, 0));
    TextScrubberOptions.ShouldTrim := IniFile.ReadBool(cOptions,
      cShouldTrimText, False);
  finally
    IniFile.Free;
  end;
end;

Those constants are also used when I write out information to the INI file, so that I can change the value in one place if I need to, and so that I can know that there won’t be any typographical errors in my strings that will cause a bug.  The same string value is always going to be used for the INI file entry.

Now let’s take a look specifically at strings.  Strings are a bit special because they are very often used to communicate information, and as such, they frequently need to be translated into other languages through the process of “localization”.  Windows provides an easy way to do this via string resources, and Delphi provides an easy way to create string resources via the resourcestring identifier.  These strings are then created as resources, making them easy to translate.  And “easy to translate” can often be, well, translated into “cheaper to translate” and that is a good thing.

Practically speaking, I apply this principle by placing as many const and resourcestring declarations as I can in a single file, thus centrally locating them for easy management and translation.  In our case with the TextScrubber project, I’ve created a file called uTextScrubberConsts.pas and put constants and resource string values into it.  If you look through the project code, you’ll probably notice that I need to apply the DRY Principle to the TVersionInfo constructor in uTextScrubberTypes.pas, even if it is keeping those constants within the unit.

One question that might get asked is “How do you choose what to make a const and what to make a resourcestring?”  My answer is that as a general rule, any string that could change without changing the UI or the functionality of the code, make a const.  Any string that, if changed, will trigger a change in UI or translation should be made a resourcestring.  There are exceptions to that, of course. More simply, a rule to use that anything that might get translated should be a resourcestring.

Now, this is a pretty simple “pretty good practice” to follow, but for you folks who have not been following this principle,  simply doing so can make your code more readable and maintainable.

18 Responses to “Delphi Pretty Good Practices #5 – Don’t Use Literals”

  1. 1
    John Jacobson Says:

    I’ve long been impressed with how code can be normalized using many of the same principles as database normalization. The DRY principle applies just as well to both.

  2. 2
    RichardS Says:

    Indeed, some good Delphi Programming tips there, of interest to all those who are interested in Delphi Programming.

  3. 3
    Xepol Says:

    ya, I don’t always agree with this. First: creating needless constants for one time use magic numbers is pointless, just encourages the compiler to slow down and blow up as the number of defined constants approaches infinity. Besides, if you have a single use magic number, you really have to ask yourself why. Chances are that its meaningful, and 17 is MUCH more meaningful in context than "MyMagicNumber" - also, the 17 is likely more readable.

    If the number gets reused a LOT, you still have to ask yourself why, and whether you are doing things the right way (are you guessing how many elements should be in an array/collection/tlist? Is the number significant in context, is it likely to change?).

    Some numbers get used enough and in the right way to make them constants. Everything else just makes the code harder to read and maintain.

    Secondly, your string constants. I can’t even begin to start with how wrong what you suggesting is. By moving the actual string away from its real context you make it that much easier to use in a second context. A second context that might break when you change it for the first context (it happens) - leading to subtle and hard to understand glitches.

    I’m not saying that constants are bad, but I am saying that using them EVERYWHERE for everything is. Like everything else, moderation is the key - don’t make your code harder to read and maintain by being overly compulsive.

  4. 4
    Jolyon Smith Says:

    Then your boss says… we need to repeat these iterations 18 times in these few cases, but these other cases need to stay at 17…. the DRY principle applies, but I suggest that "One often repeated literal == one constant to replace them all" is not always correct. In this case, and so often in other cases, a degree of clarification to contextualise the constant provides a further improvement:

    const
    ITER_Default = 17;
    ITER_ProcessA = ITER_Default;
    ITER_ProcessB = ITER_Default;
    ITER_ProcessC = 18;

    Essentially it extends the DRY principle one degree further, in eliminating the repetition of even the single CONSTANT throughout the code.

    (Not always applicable, obviously, but something that should be considered when CONSTing a literal… is it truly a "universal constant" or is it only potentially contextually/coincidentally constant)

  5. 5
    Nick Hodges Says:

    Xepol –

    The key is to give the constants meaningful names - I should have stressed that more.

  6. 6
    Xepol Says:

    Nick -> I took that as a given, my position stands. Like any code technique, it can be abused to the point that you actually cause new problems, and often worse problems.

    I’m not against using constants, but it actually has to solve a problem. Too often this leads to problems seen on TheDailyWtf where some moron takes your message to heart, and suddenly all your code reads:

    For MyLoopCounter := Zero To Seventeen+MinusOne Do

    And then, you go and check the actual constants and find another moron has done something like this:

    Const
    Zero = 1;
    Seventeen = 45;
    MinusOne = 3;

    And oddly, the whole project still holds together until you try to change a single value somewhere.

    (For those who say this is an unrealistic example, I leave you to look up the phrase "a baker’s dozen")

    I stand by my position: If you are using magic numbers, you need to be extra sure you are doing things the RIGHT bloody way. PI is a pretty good magic number - it has a very specific usage. A constant to hold the number 17, well that is another question entirely, particularilly if you need to use it in a dozen different locations. Maybe it actually refers to the number of elements in an array, like the # of colors in a list.

    And back to strings… Unless you are trying to internationalize something, the chances of that string being meaninful in a global sense approaches zero RAPIDLY, so why make your code harder to read.

    Unless you want to prove your Uber code skills with code like:

    Const
    ThisIsMyStringConstant = ‘This Is My String Constant’;
    ThisIsMyStringConstant2 = ‘This Is My String Constant!’; // Darn punctuation!

    Constants have their place. Everywhere isn’t it.

  7. 7
    Eric Says:

    The "one constant to rule them all" principle is evil.

    It’s a very, very common beginner mistake, especially when the constant is dissociated from its usage, say in a different file, or in a header far removed from the code using it, as the constant will then be used for a variety of situations, will often be improperly named (such as in your "NumberOfRepetitions" exemple).

    It takes a high level of consistency and control to properly handle constants, so as a general principle, just don’t do it unless you’re really sure you’re doing it right in the first place (and if you’re not designing an API, chances are you’re doing it wrong with your constants).

    Practically, this isn’t so much of an issue, as outside of strings (which usually undergo a specific localization logic), you only really need constants for… maths. The rest of the time, object properties, methods, array lengths, etc. are the way to go and you shouldn’t be using those pesky literals NOR constant in the first place.

    If you’re reusing a constant all over the place, *that* is your issue, not the constant or literal itself, and *that* is the true meaning of the wikipedia sentence IMHO.

  8. 8
    Eric Says:

    > The key is to give the constants meaningful names - I should have stressed that more.

    Properly naming stuff with global or merely broad scope is one of the hardest problems in development.

  9. 9
    John Says:

    Dont forget the third update for 2010.

  10. 10
    m. Says:

    I think what Nick said was right about numeric constats, just the "NumberOfRepetitions" is bad example.

    For example
    code1:
    const
    NumberOfDirectors = 10;

    for i := 1 to NumberOfDirectors do
    begin
    ProcessStuff;
    DoSomeMoreStuff;
    end;

    code2:
    // iterate over the number of the directors
    for i := 1 to 10 do
    begin
    ProcessStuff;
    DoSomeMoreStuff;
    end;

    In the first code snippet no additional comments are needed. The second code needs extra comment to explain what ten represents and that comment is needed every time when the number of the directors is used. Also replacing ten with something else becomes more difficult if it’s used in several places (maybe even in several units).

    I think using string constants is more complicated, because they are usually self explanatory and depend on the context. Localization is one of the cases where string constants may be a good idea, also if the string value isn’t self explanatory or the value is just very long and needs to be used in several places.

    By the way I’m pretty sure that simple constants (unless you have tens of thousands of them) doesn’t slow down compiler very much.

  11. 11
    Louis Kleiman Says:

    When you put in the contants for the names on the configuration variables, for left out constants for the default values:

    bDefaultShouldTrimValue = False;
    iDefaultClickChoice = 0;

  12. 12
    C J Hart Says:

    Nick,Good tips. I have done many of these items without thinking for years. I am not quite as good with strings, but don’t we all need improvement. I am also a big fan of being database driven as much as possible. Especially constant values, data validation codes such as valid answers are A,C,E,U etc. This ends lots of Maint programming and puts control and responsibility in the users hands. A little more hassle up front, but get some DB Lookup and validation functions working and it is much easier after that.

  13. 13
    Warren Says:

    My #1 reason not to use "magic values" in my code (literal numbers especially) is that it is very poorly documented.

    I found a lot of stuff like this recently, in one of my applications:

    if ModuleA.SomeProperty then
    SomeComponent.ImageIndex := 17
    else
    SomeComponent.ImageIndex := 18;

    The developer who wrote stuff like this is no longer with us. ;-) How much more clear it is to read:

    if ModuleA.SomeProperty then
    SomeComponent.ImageIndex := ImageShowingThingsAreGood
    else
    SomeComponent.ImageIndex := ImageShowingThingsAreBad;

    W

  14. 14
    David Moorhouse Says:

    I agree that context is the key. I always use const when accessing INI files, as this prevents typos or keyname changes from blowing things up.

    Descriptively named integer consts are a good thing ™ too e.g. I use a const named OneSecond (value = 1000) in a thread that checks for file changes in a directory once every second.

  15. 15
    Alan Clark Says:

    @David Moorhouse: FYI, there is a predefined constant called MSecsPerSec in SysUtils.pas which is the same value.

  16. 16
    Alexandre Machado Says:

    @Xepol: I can’t agree with you more! Constant abuse is much worse.

  17. 17
    Olaf Monien Says:

    I agree that "magical" numbers should be avoided and constants with **meaningful** names should be used instead. Because of its nonsense name NumberOfRepetitions is more evil in this case actually.

    But appart of that the code example shows an other bad design. As Nick took it from his TextScrubber project, it is apparently not artificial:

    InitializeMainFormInformation is obviously implemented in a Form unit (TStraightTextMainForm) , but does not have any relationship to Form/visual code at all. I assume that even this business logic class "TextScrubberOptions" is implemented in the Form unit. It should instead go into its own file, and visual classes (such as that Form) should just reference it.

    I know that many Delphi projects start exactly like that - mixing visual and business code - and later it gets harder and harde to get your code organized. This is certainly because it’s just "so easy" and all beginner examples work like this. If we are talkign about coding style/design, then let’s scrub the examples completely ;-)

    procedure TStraightTextMainForm.InitializeMainFormInformation;
    var
    IniFile: TIniFile;
    begin
    IniFile := TIniFile.Create(IniFileName);
    try
    TextScrubberOptions.ClickChoice := TClickChoice(
    IniFile.ReadInteger(’Options’, cClickChoice, 0));
    TextScrubberOptions.ShouldTrim := IniFile.ReadBool(cOptions,
    ‘ShouldTrimText’, False);
    finally
    IniFile.Free;
    end;
    end;

  18. 18
    基础编程习惯的问题 | 创意纪 Says:

    [...] 看到这里 http://blogs.embarcadero.com/nickhodges/2010/06/09/39453 [...]

© 2014 Nick Hodges | Entries (RSS) and Comments (RSS)

Your Index Web Directorywordpress logo
Close