Magic numbers are bad... I totally agree. But there's one magic number I find hard to fix:
'100' is a magic number.
Consider this code:
public double getPercent(double rate) {
return rate * 100;
}
public double getRate(double percent) {
return percent / 100;
}
SonarQube will raise 2 violations, one for each use of 100. I could replace 100 with a constant variable, but what would be a good name for it? Is that really a good idea? If somebody ever changes its value that will be a disaster. I could also just add // NOSONAR
to all the lines that use 100.
What is the best practice for dealing with the magic number 100?
UPDATE
From the answers, the most useful bit for me was this:
Practical Magic Number rule: A literal is a not a magic number if the most meaningful variable name for it is the same as the spoken name of the literal.
So by this logic, 100 is NOT a magic number.
However, to make the violation go away, I decided to replace the 100 with a constant:
public static final double HUNDRED = 100; // change it and I'll kill you
In my real project there are many lines using 100, and if I put // NOSONAR
on all those lines that might cover up other potential problems that other developers might inadvertently add later.
Not sure if there will be any real benefits using this constant. A small one may be that when I do git grep 100
I see a lot of matches from resource files in the project, while git grep HUNDRED
turns up just the the Java code that uses this.
9 Answers 9
Speaking as a human programmer (i.e. I am not Lint software), your use of "100" there looks fine to me.
Wikipedia has an article (without citations) titled Accepted limited use of magic numbers: IMO your "100" is in the same category as these other "accepted" magic numbers.
This Wiki describing magic numbers says two things.
Firstly,
Practical Magic Number rule: A literal is a not a magic number if the most meaningful variable name for it is the same as the spoken name of the literal.
That's applicable here: you're looking for a named constant like HUNDRED
or CENTUM
.
Secondly, it also suggests loading "magic" numbers (e.g. a "discount rate") from a configuration file:
static final double DISCOUNT_PERCENT = getProperty( "sales.discount_percent" );
static final double DISCOUNT_FACTOR = 1 - (DISCOUNT_PERCENT / 100);
// ...
salePrice = DISCOUNT_FACTOR * regularPrice;
Note that though this example code carefully loaded DISCOUNT_PERCENT
from a configuration, the "100" used to calculate the DISCOUNT_FACTOR
is hard-coded.
If you use "100" instead of HUNDRED
, it's easier for a programmer to understand, and to verify that it's correct.
IMO the only benefit to using HUNDRED
is to find the several methods which use the same magic number (in your example it's used by getPercent
and getRate
).
-
15\$\begingroup\$ @janos then you can do
return rate * 2 * 2 * 5 * 5;
. Much easier since it doesn't contain magic numbers. Much easier to read as well since it's been factorized /s \$\endgroup\$WernerCD– WernerCD2014年03月11日 17:31:42 +00:00Commented Mar 11, 2014 at 17:31 -
4\$\begingroup\$ I don’t even agree with the "only benefit" you mention: searching for "HUNDRED" is no easier than searching for "100". 100 is not a magic number here and there’s no reason to reduce clarity in order to kowtow to some linter. \$\endgroup\$bdesham– bdesham2014年03月11日 21:34:58 +00:00Commented Mar 11, 2014 at 21:34
-
10\$\begingroup\$ @WernerCD: Sorry, only 0,1 and 2 are not magic. ITYM
(rate << 2) * (2*2+1) * (2+2+1)
. \$\endgroup\$MSalters– MSalters2014年03月12日 08:30:24 +00:00Commented Mar 12, 2014 at 8:30 -
30\$\begingroup\$ @WernerCD Too complicated!
1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1+1
ftw. \$\endgroup\$David Richerby– David Richerby2014年03月12日 11:45:46 +00:00Commented Mar 12, 2014 at 11:45 -
7\$\begingroup\$ My first thought was that surely you shouldn't be converting between rate and percentage in code since you'd use rate for all calculations and percentage only when displaying to the user. Does Java not have any nice number formatting methods that will format a double as a percentage string? \$\endgroup\$Chris– Chris2014年03月12日 16:29:05 +00:00Commented Mar 12, 2014 at 16:29
Although 100
should be fine in source-code, I'm surprised nobody has offered the most readable alternative yet. This should be acceptable for both humans and lint-code:
Define your constant PERCENT=0.01
.
Then, when you need to do a conversion:
rate = discount*PERCENT
or
discount = rate/PERCENT
This can completely eliminate your short functions (which are, indeed, trivial). You could have additional constants PERMILLE
, PPM
, PPB
, etc., and it should be obvious for humans what is happening.
-
1\$\begingroup\$ +100 Or if you really must make it clear what the constant means, name it
PER_CENT
as in "per one hundred"--the definition of percent. \$\endgroup\$David Harkness– David Harkness2014年03月12日 18:37:29 +00:00Commented Mar 12, 2014 at 18:37 -
5\$\begingroup\$ Created an account just to vote this up. This is way better than the other answers. \$\endgroup\$bjb568– bjb5682014年03月13日 02:24:26 +00:00Commented Mar 13, 2014 at 2:24
-
8\$\begingroup\$ very good, but there are some caveats, I believe, with the internal representation... you may need to have one per types of data (float, double float, etc)... 100 (or a constant valued at 100) is safer to use.See: Java Double value = 0.01 changes to 0.009999999999999787 or why-not-use-double-or-float-to-represent-currency or rounding-errors \$\endgroup\$Olivier Dulac– Olivier Dulac2014年04月03日 16:12:54 +00:00Commented Apr 3, 2014 at 16:12
-
3\$\begingroup\$ I can't really call this more readable. In particular,
discount = rate/PERCENT
feels rather contorted. Without very clear names, and perhaps even with, it'd be way too easy to forget whetherPERCENT
is 0.01 or 100. Perhaps if it were calledONE_PERCENT
or something, i dunno...but as is, it's more magical than if it were left a "magic number". \$\endgroup\$cHao– cHao2014年06月26日 01:33:46 +00:00Commented Jun 26, 2014 at 1:33 -
14\$\begingroup\$ Sigh.
discount*0.01
andrate/0.01
is not the same asdiscount/100
andrate*100
. While100
is exact,0.01
is not. \$\endgroup\$David Ongaro– David Ongaro2014年10月01日 19:49:02 +00:00Commented Oct 1, 2014 at 19:49
Some numbers are called 'magic' because it is unclear where they come from. I think in this particular case, it is clear that 100 originates from the definition of percent. However, if you wish you can define a constant PERCENTS_IN_UNIT_RATE=100
instead of using it directly.
Violations reported by code analysis tools are really only suggestions and it is okay to disagree with them. If in doubt ask other programmers who work on the same project, or toss a coin and move to the next task! :-)
-
6\$\begingroup\$ I don't think
HUNDRED
is any more readable than 100. Yes, it will satisfy the immediate tool complaints, but you probably use the tool to raise code quality, and as I mentioned it does not do so in my opinion.MAX_PERCENT
is better but still may raise questions.PERCENT_MAX
sounds similar toINT_MAX
and may be slightly better yet.PERCENTS_IN_UNIT_RATE
is imho most understandable but is also too long. Hence, I'd vote for keeping literal 100. \$\endgroup\$mkalkov– mkalkov2014年03月11日 10:39:04 +00:00Commented Mar 11, 2014 at 10:39 -
1\$\begingroup\$
MAX_PERCENT
seems misleading in that it implies that the 100 is just an arbitrary constant that could be changed any time. It would seem to me that outside of dodgy accounting practices, 100 is pretty immutable :-) \$\endgroup\$microtherion– microtherion2014年03月11日 10:51:43 +00:00Commented Mar 11, 2014 at 10:51 -
1\$\begingroup\$
A100PERCENTS
orONE100PERCENTS
orPERCENTS_100
orX100PERCENTS
would all work IMO. I like these because they include numeric 100, and also include word percent, making both numeric value and context apparent. \$\endgroup\$hyde– hyde2014年03月11日 11:13:54 +00:00Commented Mar 11, 2014 at 11:13 -
1\$\begingroup\$ If I buy a goat for 1ドル and sell it for 8,ドル then I've sold it for 800% of its original price.
MAX_PERCENT
should probably be a constant equal toDBL_MAX
. Just use 100 and avoid confusion. \$\endgroup\$Brendan– Brendan2014年03月11日 17:39:10 +00:00Commented Mar 11, 2014 at 17:39 -
\$\begingroup\$ I have no idea where
RATE
inPERCENTS_PER_UNIT_RATE
came from. Because you're working on an interest rate? But that has absolutely no effect on conversion between percentage and scale. I'd suggestPERCENT_PER_UNITY
or something like that. \$\endgroup\$Ben Voigt– Ben Voigt2014年03月11日 22:12:44 +00:00Commented Mar 11, 2014 at 22:12
I am on the fence about the 100 being a named constant or not.
I habitually make magic numbers named constants, but not all the time.... it is not unusual for me to have constants called HUNDRED
or MILLION
. I also regularly have trivial numbers as magic-number constants ....
But, in this case, and what I particularly want to draw attention to, is that your value should be a floating-point, not an integral value.
When you have constants they should be in the most convenient form for users to understand. For example, your value 100
should really be 100.0
which makes the fact that it is a double value obvious.
-
7\$\begingroup\$ When you use a constant called
HUNDRED
, is it100
or100.0
? \$\endgroup\$Joonas Pulakka– Joonas Pulakka2014年03月11日 19:09:34 +00:00Commented Mar 11, 2014 at 19:09 -
3\$\begingroup\$ Ok, but that's just one possibility.
HUNDRED
could as well be aBigInteger
, orBigDecimal
, orbyte
, just to mention a few. With number literals, the type is immediately obvious. \$\endgroup\$Joonas Pulakka– Joonas Pulakka2014年03月12日 07:36:39 +00:00Commented Mar 12, 2014 at 7:36 -
7\$\begingroup\$ If you end up dealing with large numbers a lot, i think things like
MILLION
could come in handy just because it could be very easy to mistake 1000000 for 10000000 etc \$\endgroup\$DLeh– DLeh2014年03月12日 20:09:14 +00:00Commented Mar 12, 2014 at 20:09 -
1\$\begingroup\$ If you use
HUNDRED
in lots of places, then, when you need to change it in a subset of these places and try to change the constant... you're messed up. This would be no better than doing something likesed 's/100/101/g'
for all of your project codebase. \$\endgroup\$Ruslan– Ruslan2014年03月14日 06:27:49 +00:00Commented Mar 14, 2014 at 6:27 -
2\$\begingroup\$ @DLeh
1_000_000
\$\endgroup\$Justin– Justin2016年03月14日 18:43:40 +00:00Commented Mar 14, 2016 at 18:43
Others have already stated why they believe that 100 is an acceptable constant here. This answer shows how you can get Sonar to accept that.
Under your-sonar-domain/profiles/
, you can edit quality profiles, and the rules in them. The "Magic Number" rule is customisable, you can specify certain numbers for the rule to ignore. Simply specify "100" as a number to ignore.
Change ignoreNumbers to include "100!
It technically is since both those 100's have the same context. Similar to me using 32 as a width in several different places. It depends how nitpicky you want to be. In this case I might use something like MAX_PERCENT
. It does help with readability as well since when I see that I know the calculations have to do with percentages.
-
2\$\begingroup\$ I would be rather amused if I saw code that used
MAX_PERCENT
\$\endgroup\$Navin– Navin2014年03月12日 04:38:43 +00:00Commented Mar 12, 2014 at 4:38 -
\$\begingroup\$ Poor attempt at brevity I guess. Though being amused by something helps remember it. \$\endgroup\$0xFADE– 0xFADE2014年03月12日 18:12:01 +00:00Commented Mar 12, 2014 at 18:12
If forced to use a symbol, I might call it N100. That way you can tell what its value is. A long time ago I saw this done for common constants. And negative numbers were prefixed with "M". Why did they do this? Because the computer instruction set couldn't load numbers in line ("immediate"); they had to be fetched from a memory location. And the assembler used symbols to refer to those locations.
I think 100 IS a magic number here.
Percentage is vastly used, but also per mille (1/1000) is often used, and basis point (1/10.000) and percent mille (1/100.000) are used enough to have their own name.
I would change your function to getPerPart(value, part)
and getFromPart(value, part)
, maybe with a default getPerPart(value)
and getFromPart(value)
, which use DEFAULT_PART_VALUE = 100
.
Also, you may still want to write a getPercent
, in which case the use of hardcoded 100 is fine.
-
\$\begingroup\$ You are really going to make functions for multiplication and division? Why not allow your formatter to display numbers how the user wants to see them? You're not going to use these functions in calculations. \$\endgroup\$Navin– Navin2014年03月15日 05:55:50 +00:00Commented Mar 15, 2014 at 5:55
Any number that remains unnamed remains a magic number. It's not related common usage as common usage is relative. The problem with it is "when" or "where" to draw a line... when does a number changes its state to non-magic...
2,71828... may not be magic for mathematicians (Euler)
1760 may not be a magic number for anglo saxons (Yards per mile)
10000000 may not be magic for south inhabitant of south asia (crore)
1 may not be magic in the context of multiplication (neutral element)
The relative approach will lead to a discussion that will evolve a local aggreement. And the result to consider a number to be non-magic, will depend on when and where you start the discussion. Unfortunally this discussion has to be lead for each number separately.
The absolute approach is simple an clear: Every number that is not named is a magic number. Discussion over... for all numbers... Of course this is only a (one) definition. But this definition has inherent properties that will make it possible to evaluate it exactly.
So if you want to discuss, go for the relative approach. You will find the discussions never ending. And if you think it is over another number comes along.
Or you go and find a definition that can be used in boolean expressions that will evaluate to true or false.
I go the second way with following definition: Every unnamed number is magic. Even if you practically ignore it the statement stays the same.
100
here would just be obfuscation. However, I feel you aren't looking for a code review of that very small snippet, but for best practices regarding such cases, which unfortunately is an opinion-based question. \$\endgroup\$getPercent
! \$\endgroup\$HUNDRED
is absolutely as useless as100
, and potentially even more harmful since someone could ignore your comment and change it. If anything it proves that it matches the "practical magic number rule" and shouldn't be a variable, final or not. \$\endgroup\$