Hacker News Reader

Comment by awestroke | Hacker News Reader

Really? You could not come up with a way to avoid magic numbers?

Sometimes, a number is just a number. The average of a, b, c is (a+b+c)/3, an positive interger is single digit when less then 10, the formula for the volume of a sphere is 4/3PIr^3, etc... And that's excluding 0, 1 and 2 which are naturally everywhere.

That some numbers are not magic would be obvious to a human reviewer, but the tool probably just treats any number in an expression as a magic number or something like that, and the workaround is to define constants for raw numbers. Which entirely defeats the purpose since now, people will just use these constants for actual magic numbers and the tool will see nothing.

Maybe in rare cases, but in my experience, literals (numbers or strings) almost always have _some_ meaning where giving them a name helps readability. In my experience, it’s rare that “a number is just a number”. And sure, in those cases, naming them is silly.

Listen, what happens if we want to calculate delivery distances in a n-dimensional universe?

That 3 in your first example is definitely a magic number that should be either dynamically calculated from the number of elements being averaged, or defined as a const NUMBER_ELEMENTS_AVERAGED.

In the general case, of course you would use arrays (static or dynamic) and some kind of "size" attribute.

But this is just 3 values in an expression and using a constant could actually be bad. Let's be a bit more practical.

  int lightness(int r, int g, int b) { return (r+g+b)/3; }

Simple and straightforward

  int lightness(int r, int g, int b) {
    const int NUMBER_ELEMENTS_AVERAGED = 3;
    return (r+g+b)/NUMBER_ELEMENTS_AVERAGED; 
  }

Ok, I guess, but I think verbose for no good reason. But not as bad as the seemingly "cleaner"

  const int NUMBER_OF_COLOR_COMPONENTS = 3;
  int lightness(int r, int g, int b) {
    return (r+g+b)/NUMBER_OF_COLOR_COMPONENTS; 
  }

Imagine that you want want to add a color component, for example to support transparency (alpha). So you set NUMBER_OF_COLOR_COMPONENTS = 4, and then, your "lightness" function breaks, the simple (r+g+b)/3 would have stayed correct. That happened because didn't get the real meaning of that "3". Even if semantically, at the time you written that code, it is the number of color components, in reality, it is the number of terms in the expression. There is r, g, b: 3 terms, so 3. Who cares how many color components there are?

Side note: I know it is the wrong formula for lightness, that's just an example.

If you name it `NUMBER_ELEMENTS_AVERAGED`, then when you add a new element to average, you will miss the fact that you also need to modify that value :)

You either have them on a list and calculate it dynamically based on the size, or have it as a magic number.

Sometimes a magic number is just a magic number.

I believe even truly magic numbers don’t need to be extracted in many cases.

Say in some UI code called FooBarItem you suddenly have some call to set padding to 12. People will take this number, put it in constant and name it FOO_BAR_ITEM_PADDING = 12.

This is not better. It’s just jumping more through the code, and whatever is in the name of the constant is easily deductible from the usage pattern.

I learnt that pattern from others and nowadays I see it as useless. If you can add interesting info in the name or comments of the number, don’t bother extracting a constant.

Don't get me started on string constants. OK_LABEL_TEXT = "OK". GET_METHOD = "GET". Bonus points if this is only used in exactly one place.

Is it really difficult to see that these are pointless? If you actually think about what you are writing, that is.

Many uses of numbers aren’t magic numbers. If you want to count something then setting the counter to 0 and incrementing by 1 isn’t unexplained or subject to change.

Sometimes it's much clearer the number itself, specially when it's a mathematical formula. For instance, is

const MILLIS_PER_SECOND = 1000;

...

const durationMs = MILLIS_PER_SECOND * duration

really clearer than

const durationMs = 1000 * duration

Capital M is for Mega. I would use duration_s and duration_ms.

const duration_ms = 1000 * duration_s

And _us for microsec.

const duration_us = 1000 * duration_ms

But then the tool would probably reject my code for not following the naming conventions which ”disallows using underscores in variable names”.

Guess what I wanted to say is that there are always exceptions to the rule and there should always be some way to turn off the automatic checker for certain sections of the code.

Along the same lines, I see this a lot

    long delay = (5 * 60 * 1000); // 5 minutes, in millseconds

And it's perfectly clear to me. Now, I think the comment is really helpful there (indicating intent), but I don't think having separate constants for each of the numbers there is going to make the code better. As it is, it's very easy to read at a glance, know what it's intended to do, and determine if it's correct (should you be worried about that at the moment). Which is what's important there.

I do that as well, but typically name that variable delayMilliseconds so there's no confusion.

sleep(delay) always looks ok, but sleep(delayHours) is probably going to catch your eye as suspicious.

The latter one leaves the possibility for this though

const duration = 1000 * duration

Reply on news.ycombinator.com