Clean Code – Should Literal 1 Be Changed to a Constant?

clean codecode-qualitycode-reviewscoding-style

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?

Best Answer

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.

Related Solutions

Clean Code – Why Avoid Protected Variables

Protected variables should be avoided because:

They tend to lead to YAGNI issues. Unless you have a descendant class that actually does stuff with the protected member, make it private.
They tend to lead to LSP issues. Protected variables generally have some intrinsic invariance associated with them (or else they'd be public). Inheritors then need to maintain those properties, which people can screw up or willfully violate.
They tend to violate OCP. If the base class makes too many assumptions about the protected member, or the inheritor is too flexible with the behavior of the class, it can lead to the base class' behavior being modified by that extension.
They tend to lead to inheritance for extension rather than composition. This tends to lead to tighter coupling, more violations of SRP, more difficult testing, and a slew of other things that fall within the 'favor composition over inheritance' discussion.

But as you see, all of these are 'tend to'. Sometimes a protected member is the most elegant solution. And protected functions tend to have fewer of these issues. But there are a number of things that cause them to be treated with care. With anything that requires that sort of care, people will make mistakes and in the programming world that means bugs and design problems.

Php – Reusable VS clean code – where’s the balance

The trick is that you do not have to choose. One nice thing that many ORMs provide (for instance that of Django, Squeryl, SQLAlchemy and more) is composable queries. That is, you can not only query a table but query the results of another query. This is not as costly as it looks, as either the ORM or the DB query optimizer can reduce it to a combined SELECT under the hood.

So you would end up with something like (Django example)

class Article:
  @staticmethod
  def posts():
    return Article.objects.all()

  @staticmethod
  def author_posts(author):
    return Article.posts().filter(author=author)

There you have - two separate methods and no repetition. :-)

When you have to choose between clean and reusable, there is often a third option: a little more sophisticated but both clean and reusable.

(By the way, the static methods are there just to mimic your structure. Actually one would have instance methods on the Article manager class. I should also add that this is a trivial example, as the default manager - named Article.objects - already provides the all method to retrieve all results. But this is not the point - the point is that you have obtained a more specific result set by querying a larger result set.)

Related Topic