Аннотация

Это третья статья, где я хочу рассказать про новую пару приёмов при программировании, которые помогут сделать код более простым и надежным. С предыдущими двумя заметками можно познакомиться здесь [1] и здесь [2]. В этот раз примеры будут взяты из проекта Qt.

Введение

Проект Qt 4.7.3. попался мне для изучения не случайно. Пользователи PVS-Studio обратили внимание, что анализ как-то слабоват, если дело касается проверки проектов, построенных на основе библиотеки Qt. Это не удивительно. Статический анализ позволяет находить ошибки за счет того, что смотрит на код более высокоуровнево, чем компилятор. Следовательно, он должен знать определенные паттерны кода и, что делают функции различных библиотек. В противном случае, он пройдет мимо многих замечательных ляпов. Поясню на примере:



Бессмысленно сравнивать строку саму с собой. Но компилятор промолчит. Он не задумывается, в чем суть функции strcmp(). У него хватает своих забот. А вот статические анализаторы могут заподозрить неладное. В Qt есть своя разновидность функции сравнения строк - qstrcmp(). И, соответственно, анализатор нужно обучить обращать внимание и на такую строку:



Осваивание библиотеки Qt и создание специализированных проверок - это большая и планомерная работа. И началом этой работы стала проверка самой библиотеки.

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

1. Обрабатывайте переменные в той же последовательности, как они объявлены

Код библиотеки Qt очень качественен и практически не содержит ошибок. Зато в нём обнаружилось большое количество излишних инициализаций, излишних сравнений или лишних копирования значений переменных.

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



Здесь два раза продублировано одно и то же сравнение. Это не ошибка, но совершенно избыточный код. Другой аналогичный пример:



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

1. Дубликаты увеличивают длину кода. А чем длиннее код, тем легче добавить еще один дубликат.

2. Если мы захотим изменить логику программы и удалим одну проверку или одно присваивание, то дубликат этого действия может подарить вам несколько часов увлекательной отладки. Сами представьте, вы пишите "tos = 1" (см. первый пример), а потом в другой части программы удивляетесь, отчего же по-прежнему "tos" равен нулю.

3. Замедление скорости работы. Практически всегда им можно пренебречь в таких ситуациях, но оно всё-таки есть.

Надеюсь, убедил, что дубликатам не место в нашем коде. Как с ними бороться? Как правило, подобные инициализации/сравнения идут блоком. И есть такой же блок переменных. Рационально писать код так, чтобы порядок объявлений переменных и порядок работы с ними совпадал. Приведу пример не очень хорошего кода:



Естественно, это схематичный пример. Смысл в том, что когда инициализация не последовательна, то намного легче написать две одинаковых строки. В приведенном коде дважды инициализируется переменная 'q'. И ошибка плохо заметна, если просматривать код бегло. Если теперь инициализировать переменные в той же последовательности, как они объявлены, то подобной ошибке просто не будет места. Улучшенный вариант кода:



Конечно, я знаю, что не всегда так можно написать и работать с переменными в порядке их объявления. Но часто это возможно и полезно. Дополнительным плюсом станет то, что вам легче будет ориентироваться в коде. 

Рекомендация. Добавляя новую переменную, постарайтесь, чтобы её инициализация и обработка, осуществлялась в соответствии с её положением относительно других переменных.

2. Табличные методы - это хорошо.

Про табличные методы хорошо написано у С. Макконнелла в книге "Совершенный код" в главе N18 [3]:

Табличный метод - это схема, позволяющая искать информацию в таблице, а не использовать для этого логические выражения, такие как if и case. Практически все, что вы можете выбирать посредством логических операторов, можно выбирать, применяя таблицы. В простых случаях логические выражения проще и понятней. Но при усложнении логических построений таблицы становятся всё привлекательнее.

Так вот, очень жаль, что программисты по-прежнему любят огромные switch() или густой лес конструкций if-else. Перебороть в себе это очень сложно. Кажется, ну еще-то один "case:" или маленький "if" не повредит. Но он вредит. И неудачно добавляют новые условия даже самые опытные программисты. В качестве примера пара дефектов, найденных в Qt.



Длинный-предлинный switch(). И, естественно, имеется забытый оператор "break". Анализатор выявил эту ошибку за счет того, что переменной "ret" дважды подряд присваивается разное значение.

Пожалуй, намного лучше, было бы завести какой-то std::map<PixelMetric, int> и явно табличкой задать соответствие между метрикой и числами. Можно придумать и другие табличные варианты реализации функции.

Еще один пример:



В коде два раза сравниваем переменную 'ver' с константой WV_2000. Хороший пример, где самое место табличному методу. Например, такой метод мог бы выглядеть так:





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

Рекомендация. Не ленитесь написать функцию с использованием табличных методов. Да, это займет немного времени, но зато это окупится потом. Вам будет проще и быстрее добавлять новые условия, а вероятность ошибки при этом будет намного ниже.

3. Разное интересное

Поскольку Qt большая библиотека, то, несмотря на высокое качество, в ней можно встретить разнообразнейшие ошибки. Действует закон больших чисел. Размер *.cpp, *.h и аналогичных файлов проекта Qt составляет около 250 мегабайт. Как не маловероятна ошибка, в большом коде её встретить вполне реально. На основании других ошибок, которые я обнаружил в Qt, составить какие-то рекомендации сложно. Просто опишу некоторые ошибки, которые мне понравились.



Случайно используется оператор && вместо &. Обратите внимание, как полезно иметь в коде комментарии. Сразу становится понятно, что это действительно ошибка и как на самом деле должны обрабатываться биты. 

Следующий пример будет на тему длинных выражений:



