views:

370

answers:

10

Hi,

I've just completed writing a program for a programming class, and I want to avoid use of magic numbers, so here's my question:

In the function below, would my array indexers be considered magic numbers?

Code:

string CalcGrade(int s1, int s2, int s3, double median)
{
const int SIZE = 23;
const int LETTER_GRADE_BARRIERS[SIZE] = { 400, 381, 380, 361, 360, 341, 340, 321, 320, 301, 300, 281, 280, 261, 260, 241, 240, 221, 220, 201, 200, 181, 180 }; 
double finalGrade;
string letterGrade;

finalGrade = s1 + s2 + s3 + median;

if (finalGrade >= LETTER_GRADE_BARRIERS[1] && finalGrade <= LETTER_GRADE_BARRIERS[0])
{
    letterGrade = "A";
}
else if (finalGrade >= LETTER_GRADE_BARRIERS[3] && finalGrade <= LETTER_GRADE_BARRIERS[2])
{
    letterGrade = "A-";
}
else if (finalGrade >= LETTER_GRADE_BARRIERS[5] && finalGrade <= LETTER_GRADE_BARRIERS[4])
{
    letterGrade = "B+";
}
else if (finalGrade >= LETTER_GRADE_BARRIERS[7] && finalGrade <= LETTER_GRADE_BARRIERS[6])
{
    letterGrade = "B";
}
else if (finalGrade >= LETTER_GRADE_BARRIERS[9] && finalGrade <= LETTER_GRADE_BARRIERS[8])
{
    letterGrade = "B-";
}
else if (finalGrade >= LETTER_GRADE_BARRIERS[11] && finalGrade <= LETTER_GRADE_BARRIERS[10])
{
    letterGrade = "C+";
}
else if (finalGrade >= LETTER_GRADE_BARRIERS[13] && finalGrade <= LETTER_GRADE_BARRIERS[12])
{
    letterGrade = "C";
}
else if (finalGrade >= LETTER_GRADE_BARRIERS[15] && finalGrade <= LETTER_GRADE_BARRIERS[14])
{
    letterGrade = "C-";
}
else if (finalGrade >= LETTER_GRADE_BARRIERS[17] && finalGrade <= LETTER_GRADE_BARRIERS[16])
{
    letterGrade = "D+";
}
else if (finalGrade >= LETTER_GRADE_BARRIERS[19] && finalGrade <= LETTER_GRADE_BARRIERS[18])
{
    letterGrade = "D";
}
else if (finalGrade >= LETTER_GRADE_BARRIERS[21] && finalGrade <= LETTER_GRADE_BARRIERS[20])
{
    letterGrade = "D-";
}
else if (finalGrade <= LETTER_GRADE_BARRIERS[22])
{
    letterGrade = "Fail";
}

return letterGrade;
}

Thanks!

+4  A: 

In the fashion you are doing things, I would say they are not magic numbers. What would you rename them? I can't think of any useful answer (static const int One = 1; is useless.)

The 400, 381, etc. line is more confusing to me at first. I would put something like // GPA times 100 above it to clarify.

In fact, while your question (array indexes) isn't too magical, the 400... line should probably be replaced with static const int A = 400; static const int AMinus = 381; then ...BARRIERS[] = {A, AMinus,} and so on. Those are definitely meaningful constants

There are alternate (cleaner) methods that would need numbers that should definitely be turned into named constants. (The same ones suggested above)

GMan
I'm always wary of saying stuff like "The same ones suggested above" in case the answer rises to the top. Then you just look silly :-)
paxdiablo
@pax There, I up-voted the answers above me, haha!
GMan
+1  A: 

The definition of LETTER_GRADE_BARRIERS is disjoint from what they actually represent, so yes. If it was an array of structs of an int and a char* (the mark) then no.

Ignacio Vazquez-Abrams
+4  A: 

Yes. You need to recompile to change the numbers; that's where the problem lies.

Any configuration things like that should be, well, configurable, and not require a recompliation. Of course, you may still have numbers in your config, but in your case, it all seems like legitimate data for a configuration table.

