5

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.

  • 4
    So what is the question? – JacquesB Jul 10 '16 at 19:07
  • 1
  • I assume most people here disagree with me. I would like to hear why magic numbers are bad, specifically in context like this. – Atte Juvonen Jul 10 '16 at 19:09
  • 3
    If you need to do this kind of url hacking you might be mixing responsibilities and that might be the reason for your issue. If you have a place where you parse the url centrally it may remove this issue in total. Normally urls are parsed as pieces on the basis of a template. Template might be: "/*controller/:action/:id" and it might call your controller class, action method and the first param would be the id. Only magic string then is the slash (URL_SEPARATOR for example). – Luc Franken Jul 11 '16 at 8:32
  • anyone remember comments? eg var code = parts[3]; //hotel code is in col 3 – Ewan Jul 11 '16 at 12:42
12

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.
  • Great point in regards to trusting abstractions. Let's say I encapsulate all URL manipulation in the project to a single class so I can call methods like extractChannelID() whenever I need something. The name "DELIMS_BEFORE_ID" becomes ambiguous as we may have different kinds of URLs and different kinds of IDs. Either we use magic numbers in that class or we have monstrosity names like "DELIMS_BEFORE_CHANNEL_ID_IN_CHAT_CHANNEL_PATH". So part of the original problem is just replicated elsewhere. – Atte Juvonen Jul 11 '16 at 16:22
  • 1
    @AtteJuvonen In that case I see no monstruosity in DELIMS_BEFORE_CHANNEL_ID_IN_CHAT_CHANNEL_PATH. It's a perfectly descriptive name. – Tulains Córdova Jul 11 '16 at 16:37
  • 2
    @AtteJuvonen: The name should only be scoped to the method or class which parses this particular url-schema. If you have multiple independent url-schemas, they should also have independent parsing. – JacquesB Jul 11 '16 at 16:42
  • 1
    @AtteJuvonen: So if you have multiple URL you would have say ChatUrl.GetChannelID(), RecipyUrl.GetDishName() etc. – JacquesB Jul 11 '16 at 16:44
8

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.

  • I did focus on making the code readable, that's why I refactored the magic numbers into the code. The actual constant name in our project was "POSITION_OF_CHANNEL_ID_IN_CHANNEL_PATH". I spent several minutes trying to think of a better name just to provide a fair comparison for this discussion. If you have a way to make this 1 line of code more readable, feel free to show it. Btw. we had over 100 lines of code in the class, not in the method. – Atte Juvonen Jul 10 '16 at 19:29
  • 3
    @AtteJuvonen: You could try asking in Code Review codereview.stackexchange.com if you need help making code more readable in general. This site is more for QA-type questions. – JacquesB Jul 10 '16 at 20:03
  • 2
    @AtteJuvonen: But I would probably have local constants corresponding to the indices of each URL segment. This makes it easier to mange if the path schema changes, e.g a new segment is introduced. If you use numeric literals for the segments, it will be a mess if the scheme changes, e.g. you have to increment all numbers for segments higher than n, if a new segment is introduced at n. This is quite error prone. – JacquesB Jul 10 '16 at 20:09
  • 2
    After reading your & other peoples' posts here I now agree that encapsulating all the url parsing to one place would be best. (Even though the id extraction in particular is only used once, the code has other URL manipulation as well and those should all be together rather than dispersed over different classes.) – Atte Juvonen Jul 11 '16 at 16:11
1

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.

-1

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

  • You are using a named constant rather than a literal 3 in both examples though, but the question is about whether using a constant at all. – JacquesB Jul 11 '16 at 15:29
  • oops fixed see edit – Ewan Jul 11 '16 at 15:34
  • Does your second example assume the fist two segments are not used anywhere in the application at all? Why would they be there in the first place then? – JacquesB Jul 11 '16 at 15:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.