Видите ошибку? Вот-вот, с ходу и не заметишь. Хорошо, подскажу. Беда вот здесь: "orig->y1 - orig->y1". Ещё меня третье умножение смущает, но возможно так и надо. 

Да, еще вопрос. А у вас ведь тоже в программах вот такие вот блоки вычислений есть? Не пора ли попробовать статический анализатор кода PVS-Studio? Так, порекламировал. Пойдем дальше.

Использование неинициализированных переменных. Их можно найти в любом большом приложении:



Здесь опять нужно дать подсказу, чтобы долго не мучить ваши глаза. Смотреть надо на инициализацию переменной "transition->m_hasGetterSetterProperties".



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



И только потом приходило осознание, зачем нужны такие на первый взгляд странные функции, как strcmp(). К сожалению, язык Си++ настолько суров, что сделать такую ошибку можно и через много лет, будучи профессиональным разработчиком с опытом:



Что бы еще показать. Вот, например, неправильно написанный обмен значений переменных.



Это пример того, что можно ошибиться даже в очень простом коде. Так, пока ещё не было примеров на тему выхода за границы массива.  Сейчас будет:



Условие "--size >= 0" всегда истинно, так как переменная size имеет беззнаковый тип. Если будут сравниваться одинаковые последовательности, то произойдет выход за границы массивов.

Можно продолжать и дальше. Надеюсь, вы как программисты понимаете, что ошибки в проекте такого объема описывать в одной статье нет никакой возможности. Поэтому последнее, на закуску:



Должно было быть что-то в духе "if (hr == S_OK)" или "if (SUCCEEDED(hr))". Макрос S_OK есть не что иное, как 0. Поэтому бяка с неправильным подсчетом количества ссылок здесь неизбежна.

Вместо заключения

Спасибо за внимание. Используйте статический анализ кода, и вы сможете сэкономить массу времени для более полезных дел, чем отладка и сопровождение кода.

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

Библиографический список

1. Андрей Карпов. Как уменьшить вероятность ошибки на этапе написания кода. Заметка N1. http://www.viva64.com/ru/a/0070/
2. Андрей Карпов. Как уменьшить вероятность ошибки на этапе написания кода. Заметка N2. http://www.viva64.com/ru/a/0072/
3. Макконнелл С. Совершенный код. Мастер-класс / Пер. с англ. - М. : Издательско-торговый дом "Русская Редакция" ; СПб. :   Питер, 2005. - 896 стр. : ил.

Thunderbolt, ты мог бы проанализировать ветку 4.8 репозитория Qt и выложить результат? Может кто возьмется за исправление.
А то далеко не все имеют возможность это сделать, так как для этой проги необходима платная студия. Я уж не говорю о цене самой проги...
наша контора купила бы ее, но мы в принципе не используем visual studio (так как разработка ведется под Linux), а платить за ненужный дополнительный софт мы готовы...

Это сообщение отредактировал(а) bsa - 13.7.2011, 10:16

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

 Код 
:
    


говорит лишь о том, что не надо делать то, что уже было сделано грамотно за вас. То есть надо было использовать стандартный алгоритм std::swap. Тогда бы такой ошибки не было бы.

То же самое касается и этого вашего примера:



Вместо этого кода следовало бы использовать стандартный алгоритм std::equal.
В этом и заключается смысл стандартных алгоритмов, что они уже грамотно написаны, протестированы, а потому гарантируют результат выполнения именно того, что вы от них ждете.

В Qt стараются не использовать STL. Видимо, это атавизм со старых времен, когда с STL были проблемы. Возможно, на некоторых платформах проблемы с ним до сих пор.

Та часть STL, которая касается алгоритмов, довольно небольшая по объему, и ее можно совершенно безболезненно заменить своей реализацией алгоритмов, таккак в стандарте вообще дангы лишь прототипы алгоритмов и сказаны общие требования к ним.

Я могу привести часто встречающуюся ошибку в реализации алгоритмов, которую можно найти даже в солидных книгах по STL. Так, например, алгоритм std::find иногда записывается следующим образом



То есть здесь вместо оператора == используется оператор !=. Тем самым сразу же отсекаются все типы данных, в которых определен лишь оператор ==. Что очевидно является некорректным.

Кроме того есть и другие ошибки, когда, например, вместо проверки условия predicate( *first, *OtherIterator ) проверяется условие с перестановкой членов predicate( *OtherIterator, *first ). Это обычно происходит тогда, когда в одном алгоритме используют для удобства другой алгоритм. 
Но алгоритмов так мало, что совершенно не ясно, почему их нельзя было заменить собственной реализацией. Это намного проще, чем каждый раз с ошибками заново писать код, который выполняет одну и ту же задачу.

Добавлено через 5 минут и 50 секунд
Кстати сказать по поводу операторов == и != у меня был забавыный случай на другом форуме. Точно такой же базграмотный самоуверенный программист, какие встречаются на этом форуме, например, те, которые говорят, что значения указателей могут быть не выровнены, как-то в ответ на мое действие, что я поменял условие

( x == y ) на условие !( x == y ), долго смеялся надо мной, заявляя, что я не знаю С++, так как любой другой бы на моем месте написал бы вместо !( x == y ) выражение ( x != y ). Я уж не стал объяснять этому безграмотному "программисту", что в С++ в общем случае !( x == y )  и  ( x != y ). - это не одно и то же.

и где Вы такой код нашли, мож в каком доисторическом компиляторе,  когда еще со стандартом толком и не определились.. 


как раз Ваш случай подпадает под указанные в стандарте требования : 
Requires: Type T is EqualityComparable (20.1.1).