Magic numbers, locality and readability

coding-styleprogramming practices

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.

Best Answer

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.

Related Solutions

Should a String Constant Be Defined if Used Only Once?

Try this. The initial reflection is certainly expensive, but if you're going to use it many many times, which I think you will, this is most certainly a better solution what what you're proposing. I don't like using reflection, but I find myself using it when I don't like the alternative to reflection. I do think that this will save your team a lot of headache, but you must pass the name of the method (in lowercase).

In other words, rather than pass "name", you would pass "fullname" because the name of the get method is "getFullName()".

Map<String, Method> methodMapping = null;

public Object getNode(String name) {
    Map<String, Method> methods = getMethodMapping(contact.getClass());
    return methods.get(name).invoke(contact);
}

public Map<String, Method> getMethodMapping(Class<?> contact) {
    if(methodMapping == null) {
        Map<String, Method> mapping = new HashMap<String, Method>();
        Method[] methods = contact.getDeclaredMethods();
        for(Method method : methods) {
            if(method.getParameterTypes().length() == 0) {
                if(method.getName().startsWith("get")) {
                    mapping.put(method.getName().substring(3).toLower(), method);
                } else if (method.getName().startsWith("is"))) {
                    mapping.put(method.getName().substring(2).toLower(), method);
                }
            }
        }
        methodMapping = mapping;
    }
    return methodMapping;
}

If you need to access data contained within members of contact, you might consider building a wrapper class for contact which has all methods to access any information required. This would also be useful for guaranteeing that the names of the access fields will always remain the same (I.e. if wrapper class has getFullName() and you call with fullname, it will always work even if contact's getFullName() has been renamed -- it would cause compilation error before it would let you do that).

public class ContactWrapper {
    private Contact contact;

    public ContactWrapper(Contact contact) {
        this.contact = contact;
    }

    public String getFullName() {
        return contact.getFullName();
    }
    ...
}

This solution has saved me several times, namely when I wanted to have a single data representation to use in jsf datatables and when that data needed to be exported into a report using jasper (which doesn't handle complicated object accessors well in my experience).

Programming Practices – Is It Good Practice to Declare a Function Inline in C#?

As far as I know, declaring a helper method as a lambda like this is not commonly done in C#, so I would advise against doing this unless you have a good reason.

Good reasons include:

The lambda could do something a separate method can't, like closing over a local variable or using anonymous type.
Others in your team agree with you that this is a good practice.

That being said, starting with C# 7.0, C# supports local functions, which are similar to such helper lambdas, but generally better: they have more succinct syntax and are more efficient. So if you used a local function for this purpose, I think that would be perfectly fine.

Best Answer

Related Solutions

Should a String Constant Be Defined if Used Only Once?

Programming Practices – Is It Good Practice to Declare a Function Inline in C#?

Related Topic