Clean Code - Should I change the literal 1 to a constant?

Question

To avoid magic numbers, we often hear that we should give a literal a meaningful name. Such as:

//THIS CODE COMES FROM THE CLEAN CODE BOOK
for (int j = 0; j < 34; j++) {
    s += (t[j] * 4) / 5;
}

-------------------- Change to --------------------

int realDaysPerIdealDay = 4;
const int WORK_DAYS_PER_WEEK = 5;
int sum = 0;
for (int j = 0; j < NUMBER_OF_TASKS; j++) {
    int realTaskDays = taskEstimate[j] * realDaysPerIdealDay;
    int realTaskWeeks = (realdays / WORK_DAYS_PER_WEEK);
    sum += realTaskWeeks;
}

I have a dummy method like this:

Explain: I suppose that I have a list people to serve and by default, we spend $5 to buy food only, but when we have more than one person, we need to buy water and food, we must spend more money, maybe $6. I'll change my code, please focus on the literal 1, my question about it.

public int getMoneyByPersons(){
    if(persons.size() == 1){ 
        // TODO - return money for one person
    } else {
        // TODO - calculate and return money for people.
    }

}

When I asked my friends to review my code, one said giving a name for the value 1 would yield cleaner code, and the other said we don't need a constant name here because the value is meaningful by itself.

So, my question is Should I give a name for the literal value 1? When is a value a magic number and when is it not? How can I distinguish context to choose the best solution?

Answer 1

No. In that example, 1 is perfectly meaningful.

However, what if persons.size() is zero? Seems strange that persons.getMoney() works for 0 and 2 but not for 1.

Answer 2

Why does a piece of code contain that particular literal value?

• Does this value have a special meaning in the problem domain?
• Or is this value just an implementation detail, where that value is a direct consequence of the surrounding code?

If the literal value has a meaning that is not clear from context, then yes, giving that value a name through a constant or variable is helpful. Later, when the original context is forgotten, code with meaningful variable names will be more maintainable. Remember, the audience for your code is not primarily the compiler (the compiler will happily work with horrible code), but future maintainers of that code – who will appreciate if the code is somewhat self-explaining.

• In your first example, the meaning of literals such as 34, 4, 5 is not apparent from the context. Instead, some of these values have a special meaning in your problem domain. It was therefore good to give them names.

• In your second example, the meaning of the literal 1 is very clear from the context. Introducing a name is not helpful.

In fact, introducing names for obvious values can also be bad as it hides the actual value.

• This can obscure bugs if the named value is changed or was incorrect, especially if the same variable is reused in unrelated pieces of code.

• A piece of code might also work fine for a specific value, but might be incorrect in the general case. By introducing unnecessary abstraction, the code is no longer obviously correct.

There is no size limit on “obvious” literals because this depends totally on context. E.g. the literal 1024 might be totally obvious in the context of file size calculation, or the literal 31 in the context of a hash function, or the literal padding: 0.5em in the context of a CSS stylesheet.

Answer 3

There are several issues with this piece of code, which, by the way, can be shortened like this:

public List<Money> getMoneyByPersons() {
    return persons.size() == 1 ?
        moneyService.getMoneyIfHasOnePerson() :
        moneyService.getMoney(); 
}

1. It is unclear why one person is a special case. I suppose that there is a specific business rule which tells that getting money from one person is radically different from getting money from several persons. However, I have to go and look inside both getMoneyIfHasOnePerson and getMoney, hoping to understand why are there distinct cases.

2. The name getMoneyIfHasOnePerson doesn't look right. From the name, I would expect the method to check if there is a single person and, if this is the case, get money from him; otherwise, do nothing. From your code, this is not what is happening (or you're doing the condition twice).

3. Is there any reason to return a List<Money> rather than a collection?

Back to your question, because it is unclear why there is a special treatment for one person, the digit one should be replaced by a constant, unless there is another way to make the rules explicit. Here, one is not very different from any other magic number. You could have business rules telling that the special treatment applies to one, two or three persons, or only to more than twelve persons.

How can I distinguish context to choose the best solution?

You do whatever makes your code more explicit.

Example 1

Imagine the following piece of code:

if (sequence.size() == 0) {
    return null;
}

return this.processSequence(sequence);