Noon Silk
Magic numbers don't necessarily have anything to do with run-time configuration. Anywhere in the code where you use a hardcoded value who's meaning isn't apparent is considered a magic number, and should be replaced by a named constant. Would be great if it was also configurable, but it doesn't need to be by definition.
womp
womp: I don't disagree; but magic numbers tend to go hand in hand with an adjustment to a configuration-based approach, in my experience. At least, in this specific case, it would make the code significantly better, and remove any question of magic numbers; it would just leave "specific configuration" :)
Noon Silk
While it might be desirable in this case to configure the values at runtime, the part about "You need to recompile to change the numbers" isn't a general rule. There are plenty of places where it's appropriate to use named, compile-time constants. For example, `NULL`, `EOF`, `INT_MAX`, `RAND_MAX`, `CHAR_BIT`, .....
jamesdlin
jamesdlin: Null? Really? but for the others; I'd think that pretty much goes without saying, no? You're right, obviously.
Noon Silk
+14  A: 

Yes, any number other than -1,0 or 1 is probably a magic number.

Unless you're a real guru, then you're probably allowed to use powers of two freely as well :-)

As an aside, you could probably refactor that code to be a little more understandable, something like:

string CalcGrade (int s1, int s2, int s3, double median) {
    // Grade lookup arrays. If grade is >= limit[n], string is grades[n].
    // Anything below D- is a fail.
    static const int Limits[] = {400, 380, 360, 340,320, 300, 280,260, 240, 220,200,180 }; 
    static const int Grades[] = {"A+","A","A-","B+","B","B-","C+","C","C-","D+","D","D-"};

    double finalGrade = s1 + s2 + s3 + median;

    // Check each element of the array and, if the final grade is greater
    //   than or equal to, return the grade string.
    for (int i = 0; i < sizeof(Limits) / sizeof(*Limits); i++)
        if (finalGrade >= Limits[i])
            return Grades[i];

    // Otherwise, failed.
    return "Fail";
}

This removes the magic numbers spread all over the code to an area where it's immediately obvious how they work (assuming you align them nicely).

It also removes a problem with your original solution as to what we do with someone that achieved a score of 380.5 - it's not really fair to fail those bods :-) Or to assign a grade to "" to those above 400 (since there doesn't appear to be a way to return "A+").

paxdiablo
I'm not defining a macro for the initial size of the next hashtable I create, and I'm not using 1 either!
Pascal Cuoq
Hey pax, why are you returning `new string`? The return value is not a pointer. :P Just `return Grades[i];` and `return "Fail"` will do.
GMan
@GMan, I wasn't sure that was kosher (it's been a while since I did C++) but, if char pointers are automatically constructed into string objects, you can change the code to do that. It doesn't really have a bearing on the simplification though so I won't bother about it myself.
paxdiablo
I done it. :​​3 EDIT: Now you done it. :3 EDIT²: No problem. :3
GMan
Sorry, @GMan, we had crossed edits, I've incorporated your suggestion, thanks.
paxdiablo
pax: You've written it better, and significantly clearer, no doubt, but it still suffers from the same issues as the OP as actually asking about (i.e., I think it's a little distracting from the main point).
Noon Silk
@silky, magic numbers are only magic (IMNSHO) when their intent is not immediately clear. By structuring the numbers as I have and adding the two-line comment, I challenge anyone to find a person with an IQ greater than Sponge Bob Squarepants that cannot see the intent :-)
paxdiablo
Since the steps are uniform, there's no need to loop or to have an explicit limits array. You could simply divide the difference between the maximum grade and the actual one by 20 and you'd get an index into the grades array. Clearer and more efficient.
Max Shawabkeh
pax: I suppose I like to demagicify numbers by moving to configuration, as opposed to trying to explain them slightly better. Perhaps it's just a general disagreement, and I'm happy to leave it at that :) And I think we both know the kind of intelligence that you can find in the programming field :P
Noon Silk
I thought about that, @MaxS, but still opted for the explicit checks on the off-chance they may not necessarily be uniform. And I *still* think it's clearer with the limits and grades lined up rather than a formula. But that's just a matter of taste, I'm not violently opposed to a formulaic approach.
paxdiablo
I agree with pax's approach there. Moving to a configuration file is not going to remove the mapping between the score and the grade. I would consider that anything that is NOT repeated is not a magic number either.
Matthieu M.
+1  A: 

Yes, but they are properly represented using constants, so no problems there.

I would, however, consider assigning the letter grades to another array and aligning them with the barriers.

And I would definitely use a loop and not write out each of the 12 cases seperately.

aib
+1  A: 

it can look a lot simpler, for example using std::lower_bound to find which bracket score belongs to and array of letters , e.g. letter_grade[]= { "A", ... }; to convert bracket to a letter grade

aaa
A: 

Yes, they are most definitely magic numbers. The way you are going about it doesn't help either. All these numbers are spaced 20 steps apart (with an extra +1 buffer before each) but that is not apparent from the code. A much better implementation would be something like this:

