Monthly Archives: Октябрь 2014

Частая ошибка при использовании std::erase в связке со std::remove_if в C++

Для того, что бы удалить эелемент из контейнера типа vector, string, list или deque мы пользуемся методом erase, которому параметром передаём итератор на ненужный элемент.

В какое то время, происходит оптимизация кода или же изначально закладывается возможность поиска ненужного элемента с помощью алгоритма STL remove_if.

Далее код обрастает использованием boost::bind для удобства и прочими артефактами.

И тут в какой то момент становится нужно удалять не один элемент, а несколько. Неважно, в новом коде, или же расширяя возможности старого. И здесь многие новички в C++ делают ошибку, не зная и не посмотрев в документацию метода erase. Они копируют по примеру код, написанный ранее и переделывают условие поиска элементов для remove_if. Но они забывают, что если методу erase передаётся один аргумент, то этот метод удаляет именно тот элемент, итератор на который был передан аргументом.

Для удаления нескольких элементов существует перегруженный метод erase, который принимает два аргумента — на начало и конец итеративной последовательности, подлежащей удалению. В большинстве случаев, когда используется алгоритм remove_if, он реорганизует последовательность так, что все элементы не подлежащие удалению переставляются в начало контейнера. Например, у нас есть vector , содержащий следующие значения.
1 2 3 1 2 3 1 2 3

Если вызвать
bool IsTwo (int i) { return (i == 2); }

tstart = remove_if(v.begin(), v.end(), isTwo)
//удалить элемент со значением 2 из контейнера v

получится
1 3 1 3 1 3 ? ? ?
где ? — некий мусор. Итератор tstart, который возвратит remove_if, будет указывать на первый мусорный элемент, в этом случае на третий с конца. Мусорные элементы могут иметь те же значения, что и до вызова remove_if, то есть 1 2 3, а могут иметь и любые другие, на это полагаться не стоит. Размер контейнера остается неизменным.

Таким образом, если мы запишем полный кусок кода, с которым работает новичок, то получится следующее:
v.erase(remove_if(v.begin(), v.end(), isTwo));

Соответственно erase удаляет первый мусорный элемент и изменяет размер контейнера на единицу. В результате содержимое контейнера начинает выглядеть так:
1 3 1 3 1 3 ? ?
что является неправильным и соответственно приводит к непредсказуемым последствиям при дальнейшей работе с этим контейнером.

Как же не допускать такого? Когда, разработчик не уверен в своих знаниях, то хорошим тоном будет проверить их по документации и узнать, что если вызвать erase таким образом:
v.erase(remove_if(v.begin(), v.end(), isTwo), v.end());
то все мусорные элементы будут удалены.

Если же разработчик уверен (иногда случается ложная уверенность, это нормально), то тестирование всё равно никогда не бывает лишним. Причём не ленимся, и пишем пограничные варианты тестов в том числе. В нашем случае, мы должны проверить на вскидку не только правильность удаления одного элемента, но также нескольких, и ни одного, и конечно же случай, когда контейнер вообще пуст.

Опасность использования значений по умолчанию в обьявлениях

Недавно работал на одним большим проектом написанным на C++, и во время отладки встретился с довольно опасной вещью. Посмотрим на примерах. В начале был некий метод с парой параметров один из которых задаётся по умолчанию в обьявлении:

Class1::SetEditBox(CString const& value, bool isUppercase = true);

И гдето в коде этот метод по разному вызывался:

obj1.SetEditBox(_T("Volume"), false);
 ...
 obj2.SetEditBox(_T("SerialNo"));

Прошло время и обьявление метода немного дополнили:

Class1::SetEditBox(CString const& value, bool isEditable = false, bool isUppercase = true);

В новых местах использование, всё было хорошо, да вот старый код толком никто так и не посмотрел, таким образом после вызова

obj1.SetEditBox(_T("Volume"), false);

isUppercase становился true. Однако, так как обьём обычно задаётся в числах, то без толкого тестирования различных вариантов, такой случай прошёл незамеченным.
Время шло, система росла и становилась сложнее. С каждой сменой команды разработчиков её брались переписывать и оптимизировать и в какой то момент бросали. Метод задействовался в других классах:

Class2::SetListEdit(CString const& value, bool isEditable = false, bool isUppercase = true)
{
    // ...
    obj4.SetEditBox(value, isEditable, isUppercase);
    // ...
}

И снова толком никто не просмотрел все места, где нужно было внести изменения.
Время шло и обьявление метода снова пополнилось новым аргументом:

Class1::SetEditBox(CString const& value, bool isNumeric = true, bool isEditable = false, bool isUppercase = true);

На этот раз уже гораздо больше кода следовало просмотреть и изменить, но как то изменение забылось и прошло незамеченным. И далее код ещё более расширился:

Class3::AddListEdit(CString const& value, bool isUppercase = true)
{
    Class2 obj5;
    obj5.SetListEdit(value, isUppercase);
}

В результате мы имеем ошибку, которую я назвал «Shifting Boolean».
Таким образом вызов Class3::AddListEdit() и передача аргументом только строки, приводит к тому, что значениее isUppercase становится значением для isEditable в Class2::SetListEdit() и затем становится значением для isNumeric в Class1::SetEditBox().

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

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

Рефакторинг этого кода занял у меня 6 часов. Зато я избавился от переменных типа bool, заменил их единственным аргументом enum, так как в данном случае комбинации изначальных аргументов были между собой связаны логически. И теперь следующий разработчик сможет спокойно воспользоваться единственным нужным значением, вместо того, что бы смотреть где что установлено по умолчанию и что нужно прописать руками. Да и перебрать один аргумент через switch() гораздо легче, чем продираться через лес if()-ов для проверки всех возможных комбинаций для bool.

Округление дробей в стиле Omniva

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

Округление в стиле Omniva