Is zero here a magical value? The code is rather clear: if there are no elements in the sequence, let's not process it and return a special value. But this code can also be rewritten like this:

if (sequence.isEmpty()) {
    return null;
}

return this.processSequence(sequence);

Here, no more constant, and the code is even clearer.

Example 2

Take another piece of code:

const result = Math.round(input * 1000) / 1000;

It doesn't take too much time to understand what it does in languages such as JavaScript which don't have round(value, precision) overload.

Now, if you want to introduce a constant, how would it be called? The closest term you can get is Precision. So:

const precision = 1000;
const result = Math.round(input * precision) / precision;

Does it improve readability? It might be. Here, the value of a constant is rather limited, and you may ask yourself if you really need to perform the refactoring. The nice thing here is that now, the precision is declared only once, so if it changes, you don't risk making a mistake such as:

const result = Math.round(input * 100) / 1000;

changing the value in one location, and forgetting to do it in the other one.

Example 3

From those examples, you may have an impression that numbers should be replaced by constants in every case. This is not true. In some situations, having a constant doesn't lead to code improvement.

Take the following piece of code:

class Point
{
    ...
    public void Reset()
    {
        x, y = (0, 0);
    }
}

If you try to replace zeroes by a variable, the difficulty would be to find a meaningful name. How would you name it? ZeroPosition? Base? Default? Introducing a constant here would not enhance the code in any way. It would make it slightly longer, and just that.

Such cases are however rare. So any time you find a number in code, make the effort trying to find how the code can be refactored. Ask yourself if there is a business meaning to the number. If yes, a constant is mandatory. If no, how would you name the number? If you find a meaningful name, that's great. If not, chances are you found a case where the constant is unnecessary.

Answer 4

You could make a function that takes a single parameter and returns times four divided by five that offers a clean alias into what it does while still using the first example.

I'm just offering my own usual strategy but maybe I'll learn something too.

What I am thinking is.

// Just an example name  
function normalize_task_value(task) {  
    return (task * 4) / 5;  // or to `return (task * 4) / NUMBER_OF_TASKS` but it really matters what logic you want to convey and there are reasons to many ways to make this logical. A comment would be the greatest improvement

}  

// Is it possible to just use tasks.length or something like that?  
// this NUMBER_OF_TASKS is the only one thats actually tricky to   
// understand right now how it plays its role.  

function normalize_all_task_values(tasks, accumulator) {  
    for (int i = 0; i < NUMBER_OF_TASKS; i++) {  
        accumulator += normalize_task_value(tasks[i]);
    }
}  

Sorry if I'm off base but I'm just a javascript developer. I'm not sure what composition I justified this, I guess not everything has to be in an array or list but .length would make alot of sense. And the * 4 would make the good constant since its origin is nebulous.

Answer 5

That number 1, could it be a different number? Could it be 2, or 3, or are there logical reasons why it must be 1? If it must be 1, then using 1 is fine. Otherwise, you can define a constant. Don't call that constant ONE. (I have seen that done).

60 seconds in a minute - do you need a constant? Well, it is 60 seconds, not 50 or 70. And everyone knows it. So that can stay a number.

60 items printed per page - that number could have easily been 59 or 55 or 70. Actually, if you change the font size, it might become 55 or 70. So here a meaningful constant is more asked for.

It's also a matter how clear the meaning is. If you write "minutes = seconds / 60", that's clear. If you write "x = y / 60", that's not clear. There must be some meaningful names somewhere.

There is one absolute rule: There are no absolute rules. With practice you will figure out when to use numbers, and when to used named constants. Don't do it because a book says so - until you understand why it says that.

Answer 6

I have seen a fair amount of code like in the (modified) OP in DB retrievals. The query returns a list, but the business rules say there can be only one element. And then, of course, something changed for 'just this one case' to a list with more than one item. (Yeah, I said a fair amount of times. It's almost like they ... nm)

So rather than then create a constant, I would (in clean code method) create a method to give a name or make clear what the conditional is intended to detect (and encapsulate how it detects it):

public int getMoneyByPersons(){
  if(isSingleDepartmentHead()){ 
    // TODO - return money for one person
  } else {
    // TODO - calculate and return money for people.
  }
}

public boolean isSingleDepartmentHead() {
   return persons.size() == 1;
}