Are string constants overrated?

Question

It's easy to lose track of odd numbers like 0, 1, or 5. I used to be very strict about this when I wrote low-level C code. As I work more with all the string literals involved with XML and SQL, I find myself often breaking the rule of embedding constants in code, at least when it comes to string literals. (I'm still good about numeric constants.)

Strings aren't the same as numbers. It feels tedious and a little silly to create a compile-time constant that has the same name as its value (E.g. const string NameField = "Name";), and although the repetition of the same string literal in many locations seems risky, there's little chance of a typo thanks to copying and pasting, and when I refactor I'm usually doing a global search that involves changing more than just the name of the thing, like how it's treated functionally in relation to the things around it.

So, let's say you don't have a good XML serializer (or aren't in the mood to set one up). Which of these would you personally use (if you weren't trying to bow to peer pressure in some code review):

static void Main(string[] args)
{
    // ...other code...

    XmlNode node = ...;

    Console.WriteLine(node["Name"].InnerText);
    Console.WriteLine(node["Color"].InnerText);
    Console.WriteLine(node["Taste"].InnerText);

    // ...other code...
}

or:

class Fruit
{
    private readonly XmlNode xml_node;

    public Fruit(XmlNode xml_node)
    {
        this.xml_node = xml_node;
    }

    public string Name
    { get { return xml_node["Name"].InnerText; } }

    public string Color
    { get { return xml_node["Color"].InnerText; } }

    public string Taste
    { get { return xml_node["Taste"].InnerText; } }
}

static void Main(string[] args)
{
    // ...other code...

    XmlNode node = ...;
    Fruit fruit_node = new Fruit(node);

    Console.WriteLine(fruit_node.Name);
    Console.WriteLine(fruit_node.Color);
    Console.WriteLine(fruit_node.Taste);

    // ...other code...
}

Answer 1

As you probably guessed. The answer is: it depends on the context.

It depends on what the example code is part of. If it's just part of a small throw away system then hard coding the constants may be acceptable.

If it's part of a large, complex system and the constants will be used in mulitple files, I'd be more drawn to the second option.

Answer 2

As in many matters of programming, this is a matter of taste. The "laws" of proper programming were created from experience -- many people have been burned by global variables causing namespace or clarity problems, so Global Variables Are Evil. Many have used magic numbers, only to later discover that the number was wrong or needed changing. Text search is ill-suited to changing these values, so Constants In Code Are Evil.

But both are permitted, because sometimes they aren't evil. You need to make the decision yourself -- which leads to clearer code? Which is going to be better for maintainers? Does the reasoning behind the original rule apply to my situation? If I had to read or maintain this code later, how would I rather that it were written?

There is no absolute law of good coding style, because no two programmers' minds works exactly alike. The rule is to write the clearest, cleanest code that you can.

Answer 3

Personally, I'd load the fruit from the XML file in advance - something like:

public class Fruit
{
    public Fruit(string name, Color color, string taste)
    {
        this.Name = name;  this.Color = color; this.Taste = taste;
    }

    public string Name { get; private set; }
    public Color Color { get; private set; }
    public string Taste { get; private set; }
}

// ... In your data access handling class...
    public static FruitFromXml(XmlNode node) 
    { 
            // create fruit from xml node with validation here 
    }
}

That way, the "fruit" isn't really tied to the storage.

Answer 4

For something like that it depends on how often the constant is used. If it's just in one place as per your example, then hard-coding is fine. If it's used in many different places, definitely use a constant. One typo could lead to hours of debugging if you're not careful, because your compiler isn't going to notice that you typed "Tsate" instead of "Taste", while it WILL notice that you typed fruit_node.Tsate instead of fruit_node.Taste.

Edit: I see now that you mentioned copying and pasting, but if you're doing that you may also be losing the time you save by not creating a constant in the first place. With intellisense and auto-completion, you could have the constant out there in a few keystrokes, instead of going through the trouble of copy/paste.

Answer 5

A defined constant is easier to refactor. If "Name" ends up being used three times and you change it to "FullName", changing the constant is one change instead of three.

Answer 6

I'd go with the constants. It is a little more work, but there is no performance impact. And even if you usually copy/paste the values, I've certainly had instances where I changed code when I typed and didn't realize that Visual Studio had focus. I'd much prefer these resulted in compile errors.

Answer 7

For the example given, where the Strings are used as keys to a map or dictionary, I would lean toward use of an enum (or other object) instead. You can often do much more with an enum than with a constant string. In addition, if some code is commented out, IDE's will often miss that when doing a refactor. Also, references to a String constant that are in comments may or may not be included in a refactor.

I will make a constant for a string when the string will be used in many locations, the string is long or complicated (such as a regex), or when a properly-named constant will make the code more obvious.

I prefer my typos, incomplete refactorings, and other bugs of this sort to fail to compile rather than to just fail to operate properly.

Answer 8

Like many other refactorings, it's an arguably optional additional step that leaves you with code that's less risky to maintain and is more easily grokked by the "next guy". If you're in a situation that rewards that kind of thing (most that I'm in do), go for it.