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

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 realTaskWeeks = (realdays / WORK_DAYS_PER_WEEK);
}
``````

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?

• Where does `persons` come from and what does it describe? Your code has no comments whatsoever so its hard to guess what it's doing. – Hubert Grzeskowiak Sep 11 '18 at 3:21
• It does not get any clearer than 1. I would change "size" to "count" though. And moneyService is stupid as it is, it should be able to decide how much money to return depending on the persons collection. So I would pass persons to getMoney and let it sort out any exceptional cases. – Martin Maat Sep 11 '18 at 5:26
• You could also extract the logical expression from your if clause to a new method. Maybe call it IsSinglePerson(). That way you extract a little more than just that one variable but also make the if clause a little more readable... – selmaohneh Sep 11 '18 at 5:41
• Possible duplicate of Is every number in the code considered a "magic number"? – gnat Sep 11 '18 at 6:28
• Perhaps this logic would be better suited in moneyService.getMoney()? Would there ever be a moment where you'd need to call getMoney in the case of 1 person? But I would agree with the general sentiment that 1 is clear. Magic numbers are numbers where you'd have to scratch your head asking how the programmer arrived at that number.. i.e. `if(getErrorCode().equals(4095)) ...` – Neil Sep 11 '18 at 6:38

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.

• I agree that 1 is meaningful. Thanks, by the way, I updated my question. You can see it again to get my idea. – Jack Sep 12 '18 at 7:01
• A phrase I've heard multiple times over the years is that magic numbers are any unnamed constants other than 0 and 1. While I can potentially think of examples where these numbers should be named, it's a fairly simple rule to follow 99% of the time. – Baldrickk Nov 15 '18 at 14:42
• @Baldrickk Sometimes, other numeric literals should not be hidden behind a name either. – Deduplicator Nov 21 '18 at 11:56
• @Deduplicator any examples come to mind? – Baldrickk Nov 21 '18 at 13:35
• @Baldrickk See amon. – Deduplicator Nov 21 '18 at 15:59

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.

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.

• I would add 3a: Don't use the word money in variable names, use amount, balance or the like. A collection of money does not make sense, collections of amounts or balances does. (OK I do have a small collection of foreign money (or rather coins) but that is a different non-programming matter). – Bent Sep 11 '18 at 21:22

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
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.

for (int i = 0; i < NUMBER_OF_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.

• But what do those values 4 and 5 mean? If one of them ever needs to change, would you be able to find all places where the number is used in a similar context and update those locations accordingly? That is the crux in the original question. – Bart van Ingen Schenau Sep 11 '18 at 7:33
• I think the first example was pretty dang self explanatory so that might not be one I can answer. The operation should not change in the future, when you need to I'd write a new one to accomplish whats needed. Comments are what I think would most benefit this purpose. Its a one line call to reduce in my language. How important is it to make absolutely understandable to the layperson? – etisdew Sep 11 '18 at 10:14
• @BartvanIngenSchenau The whole function is improperly named. I would prefer a better function name, a comment with a spec what the function is supposed to return, and a comment why * 4 / 5 achieves that. Constant names are not really needed. – gnasher729 Sep 11 '18 at 23:21
• @gnasher729: If the constants appear exactly once in the source code of a well named, well documented function, then it might not be needed to use a named constant. As soon as a constant appear multiple times with the same meaning, giving that constant a name ensures that you don't have to figure out if all those instances of the literal 42 mean the same thing or not. – Bart van Ingen Schenau Sep 12 '18 at 7:59
• At the heart of it, if you expect you may need to change the value yes. I would change it to a const to represent the variable, however. I don't think it is the case and this should simply be an intermediate variable residing in the scope above it. I don't want to cop out and say make everything a parameter to a function but if it can change but rarely I would make it an internal parameter to that function or object/struct scope where you're currently working to make that change leaving the global scope alone. – etisdew Sep 12 '18 at 10:16

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.

• "60 seconds in a minute ... And everyone knows it. So that can stay a number." - Not a valid argument to me. What if I have 200 places in my code where "60" is used? How do I find the places where it's used as seconds to the minute vs. other possibly equally "natural" uses of it? - In practice, however, I, too, dare to use `minutes*60`, sometimes even `hours*3600` when I need it without declaring additional constants. For days I'd probably write `d*24*3600` or `d*24*60*60` because `86400` is close to the edge where someone will not recognize that magic number from a glance. – JimmyB Nov 15 '18 at 13:31
• @JimmyB If you're using "60" on 200 places in your code, it is very likely that the solution is refactoring out a function rather than introducing a constant. – klutt Nov 21 '18 at 7:18

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(){