Good Code And Bad Code

[kodax]

For production code, is it better to write 1 line ?

x = ((x / 15) / 13) / 12;

or is it better to write multiple lines ?

x = x / 15;
x = x / 13;
x = x / 12;

[stahta01]

1. It is better to not use magic numbers
2. Multiply is easier than divide
3. Using longer variable names than one letter is preferred unless loop variable

x /= (15*13*12); // But, use constants or defines in place of the numbers.

Tim S.

[Salem]

It would certainly need a comment explaining the significance of 15, 13 and 12.


Regardless, the compiler is likely to reduce it to
x /= 2340;

So however you write it, choose the way that makes it most obvious to the reader what the purpose of the code is.

[kodax]

15 * 13 * 12, big values will overflow.

What is a "magic" number ?

x = ((((x / 15) / 13) / 12) / 11) / 10;
My point was that one line leads to clutter ?
Is clutter considered terrible for production code ?

[Salem]

What overflow?
Those values are comfortably in range of any size of int you will come across.


> What is a "magic" number ?
Any number (with the possible exception of zero) you write as a number in the code.

x = 12 ;  // a magic number
// later on
y = 12;  // ?  Is this the same 12 as above, or are you counting something else here?

Compare with

const int NUMBER_OF_ELEPHANTS = 12;
// later on
x = NUMBER_OF_ELEPHANTS;  // symbolic representation.
// later on
y = NUMBER_OF_ELEPHANTS;

If you later decide you need more elephants, it's an obvious 1-line change.
Trying to globally replace 12 is sure to screw up eventually.

[Schol-R-LEA-2]

The Wikipedia entry on the topic actually gives a reasonably good explanation of why naming constants is generally desirable.

[Schol-R-LEA-2]

is the given line x = ((x / 15) / 13) / 12; from the actual code you have in mind, or is it just an example? Given that you expand on it in your later post to x = ((((x / 15) / 13) / 12) / 11) / 10;, I am assuming that it is only meant to be an example, but if so, let us know.

Is clutter considered terrible for production code ?

It can be a problem, yes, but in this case, the lack of transparency about what the constants mean is the bigger issue. What, for example, does the 15 refer to? is it a purely mathematical constant, or is it a value from some derived source? As it is now, the 15 could be anything, as could the 13 and 12. Are they scaling factors? Stoichiometric dimensional transformations (for changing the units of some measurement to a different type of units, e.g., pounds to newtons)? Part of a numeric series? The dimensions of a jagged multidimensional array? Some combination of the above? The code gives no clue.

What does x refer to? What is it's specific type (since you mentioned overflow, which implies that it is a very small range - any of the common sizes larger than a single byte should not overflow with the values involved)? Again, this is where a using a more descriptive variable name would help - outside of its context, a single letter variable name such as x could literally mean anything.

Perhaps we could use some more details about what kind of program the code is from.

[kodax]

Another example of clutter
u_int64_t filesize_in_bytes = channels * bytes_per_sample * samples_per_second * duration_in_second;

Refer to my post audio bitrate calculation

[Salem]

How is that cluttered?

Four symbolic names are very descriptive of what's being done.

[hamster_nz]

The other side issue here is the meaning or units of 'x' changes

Take as an example:

    x = x / 60 / 60 / 24;

Now your question is... "Is this better?":

    x = x / 60;
    x = x / 60;
    x = x / 24;

Here's the sort of things that others have been saying are better:

    x /= (60 * 60 * 24);

Or:

    x = x / SECS_PER_MIN / MINS_PER_HOUR / HOURS_PER_DAY;

They also seem slightly lacking to me because the contents of 'x' changes from being seconds to days (although the last is the best of the bunch).

I would have a preference to see

     duration_days = duration_seconds / (SECS_PER_MIN * MINS_PER_HOUR * HOURS_PER_DAY);

It guards against things like when people move blocks of code around, and suddenly they get charged for 432,000 days, rather than just five.

    x = end_time - start_time;;
    charge_per_second_usage_charge(x);
    ... many lines of code ...
    x /= 24*60*60;
    ... many lines of code ...
    charge_conection_fee(x);

Somebody might refactor or "tidy up" the code and get:

    x = end_time - start_time;;
    charge_per_second_usage_charge(x);
    charge_conection_fee(x);
    ... many lines of code ...
    x /= 24*60*60;
    ... many lines of code ...

Here's another example of that kind of thing, where a variable has inconsistent use:

    x = 0;
    for(int i = 0; i < n ; i++)
        x += value[i];
    x /= n;

'x' is acting in two different roles. I would prefer to code:

    total = 0;
    for(int i = 0; i < n ; i++)
        total += value[i];'
    average = total / n;