Будут ли они считаться магическими числами?
Я только что закончил писать программу для класса программирования, и я хочу избежать использования магических чисел, поэтому вот мой вопрос:
В функции ниже, будут ли мои индексаторы массива считаться магическими числами?
Код:
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;
}
Спасибо!
10 ответов
Да, любое число, кроме -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+").
В моде вы делаете вещи, я бы сказал, что они не магические числа. Что бы вы их переименовали? Не могу придумать ни одного полезного ответа (static const int One = 1; бесполезно.)
400, 381, и т. д. линия вначале меня больше смущает. Я бы поставил что-то вроде // GPA times 100 выше это уточнить.
На самом деле, хотя ваш вопрос (индексы массивов) не слишком волшебен, 400... строка, вероятно, должна быть заменена на static const int A = 400; static const int AMinus = 381; затем ...BARRIERS[] = {A, AMinus,} и так далее. Это определенно значимые константы
Существуют альтернативные (более чистые) методы, которым нужны числа, которые обязательно должны быть превращены в именованные константы. (Те же, что предложены выше)
Как насчет того, как не сделать это для юмора?
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];
}
Да. Вам нужно перекомпилировать, чтобы изменить числа; вот в чем проблема.
Любые подобные настройки должны быть настраиваемыми и не требовать перекомпиляции. Конечно, у вас все еще могут быть числа в вашей конфигурации, но в вашем случае все это выглядит как допустимые данные для таблицы конфигурации.
Определение LETTER_GRADE_BARRIERS не зависит от того, что они на самом деле представляют, так что да. Если это был массив структур типа int и char* (знак), то нет.
Это может выглядеть намного проще, например, используя std::lower_bound чтобы найти, к какому количеству скобок относится, и массив букв, например, letter_grade[]= { "A", ... }; преобразовать скобку в буквенную оценку
Да, но они правильно представлены с использованием констант, поэтому проблем нет.
Однако я хотел бы рассмотреть вопрос о присвоении буквенных рангов другому массиву и сопоставлении их с барьерами.
И я бы определенно использовал цикл и не записывал каждый из 12 случаев отдельно.
Да. Индексы в массиве не имеют никакого семантического значения. Это делает их "волшебными".
Ответ 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".
Хранение их в массиве мало что дает, так как вы фактически не выполняете итерации по массиву.
Хорошая проверка для магических чисел - это описание решения проблемы кому-то. Если числа не отображаются в вашем словесном описании, они почти наверняка волшебны.
Если вы говорите о числах, которые составляют содержание LETTER_GRADE_BARRIERS массив, я бы, вероятно, рассматривал эти числа как данные, а не обязательно числа, которые заслуживают уникальных имен.
Я предполагаю, что в идеале они должны исходить из файла данных, а не встраиваться в программу, но ваше назначение / требования могут диктовать иное.
Однако числа, используемые для индексации массива, вполне могут заслуживать имен.
Да, это определенно магические числа. То, как вы это делаете, тоже не помогает. Все эти числа разнесены на 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)];
}
}