Отправлено: 08.10.13 21:27. Заголовок: Пример того, как не следует писать программы.

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

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

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

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

Но вернемся к задаче и соответствующему коду, упомянутому мною в начале темы.
Задание выглядит довольно просто: нужно в объекте типа std::string сдвинуть все элементы влево на несколько позиций, заменяя высвобождаемые позиции символами пробела. Вот код этого, так называемого "гуру":

  
 #include <iostream>  
 #include <string>  
  
 using namespace std;  
  
 void shift_left( string& s, int n = 1 )  
 { s = s.erase( 0, n ) + string( n, ' ' ); }  
   
   
 auto main() -> int  
  {  
      string s = "this and that";  
   
      cout << "at start:      '" << s << "'\n";  
   
      shift_left( s, 3 );  
   
      cout << "after shift 3: '" << s << "'\n";  
   
  
      for( int i = 1; i <= 12; ++i )  
      {  
          shift_left( s );  
   
          cout << "after shift 1: '" << s << "'\n";  
      }  
   
 }  
 

Сначала обратим внимание на объявление main. Как говорил один киногерой в одном американском фильме: "Я люблю умных людей, но не люблю умников."

Каждый, кто встретит такое объявление main будет вынужден потратить свое время, чтобы понять, а почему автор кода выбрал именно такое объявление, а не привычное int main(). Какие на то были причины?.

Очевидно, что набирать текст auto main() -> int значительно обременительнее, чем привычное int main(). Как известно, чем больше приходится набирать текста, тем больше вероятность допустить опечатку. В этом примере вместо одной лексической единицы int, автор набирает целых три лексических единицы: auto, -> и int. Зачем? Делает ли это текст программы более читаемым и понятным? Очевидно, что нет, и реакция читающего текст программы будет прямо противоположная: он вынужден будет потратить время, чтобы понять замысел автора. И когда обнаружит, что никакой разумной причины так писать объявление main не было, то просто будет сильно раздражен этим.

Теперь вернемся к сути задачи и ее главной функции shift_left.

Во-первых, эта функция имеет совершенно несогласованный интерфейс с функциями, объявленными в классе std::string (или, если быть более точным, то в std::basic_string). Рассмотрим простой пример. Допустим в программе есть следующее предложение:

 
 std::cout << s.erase( 0, n ) << std::endl; 
 

И спустя некоторое время было решено, что вместо вызова функции s.erase( 0, n ) следует вызывать shift_left( s, n ). .Это обычная практика в программировании, что часто в коде на протяжении времени жизни программы и ее поддержки приходится заменять вызов одной функции на вызов другой функции. В этом случае мы не можем просто написать, заменив в предложении одну функцию на другую:

 
 std::cout << shift_left( s, n ) << std::endl; 
 

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

Этой проблемы не существовало бы, если бы функция была изначально объявлена следующим образом:

 
 std::string & shift_left( std::string &s, int n = 1 ); 
 

Но это лишь самое меньшее зло из того, с чем нам предстоит столкнуться в этой функции.

Обратите внимание на объявление второго параметра функции: int n. Очевидно, что в качестве аргумента функции вероятнее всего будут передаваться значения аргумента, имеющее тип std::string::size_type, как, например, значение, возвращаемое функцией членом класса std::basic_string size или find. Но это может привести к неопределенному поведению функции при преобразовании значения неотрицательного типа к значению отрицательного типа, который не способен разместить все значения из допустимого диапазона значений неотрицательного типа.

Поэтому правильно было бы объявить функцию следующим образом:

 
 std::string & shift_left( std::string &s, std::string::size_type n = 1 ); 
 

Это объявление полностью согласовано с тем, как объявляются подобные функции в классе std::basic_string.

Теперь рассмотрим внутреннюю реализацию функцию. Она просто отвратительна. Почему? Потому что она по крайней мере дважды выделяет и удаляет память тогда, когда это совершенно не требуется для выполнения задачи. То есть функция совершенно не эффективна, приводит к фрагментации памяти и может просто аварийно завершиться, так как необходимая память, когда используются большие строки, может быть не выделена.

Первый раз память выделяется когда явным образом вызывается конструктор:

 
 string( n, ' ' ); 
 

Второй раз память выделяется, когда в работу включается функция operator +. Согласно стандарту C++ ее действие в данном случае эквивалентно вызову std::move(rhs.insert(0, lhs)), то есть функция insert в свою очередь запросит снова выделение памяти. А затем память исходной строки s должна будет быть заменена на вновь выделенную память.

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

Вот как будет выглядеть правильная реализация функции:

 
 std::string & shift_left( std::string &s, std::string::size_type n = 1 ) 
 { 
     return ( s.erase( 0, n ).append( n, ' ' ) ); 
 } 
 

Но и эту реализацию можно улучшить. Обычно для функций класса std::basic_string когда указывается число элементов объекта, то допускается задавать в качестве аргумента для такого параметра значение std::string::npos.

Вот как, например, выглядит одно из объявлений функции erase:

 
 basic_string& erase(size_type pos = 0, size_type n = npos); 
 

Поэтому было бы разумным не исключать возможности того, что пользователь функции shift_left может передать второму параметру в качестве аргумента значение std::string::npos.

В этом случае результат работы функции будет неверным, так как будет сделана попытка вызова функции append( std::string::npos, ' ' ). Поэтому более корректная реализация функции будет выглядеть следующим образом:

 
 std::string & shift_left( std::string &s, std::string::size_type n = 1 ) 
 { 
     if ( n == std::string::npos ) n = s.size(); 
     return ( s.erase( 0, n ).append( n, ' ' ) ); 
 } 
 

Из этой истории можно сделать следующие выводы.

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

2. Правильно выбирайте типы для параметров функций, которые должны иметь дело с объектами определенного пользовательского типа. Обычно все стандартные классы специально внутри себя объявляют все типы своих подобъектов. Старайтесь также писать собственные классы таким образом, чтобы они предоставляли информацию о типах, с которыми они оперируют.

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

4. Если используете в функции объекты, которые размещаются в куче, то старайтесь избегать излишнего выделения памяти и ее удаления для таких объектов. Это не только делает ваши программы более эффективными, но и устраняет источник потенциальных ошибок и исключительных ситуаций.