Magic number constant name convention for tests ONE, TWO,.. ONE_HUNDRED vs _1 , _2, _100

Question

so I'm doing some code refactoring/sonar fixes, and there are some tests that contain magic numbers, 5, 123, 567 or whatever, i wanted to create a NumberConstant class where we save numbers used in tests, everything is good we have something like this

public static final int ZERO = 0;
public static final int ONE = 1;
public static final int TWO = 2;
public static final int THREE = 3;
public static final int FOUR = 4;
public static final int FIVE = 5;

the problem is when doing the refactoring, the code is "ok" for SonarQube, but something seems off, the code somehow becomes "cluttered",

i mean just compare these two lines

before

private LocalDateTime endtDateOfFiscalYear2018 = LocalDate.of(2018, Month.DECEMBER, 31).atTime(LocalTime.MAX);

after

private LocalDateTime endtDateOfFiscalYear2018 = LocalDate.of(TWO_THOUSAND_EIGHTEEN, Month.DECEMBER, THIRTY_ONE).atTime(LocalTime.MAX);

I thought a good compromise would be :

private LocalDateTime endtDateOfFiscalYear2018 = LocalDate.of(_2018, Month.DECEMBER, _31).atTime(LocalTime.MAX);

and having my NumberConstant class like this

 public static final int _0 = 0;
 public static final int _1 = 1;
 public static final int _2 = 2;
 public static final int _3 = 3;
 public static final int _4 = 4;
 public static final int _5 = 5;

is this a good compromise or is the whole approach incorrect? what is your approach to keeping your test clean and understandable?

You've made the mistake of thinking SonarQube was right. There was nothing wrong with the original code. `2018` is not a magic number. It's obviously a year, but it's not even relevant because you're not using it for anything except constructing the actual object you're interested in, i.e. `endDateOfFiscalYear2018`. That's a *false positive* from sonar, not an actual magic number. — Kayaman, May 19 '20 at 10:26
As @Kayaman said, your code is not wrong. If you want to get rid of the "magic number" just extract it in places where they are used to local constants with proper naming like "expectedYear" or something similar — Nadir, May 19 '20 at 10:40

Ralf Kleberhoff · Accepted Answer · 2020-05-19T10:46:19.303

I think the whole approach is incorrect.

What do you gain from introducing a named constant where the name just rephrases the value?

Introducing a constant from a literal value generally has some benefits:

Instead of some magic number you see a meaningful name.
It groups together the situations where that very same value has the same meaning. E.g. you might encounter lots of places using the number 10, some as a digit representation radix, some as the base of a logarithm value, some representing the month of October, and so on.
If you find out later that a different value better represents the intended meaning, you can change that at one central place.

All this doesn't work if you replace all occurrences of a literal 10 with a constant named TEN or _10. E.g. you'll hopefully never change TEN to have a value of 20.

So, what's needed is more than just some automated, blind replacement of values. You need to understand what that specific occurrence of the literal means, and then introduce an appropriate name for this concept, replacing all occurences of the same concept with a common name.

The original developer should have done that. If now you just blindly introduce names like TEN, you just hide the deficiency, and it'll never be improved. So I'd rather have Sonar permanently remind me of the problem than to hide it forever.

And sometimes, especially in test cases, I'd even prefer to see a literal 4711 than a constant named SOME_RANDOM_FOUR_DIGIT_NUMBER.

Magic number constant name convention for tests ONE, TWO,.. ONE_HUNDRED vs _1 , _2, _100

1 Answers1