When Not to Replace a Magic Number

Here is a peculiar construct which is idiomatic in one of the code bases I maintain:

string AVSValue = packet.substr(IDX_AVSVALUE, LEN_AVSVALUE);

Much earlier in the code:

#define IDX_AVSVALUE 32
#define LEN_AVSVALUE 1

This attempt at Fowler’s Replace Nagic Number with Symbolic Constant fails to pay attention to the motivating factor behind this refactoring: improving the readability of the code.

Assuming the next person reading this code knows the basics of C++, these magic constants do not provide any new information. That’s reason enough not to introduce them, but these particular constants require the reader to flip between two distant sections of code to understand what is happening.

The other valid reason to introduce a constant is to avoid the DRY violation which would occur if the magic number is used more than once. This is not the case with the code in question. And if it were, the constants could be placed much closer to their use. This would be preferred:

    string AVSValue;
    const int IDX_AVSVALUE = 32;
    const int LEN_AVSVALUE = 1;
    if (packet.length() >= IDX_AVSVALUE+LEN_AVSVALUE) {
        AVSValue = packet.substr(IDX_AVSVALUE, LEN_AVSVALUE);
    }