Примеры неудачного стиля программирования

Часть 1

1. Дано:
   
   1 2 3 4

   #define ONE 1 #define TEN 10

   Пояснение:
   Если понадобится модифицировать программу, а именно увеличить размер массива из TEN элементов до длины 20, то получится константа c именем TEN равна 20, что противоречиво.

   Решение:
   Заменить названия ONE, TEN хотя бы на INPUT_SIZE, OUTPUT_SIZE.

2. Дано:
   1 2 3 4 5

   for (theElementIndex=0; theElementIndex < numberOfElements; theElementIndex++)      elementArray[theElementIndex] = theElementIndex;

   Пояснение:
   Слишком длинные имена переменных. Для локальных переменных лучше всегда придумывать короткие имена. Для циклов принято использовать i и j, для указателей - p и q, для строк - s и t.

   Решение:

   1 2 3 4

   for (i = 0; i < nelems; i++)     elem[i] = i;

    

3. Дано:
   1 2 3 4 5

   for (n++;n<100;field[n++]='\0'); *i='\0'; return('\n');

   Пояснение:
   Цикл лучше переписать так, чтобы присваивание оказалось в теле цикла и инкрементирование счетчика осуществлялось в третьей секции заголовка цикла. Кроме того, необходимо добавить отступы, пробелы, переходы на новую строку.

   Решение:

   1 2 3 4 5 6

   for (n++; n < 100; n++)     field[n] = '\0'; *i = '\0'; return('\n');

    

4. Дано:
   1 2	if (!(id < block_id) || !(id >= unblock_id))

   Пояснение:
   Каждая из проверок в условии записана в форме отрицания, хотя в обоих случаях для этого нет никакой необходимости. Следует писать выражения так же, как бы вы читали их вслух, то есть в естественной форме. Условные выражения с операцией отрицания всегда труднее понять. Ниже приведен улучшенный вариант. Заметим, что в нем присутствуют лишние скобки. Это не случайность, так как принято использовать скобки не только ради установления приоритетов операций, но и для удобочитаемости кода.

   Решение:

   1 2 3

   if  ((id >= block_id) || (id < unblock_id))

5. Дано:
   1 2 3 4

   *x += (*xp=(2*k < (n-m) ? c[k+1] : d[k--]));

   Пояснение:
   Сложные выражения лучше разбить на несколько отдельных фрагментов.

   Решение:

   1 2 3 4 5 6

   if (2*k < n-m)     *xp = c[k+1]; else     *xp = d[k--]; *x += *xp;

    

6. Дано:
   1 2 3 4 5 6

   class UserQueue {     int noOfItemsInQ, frontOfTheQueue, queueCapacity;     public int nOfUsersInQueue() { ... } }

   Пояснение:
   Кроме того, что имена слишком длинные, они еще не согласуются между собой. Слово queue встречается как Q, Queue, queue. Лучше свести только к одному виду. Однако, эти переменные являются членами класса UserQueue, потому слово queue можно вообще не упоминать в их именах, потому как вызов queue.queueCapacity явно содержит избыточную информацию.

   Решение:

   1 2 3 4 5

   class UserQueue {     int nitems, front, capacity;     public int nusers() { ... } }

7. Дано:
   1 2 3 4 5 6

   switch (shape.type) { case Shape_Circle: shape.DrawCircle(); break; case Shape_Square: shape.DrawSquare(); break; ... }

   Пояснение:
   Здесь методы shape.DrawCircle() и shape.DrawSquare() следует заменить на единственный метод shape.Draw(), поддерживающий рисование и окружностей, и прямоугольников. Хотя иногда блоки case служат для разделения действительно различных видов объектов или действий.

8. Дано:
   
   1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26

   void HandleStuff( CORP_DATA & inputRec, int crntQtr, EMP_DATA empRec, double & estimRevenue, double ytdRevenue, int screenX, int screenY, COLOR_TYPE & newColor, COLOR_TYPE & prevColor, StatusType & status, int expenseType ) { int i; for ( i = 0; i < 100; i++ ) {    inputRec.revenue[i] = 0;    inputRec.expense[i] = corpExpense[ crntQtr ][ i ]; } UpdateCorpDatabase( empRec ); estimRevenue = ytdRevenue * 4.0 / (double) crntQtr; newColor = prevColor; status = SUCCESS; if ( expenseType == 1 ) {    for ( i = 0; i < 12; i++ )       profit[i] = revenue[i] - expense.type1[i]; } else if ( expenseType == 2 ) {    profit[i] = revenue[i] - expense.type2[i]; } else if ( expenseType == 3 )    profit[i] = revenue[i] - expense.type3[i]; }

   Пояснение:
   а) неудачное имя HandleStuff ничего не говорит о роли метода;
   б) метод недокументирован;
   в) метод плохо форматирован, физическая организация кода почти не дает представления о его логической организации, стратегия форматирования используется непоследовательно: сравните стили операторов if с условиями expenseType==2 и expenseType==3;
   г) Входная переменная inputRec внутри метода изменяется. Если это входная переменная, изменять ее нежелательно (в случае C++ ее следовало бы объявить как const). Если изменение значения предусмотрено, переменную не стоило называть inputRec;
   д) Метод читает и изменяет глобальные переменные: читает corpExpense и изменяет profit. Взаимодействие этого метода с другими следовало бы сделать более непосредственным, без использования глобальных переменных;
   е) Цель метода размыта. Он инициализирует ряд переменных, записывает данные в БД, выполняет вычисления — все эти действия не кажутся связанными между собой. Метод должен иметь одну четко определенную цель;
   ж) Метод не защищен от получения плохих данных. Если переменная crntQtr равна О, выражение ytdRevenue * 4.0 / (double) crntQtr вызывает ошибку деления на 0;
   з) Метод использует несколько «магических» чисел: 100, 4.0, 12, 2 и 3;
   и) Параметры screenX и screenY внутри метода не используются;
   к) Параметр prevColor передается в метод неверно: он передается по ссылке (&), но значение ему внутри метода не присваивается;
   л) Метод принимает слишком много параметров. Как правило, чтобы параметры можно было охватить умом, их должно быть не более 7 — этот метод принимает 11. Параметры представлены таким неудобочитаемым образом, что большинство разработчиков даже не попытаются внимательно изучить их или хотя бы подсчитать;
   м) Параметры метода плохо упорядочены и не документированы.

9. Дано:
   
   1 2 3 4 5 6

   class Employee { ...     public TaxId GetTaxId throws EOFException { ... } ... }

   Пояснение:
   Функция GetTaxId() передает низкоуровневое исключение EOFException вызывающей стороне. Она не обрабатывает исключение сама, а раскрывает некоторые детали своей реализации, генерируя низкоуровневое исключение. Это привязывает клиентский код не к классу Employee, а к коду внутри класса Employee, генерирующему исключение EOFException. Инкапсуляция нарушена, управляемость кода ухудшается. Вместо этого код GetTaxIdQ должен передавать исключение, соответствующее интерфейсу класса, частью которого он является.