Будут ли они считаться магическими числами?

Question

Я только что закончил писать программу для класса программирования, и я хочу избежать использования магических чисел, поэтому вот мой вопрос:

В приведенной ниже функции будут ли мои индексаторы массива считаться магическими числами?

Код:

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;
}

Спасибо!

Answer 1

Да, любое число, кроме -1,0 или 1, вероятно, является магическим числом.

Если вы не настоящий гуру, то вам, вероятно, также разрешено свободно использовать полномочия двух: -)

Кроме того, вы могли бы, вероятно, реорганизовать этот код, чтобы он был немного более понятным, что-то вроде:

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";
}

Это удаляет магические числа, разбросанные по всему коду, в область, где сразу видно, как они работают (при условии, что вы правильно их выровняете).

Это также устраняет проблему с вашим исходным решением в отношении того, что мы делаем с кем-то, кто набрал 380,5 баллов - это не действительно справедливо, если эти тела не пройдут :-) Или присвоить оценку "" для тех, кто превышает 400 (поскольку, похоже, нет способа вернуть "A+").

Answer 2

В моде вы делаете вещи , я бы сказал, что они не магические числа. Что бы вы их переименовали? Я не могу придумать ни одного полезного ответа (static const int One = 1; бесполезен.)

Строка 400, 381, и т. Д. Поначалу меня больше смущает. Я бы поставил что-то вроде // GPA times 100 над ним, чтобы уточнить.

На самом деле, хотя ваш вопрос (индексы массива) не слишком магический, строку 400..., вероятно, следует заменить на static const int A = 400; static const int AMinus = 381;, затем ...BARRIERS[] = {A, AMinus,} и так далее. Это определенно значимые константы

Существуют альтернативные (более чистые) методы, для которых нужны числа, которые обязательно должны быть превращены в именованные константы. (Те же, что предложены выше)

Answer 3

Как насчет того, как не сделать это для юмора?

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];
}

Answer 4

Да. Вам нужно перекомпилировать, чтобы изменить числа; вот в чем проблема.

Любые подобные настройки должны быть настраиваемыми и не требовать перекомпиляции. Конечно, у вас все еще могут быть числа в вашей конфигурации, но в вашем случае все это выглядит как допустимые данные для таблицы конфигурации.

Answer 5

это может выглядеть намного проще, например, используя std::lower_bound, чтобы найти, к какому количеству скобок относится, и массив букв, например. letter_grade[]= { "A", ... }; для преобразования скобки в буквенную оценку

Answer 6

Да, но они правильно представлены с использованием констант, поэтому проблем здесь нет.

Однако я хотел бы рассмотреть вопрос о присвоении буквенных рангов другому массиву и выравнивании их с барьерами.

И я бы определенно использовал цикл и не записывал каждый из 12 случаев отдельно.

Answer 7

Определение LETTER_GRADE_BARRIERS не связано с тем, что они на самом деле представляют, так что да.Если это был массив структур типа int и char * (знак), то нет.

Answer 8

Если вы говорите о числах, составляющих содержимое массива LETTER_GRADE_BARRIERS, я, вероятно, рассмотрю эти числа как данные , а не обязательно числа, которые заслуживают уникальные имена.

Я бы предположил, что в идеале они должны исходить из файла данных, а не встраиваться в программу, но ваше назначение / требования могут диктовать иное.

Однако числа, используемые для индекса массива, вполне могут заслуживать имен.

Answer 9

Да. Индексы в массиве не имеют никакого семантического значения. Это делает их «волшебными».

Ответ paxdiablo - довольно хороший способ сделать это, хотя я бы соблазнился объединить имя лимита и класса в один класс / структуру.

Даже сохраняя структуру кода, рассмотрим эти два фрагмента:

// 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";
}

Второй пример придает больше семантического значения коду, а не полагается на то, что обозначают «13» и «14».

Хранение их в массиве мало что дает, так как вы фактически не выполняете итерации по массиву.

Хорошей проверкой магических чисел является описание решения проблемы кому-либо. Если числа не отображаются в вашем словесном описании, они почти наверняка волшебны.

Answer 10

Да, они, безусловно, магические числа. То, как вы это делаете, тоже не помогает. Все эти числа разнесены на 20 шагов (с дополнительным буфером +1 перед каждым), но это не видно из кода Гораздо лучшая реализация будет выглядеть примерно так:

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)];
  }
}