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