string CalcGrade(int s1, int s2, int s3, double median) {
  const int MAXIMUM_GRADE = 400;
  const int MINIMUM_GRADE = 180;
  const int GRADE_STEP = 20;
  const char* GRADES[] = { "A", "A-", "B+", "B", "B-", "C+", "C", "C-", "D+", "D", "D-" };

  double finalGrade = s1 + s2 + s3 + median;

  if (finalGrade >= MAXIMUM_GRADE) {
    return "A+";
  } else if (finalGrade <= MINIMUM_GRADE) {
    return "Fail";
  } else {
    return GRADES[(size_t)((MAXIMUM_GRADE - finalGrade) / GRADE_STEP)];
  }
}
Max Shawabkeh
A: 

Yes. The indices into the array have no semantic meaning whatsoever. This makes them 'magic.'

paxdiablo's response is a pretty good way of doing it, though I'd be tempted to combine the limit and grade name into a single class/struct.

Even keeping the code structure, consider these two fragments:

// original
else if (finalGrade >= LETTER_GRADE_BARRIERS[13] && finalGrade <= LETTER_GRADE_BARRIERS[12]) 
{ 
    letterGrade = "C"; 
} 

// compared to
else if (finalGrade >= MIN_C_GRADE && finalGrade < MIN_C_PLUS_GRADE)
{
    letterGrade = "C";
}

The second sample attaches more semantic meaning to the code, rather than relying upon what '13' and '14' represent.

Storing them in an array buys you little, as you're not actually iterating over the array.

A good check for magic numbers is to describe the solution to the problem to someone. If the numbers don't show up in your verbal description, they're almost certainly magic.

kyoryu
A: 

If you're talking about the numbers that make up the contents of the LETTER_GRADE_BARRIERS array, I'd probably consider those numbers as data, not necessarily numbers that deserve unique names.

I'd guess that ideally they would come from a data file rather than embedded in the program, but your assignment/requirements might dictate otherwise.

However, the numbers that are used to index the array might well deserve names.

Michael Burr
+3  A: 

How about how not to do it for a bit of humour?

string CalcGrade (int s1, int s2, int s3, double median) {
    int grade = median + s1 + s2 + s3;
    grade = (grade>400)?400:((grade<180)?179:grade);
    return
        "Fail\0D-\0\0\0D\0\0\0\0D+\0\0\0C-\0\0\0C\0\0\0\0"C+\0\0\0"
        "B-\0\0\0B\0\0\0\0B+\0\0\0A-\0\0\0A\0\0\0\0A+"[((grade-160)/20)*5];
}
paxdiablo
How about: (Yes, formatting will be "commentized") `string CalcGrade(int _, int __, int ___, double ____){ int _____ = ____ + _ + __ + ___; _____ = ( _____>400)?400:(( _____<180)?179: _____); return "Fail\0D-\0\0\0D\0\0\0\0D+\0\0\0C-\0\0\0C\0\0\0\0"C+\0\0\0B-\0\0\0B\0\0\0\0B+\0\0\0A-\0\0\0A\0\0\0\0A+"[(( _____-160)/20)*5];}`
GMan
+1, @Gman, it looks like a form where I'm supposed to fill in the blanks :-)
paxdiablo
Heh. Don't forget `#define x int` and `#define y string` and `#define z double` and `#define w 20` and `#define t 5`, so it becomes: `y CalcGrade(x _,x __,x ___,z ____){x _____=____+_+__+___;_____=(_____>(w*w))?(w*w):((_____<((2*t-1)*w))?((2*t-1)*w-1):_____);return "Fail\0D-\0\0\0D\0\0\0\0D+\0\0\0C-\0\0\0C\0\0\0\0"C+\0\0\0B-\0\0\0B\0\0\0\0B+\0\0\0A-\0\0\0A\0\0\0\0A+"[(( _____-(w*(2*(w/t))))/w)*(w/(t-1))];} `
GMan
And our trigraph friends: `y CalcGrade(x _,x __,x ___,z ____)??<x _____=____+_+__+___;_____=(_____>(w*w))?(w*w):((_____<((2*t-1)*w))?((2*t-1)*w-1):_____);return "Fail\0D-\0\0\0D\0\0\0\0D+\0\0\0C-\0\0\0C\0\0\0\0"C+\0\0\0B-\0\0\0B\0\0\0\0B+\0\0\0A-\0\0\0A\0\0\0\0A+"??((( _____-(w*(2*(w/t))))/w)*(w/(t-1))??);??>`
GMan