Named numbers as variables [closed]

Solution 1:

Naming numbers is terrible practice, one day something will need to change, and you'll end up with unsigned five = 7.

If it has some meaning, give it a meaningful name. The 'magic number' five is no improvement over the magic number 5, it's worse because it might not actually equal 5.

This kind of thing generally arises from some cargo-cult style programming style guidelines where someone heard that "magic numbers are bad" and forbade their use without fully understanding why.

Solution 2:

Well named variables

Giving proper names to variables can dramatically clarify code, such as

constant int MAXIMUM_PRESSURE_VALUE=2;

This gives two key advantages:

• The value MAXIMUM_PRESSURE_VALUE may be used in many different places, if for whatever reason that value changes you need to change it in only one place.

• Where used it immediately shows what the function is doing, for example the following code obviously checks if the pressure is dangerously high:

  if (pressure>MAXIMUM_PRESSURE_VALUE){
      //without me telling you you can guess there'll be some safety protection in here
  }
  

Poorly named variables

However, everything has a counter argument and what you have shown looks very like a good idea taken so far that it makes no sense. Defining TWO as 2 doesn't add any value

constant int TWO=2;

• The value TWO may be used in many different places, perhaps to double things, perhaps to access an index. If in the future you need to change the index you cannot just change to int TWO=3; because that would affect all the other (completely unrelated) ways you've used TWO, now you'd be tripling instead of doubling etc

• Where used it gives you no more information than if you just used "2". Compare the following two pieces of code:

  if (pressure>2){
      //2 might be good, I have no idea what happens here
  }
  

  or

  if (pressure>TWO){
      //TWO means 2, 2 might be good, I still have no idea what happens here
  }
  

• Worse still (as seems to be the case here) TWO may not equal 2, if so this is a form of obfuscation where the intention is to make the code less clear: obviously it achieves that.

The usual reason for this is a coding standard which forbids magic numbers but doesn't count TWO as a magic number; which of course it is! 99% of the time you want to use a meaningful variable name but in that 1% of the time using TWO instead of 2 gains you nothing (Sorry, I mean ZERO).

^{this code is inspired by Java but is intended to be language agnostic}

Solution 3:

Short version:

• A constant five that just holds the number five is pretty useless. Don't go around making these for no reason (sometimes you have to because of syntax or typing rules, though).
• The named variables in Quaternion.cs aren't strictly necessary, but you can make the case for the code being significantly more readable with them than without.
• The named variables in ext4/resize.c aren't constants at all. They're tersely-named counters. Their names obscure their function a bit, but this code actually does correctly follow the project's specialized coding standards.

What's going on with Quaternion.cs?

This one's pretty easy.

Right after this:

double zero = 0;
double one = 1;

The code does this:

return zero.GetHashCode() ^ one.GetHashCode();

Without the local variables, what does the alternative look like?

return 0.0.GetHashCode() ^ 1.0.GetHashCode(); // doubles, not ints!

What a mess! Readability is definitely on the side of creating the locals here. Moreover, I think explicitly naming the variables indicates "We've thought about this carefully" much more clearly than just writing a single confusing return statement would.

What's going on with resize.c?

In the case of ext4/resize.c, these numbers aren't actually constants at all. If you follow the code, you'll see that they're counters and their values actually change over multiple iterations of a while loop.

Note how they're initialized:

unsigned three = 1;
unsigned five = 5;
unsigned seven = 7;

Three equals one, huh? What's that about?

See, what actually happens is that update_backups passes these variables by reference to the function ext4_list_backups:

/*
 * Iterate through the groups which hold BACKUP superblock/GDT copies in an
 * ext4 filesystem.  The counters should be initialized to 1, 5, and 7 before
 * calling this for the first time.  In a sparse filesystem it will be the
 * sequence of powers of 3, 5, and 7: 1, 3, 5, 7, 9, 25, 27, 49, 81, ...
 * For a non-sparse filesystem it will be every group: 1, 2, 3, 4, ...
 */
static unsigned ext4_list_backups(struct super_block *sb, unsigned *three,
                                  unsigned *five, unsigned *seven)

They're counters that are preserved over the course of multiple calls. If you look at the function body, you'll see that it's juggling the counters to find the next power of 3, 5, or 7, creating the sequence you see in the comment: 1, 3, 5, 7, 9, 25, 27, &c.

Now, for the weirdest part: the variable three is initialized to 1 because 3⁰ = 1. The power 0 is a special case, though, because it's the only time 3^x = 5^x = 7^x. Try your hand at rewriting ext4_list_backups to work with all three counters initialized to 1 (3⁰, 5⁰, 7⁰) and you'll see how much more cumbersome the code becomes. Sometimes it's easier to just tell the caller to do something funky (initialize the list to 1, 5, 7) in the comments.

So, is five = 5 good coding style?

Is "five" a good name for the thing that the variable five represents in resize.c? In my opinion, it's not a style you should emulate in just any random project you take on. The simple name five doesn't communicate much about the purpose of the variable. If you're working on a web application or rapidly prototyping a video chat client or something and decide to name a variable five, you're probably going to create headaches and annoyance for anyone else who needs to maintain and modify your code.

However, this is one example where generalities about programming don't paint the full picture. Take a look at the kernel's coding style document, particularly the chapter on naming.

GLOBAL variables (to be used only if you really need them) need to have descriptive names, as do global functions. If you have a function that counts the number of active users, you should call that "count_active_users()" or similar, you should not call it "cntusr()".

...

LOCAL variable names should be short, and to the point. If you have some random integer loop counter, it should probably be called "i". Calling it "loop_counter" is non-productive, if there is no chance of it being mis-understood. Similarly, "tmp" can be just about any type of variable that is used to hold a temporary value.

If you are afraid to mix up your local variable names, you have another problem, which is called the function-growth-hormone-imbalance syndrome. See chapter 6 (Functions).

Part of this is C-style coding tradition. Part of it is purposeful social engineering. A lot of kernel code is sensitive stuff, and it's been revised and tested many times. Since Linux is a big open-source project, it's not really hurting for contributions — in most ways, the bigger challenge is checking those contributions for quality.

Calling that variable five instead of something like nextPowerOfFive is a way to discourage contributors from meddling in code they don't understand. It's an attempt to force you to really read the code you're modifying in detail, line by line, before you try to make any changes.

Did the kernel maintainers make the right decision? I can't say. But it's clearly a purposeful move.