Programmer Musings

Magic Constants are bad

A truly bad code example in a book on Java Servlets got me thinking about the idea of Magic Constants. Of course, we are all aware of the problem of magic numbers or magic literals in code. That's why we use symbolic constants instead to hide that implementation detail (and to simplify maintenance later).

However, I'm not talking about literals, I'm talking about symbolic constants that are used in a way as bad, or worse, than the original literals.

The example in the book was looking at the response code from an HTTP request. If you are familiar with HTTP, you know that the values from 200 to 299 are the success codes. Now obviously, we don't want to put those raw literals in our code. So we shuold use symbolic constants instead.

The book contained the following code fragment:


if (status >= HttpURLConnection.HTTP_OK ||
status < HttpURLConnection.HTTP_MULT_CHOICE) {
...


One look at this code and I finally had a name for a bad practice I'd seen many times in my career. I decided on Magic Constants. In this case, the constants are used exactly the way the original literals would have been. HttpURLConnection.HTTP_OK has the value 200 and HttpURLConnection.HTTP_MULT_CHOICE has the value 300. To understand the code, you need to know that the first value above the successful codes is HttpURLConnection.HTTP_MULT_CHOICE.

This code relies on the relative ordering of the two constants in a way that is dependent on their current implementation. If W3C ever decided to change the range of successful values or move the Multiple Choices response code, code like this could suddenly act very differently.

Unfortunately, this code has a bug that would have been more obvious if we had kept the original literals. Without the constants the code is


if (status >= 200 || status < 300) {
...


From this, it's a little more obvious that the condition will always be true. The OR condition should have been an AND. So obviously, this practice has generated code that is easier to get wrong and more fragile as well.

Before, I go any farther, I'd like to say that I do not mean to abuse these authors in general. They just happened to write the piece of code that shows a practice I've come to believe is wrong. I have seen variants of this problem for most of the 20-odd years I've been programming.

I have seen many cases where someone borrowed a constant that happened to have the right value without regard for whether or not the new use of the constant and the old use had any relationship. This leads to code that is almost as bad as the original with the magic numbers. No one will ever be able to figure out why the array containing the task structures is sized by the constant NAME_LEN.

I might suggest two practices that could solve many of the Magic Constant mistakes I've seen.

1. Constants should have only one purpose.
2. A range of constants should have extra other constants to declare the range.

In the first case, we always give each new use of a number it's own constant. In the case above, NUMBER_TASKS should be separate from NAME_LEN. If there is a reason why their sizes are actually related, define one in terms of the other. The hard part is recognizing when the numbers are really distinct and when they are the same.

The second idea is a variant on the first. The constants in the range should not be used for the first and last items in the range. This is an idea that has only completely gelled just now. I've done part of this inconsistently for years, but I think I need to be more consistent. I've often defined a constant for the number of items in a range. For example, if we have a set of constants for column numbers, I might code them using C++ enums like this:


enum eColumns
{
colID, colName, colAddress, colEmail,
NUM_COLUMNS
};


By adding the final constant, I always have a simple way to iterate over the columns. However, this approach doesn't work so well if the first value isn't 0. Now I think a better approach would define a MIN_COLUMN and a MAX_COLUMN. This would allow me to loop from min to max. I could also define the number of items based on these two constants.

This would have been especially useful in the original problem. Let's assume I had two more constants:


public final int static HTTP_MIN_SUCCESS = 200;
public final int static HTTP_MAX_SUCCESS = 299;


This allows us to recode the test as


if (status >= HttpURLConnection.HTTP_MIN_SUCCESS &&
status <= HttpURLConnection.HTTP_MAX_SUCCESS) {
...


The original code in the book was repeated several times for different examples. A much better solution would be to define a new method, isSuccess() which performs this test and is coded in one place. The usage of the code would then have been


if (isSuccess( status )) {
...


which is much more readable and maintainable.

Now obviously the function which hides the implementation details of the success test is a better idea and should be available along with the constants. The extra constants are still a good idea though. At some point, a programmer may need to use this range in a way that the original programmers didn't anticipate.