Magic numbers, locality and readability

Question

Lately I've found myself inserting magic numbers into code to make it more readable. I've done this in situations where the magic number is only used once and its purpose is obvious from the context. An example from a recent project:

/* Extract id from "/toClient/chat/id". */
String channelId = channelIdWithPath.split("/")[3];

Apparently, the "best practice" is to declare the magic number as a constant near the beginning of the class, like this:

private final int NUMBER_OF_DELIMITERS_BEFORE_ID_IN_CHAT_CHANNEL_PATH = 3;

... 100 lines of unrelated code ...

/* Extract id from "/toClient/chat/id". */
String channelId = channelIdWithPath.split("/")[NUMBER_OF_DELIMITERS_BEFORE_ID_IN_CHAT_CHANNEL_PATH];

I can't see any advantage in separating the variable declaration from the location where it's used. We've practically doubled the time it takes to understand this code and anyone reading it has to jump over 100 lines to confirm the variable has the correct value.

We could declare it as a local variable right before it's needed, but that's still a "magic number" at least according to CheckStyle. This is something I do often when I feel like the number needs a description (in this case a description like "number of delimiters..." is harder to understand than just the raw number at use).

Another option is to separate manipulation logic into a function:

String channelId = getIdFrom(channelIdWithPath);

This abstraction hides the details of the String manipulation and removes the need for a comment. Unfortunately we still have to write the function somewhere and the original problems are replicated there (including the need to document an example path with a comment).

If we want to declare the variable as a constant AND keep it close to the function, we need to create some kind of StringManipulator class for it. Now we've added a class to do something that takes 1 line of code. I feel like this type of approach leads to sprawling programs where

• the individual components of a program become easier to understand
• the structural complexity and execution flow become harder to understand.

For example, if I wanted to read how channelId is retrieved, I would first have to jump into another method in another class, and then I would have to jump to where the variable is declared. All of this could be on a single line of code.

Edit: this answer on a similar question applies pretty well for my particular question.

Answer 1

In layman's terms:

• If you will be extracting the channelID in several places, then you should create a function.
• Such a function, being cohesive, should read from the state.
• Part of that state would be the constant with a name like DELIMITERS_BEFORE_ID.

I find that this:

String channelId = channelIDdWithPath.split("/")[DELIMS_BEFORE_ID];

...is more readeble that this

String channelId = channelIDWithPath.split("/")[3];

..this is even better:

String channelId = channelIDWithPath.split(PATH_DELIM)[DELIMS_BEFORE_ID];

..but this is best:

String channelId = extractChannelID(channelIDWithPath);

• You don't have to verify if the value asigned to the constants or class variables are correct everytime you read the code. The idea is that if things pass the test, you can "abstract yourself" from the inner workings of them. Your brain can only handle so much complexity, so you will, in time, have to stop cheking if the value of DELIMS_BEFORE_ID is OK and begin to use the lego pieces without cheking whether or not the lego blocks contain the proper amount of acrylonitrile butadiene styrene.

Answer 2

Your should focus on what makes your code readable and the spirit of the guidelines rather than try to prove you can write bad code even when following the guidelines.

A constant named NUMBER_OF_DELIMITERS_BEFORE_ID_IN_CHAT_CHANNEL_PATH is not very clear or understandable. So yes a magic number is probably not any worse than using such a badly named constant. But this is not an argument for using magic numbers, this is an argument for using better names for your constants, or rewrite the code to use a different approach.

Your first code example shows a simple way to get the third segment. But presumably you also need the other URL segments somewhere else in the app, so you need to repeat .split("/")[index] and the accompanying comment every time you need a segment. This break the "dont repeat yourself" principle, and suddenly your URL parsing logic is spread all over the application. If the URL scheme changes (e.g. say a new segment is introduced between the second and third) you have to update a lot of numbers and comments, which is error prone.

By the way, you shouldn't have 100 lines of unrelated code in a method or class. As per the single responsibility principle, everything in a class should be related, otherwise it should be broken up. A variable or constant should always be declared as close to its use as possible and in as small a scope as possible.

If you have the constant at the class level, it suggest you use it in multiple places to extract segments from the path. The solution here is to encapsulate the URL parsing in a single place, so you only use the indexes there, and don't expose them to the program at large. Again the issue is not constants versus literals, but encapsulation.

Answer 3

See Luc's valid comment on your question re. URL templates. I would add... anytime you want a magic number that is neither 0 or 1 and is also not referring to a specific independent feature of the business domain, that is a code smell.

Your issue here isn't that you're hardcoding a magic number so much as you're hardcoding something which is replicating information that already implicitly exists. If your URL templates change, your magic number now needs to be brought in sync. That's a (minor) maintenance issue, and those add up, and make bugs more likely.

Answer 4

OK there are two competing questions here. One is what is the 'official' 'best' way to program this and Two is that way actually more readable

so here is my 'official' 'best' way

public class ChannelInfo
{
    public String ChannelId { get; set; }
}

public class PathExtractor : IPathExtractor
{
    private const int NUMBER_OF_DELIMITERS_BEFORE_ID_IN_CHAT_CHANNEL_PATH = 3;
    private const char SPLITCHAR = '/';
    public ChannelInfo GetChannelInfo(string path)
    {
        var info = new ChannelInfo();
        info.ChannelId = path.Split(new char[] { SPLITCHAR })[NUMBER_OF_DELIMITERS_BEFORE_ID_IN_CHAT_CHANNEL_PATH];
        return info;
    }
}

Now, is it 'better' and 'more readable(tm)' than

/* Extract id from "/toClient/chat/id". */
String channelId = channelIdWithPath.split("/")[3];

I say NO!! please! control your shock and rage dear reader, while I add some provisos....

• If you re use the code in many places the first method may save you some characters

• If you have multiple implementations of IPathExtractor the first method is more flexible

• If you have more properties you can add to ChannelInfo the first method becomes easier to use

So in general, yes its probably a good idea to split logic out into classes etc

BUT

• If you only have a single point where you extract a single value as part of some larger function and it is commented

Then no, its not particularly more readable or easier if you split it out into an interface plus an implementation plus a return object, plus all the properties and methods etc.

You will have added a tonne more code just so you can say you are 'clean coding' and ticking style check boxes