Простая функция C++ — действительно ли этот код “хорош”?

хм. Я думаю

CString strColHeader;
strColHeader.Replace(",", "\\,") 

сделал бы точно также.

Мне не нравится код, я склонен повреждаться от цикла с условием продолжения вместо того, чтобы иметь ненужный bool, 'продолжают' флаг. Это идет дважды, когда он, возможно, использовал while (occurenceInd != 0) как его контрольная переменная цикла вместо булевской переменной.

Постепенное увеличение счетчика также полагается "+2", который сразу не кажется понятным (не на быстрый взгляд), и наконец (и самое главное) он, кажется, не делает комментарии.

Существует off-one ошибка, находящаяся там прямо посередине: Смотрите на то, что происходит, если первый символ является запятой: ", abc, определение, топленое масло": я предполагаю, что желаемый вывод был бы "\, abc \, определение \, топленое масло", но вместо этого Вы возвращаете исходную строку:

int occurenceInd = originalString.Find(charFind, currentInd);

OccurrenceInd возвращается 0, так как он нашел charFind в первом символе.

if(occurenceInd>0) 

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

if(occurrenceInd >= 0)

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

CString insert_escape ( const CString &originalString, char charFind, char charInsert ) {
    std::string escaped;
    // Reserve enough space for each character to be escaped
    escaped.reserve(originalString.GetLength() * 2); 
    for (int iOriginal = 0; iOriginal < originalString.GetLength(); ++iOriginal) {
        if (originalString[iOriginal] == charFind)
            escaped += charInsert;
        escaped += originalString[iOriginal];
    }
    return CString(escaped.c_str());
}

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

Я волновался бы больше о стилистических вещах, поскольку они предлагают кого-то, кто не доволен C++. Использованием bool continueLoop является просто плохой C++. Это представляет одну треть кода функции, который мог быть устранен при помощи простого, если... повреждают конструкцию, делая код легче следовать в процессе.

Кроме того, имя переменной "originalString" является очень вводящим в заблуждение. Поскольку они передают его значением, это не исходная строка, это - копия его! Затем они изменяют строку так или иначе, таким образом, это больше не тот же объект или та же строка текста как оригинал. Это удваивается, лежат, предлагает запутанные образы мышления.

CString имеет Замену () метод... (который был моей 1-й реакцией),

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

Моя реакция пищеварительного тракта: WTF. Первоначально тем, как код форматирован (существует много вещей, мне не нравится приблизительно форматирование), и затем исследованием того, что на самом деле делает код.

Существует серьезная проблема с пониманием этого разработчика копирования объекта в C++. Примером является WTF сам по себе (если разработчик функции действительно использовал его собственную функцию как это):

// Example call
strColHeader = insert_escape(strColHeader, ',', '\\'); //Get rid of the commas and make it an escape character

CString insert_escape ( CString originalString, char charFind, char charInsert )

1. Передайте копию strColHeader как originalString (заметьте, что существует нет &)
2. Функция изменила эту (прекрасную) копию
3. Функция возвращает копию копии, которая в свою очередь заменяет оригинал strColHeader. Компилятор, вероятно, оптимизирует, это к единственной копии, но тем не менее, раздавая объектные копии как это не работает на C++. Нужно знать о ссылках.

Более закаленный разработчик разработал бы эту функцию как:

void insert_escape(CString &originalString, char charFind, char charInsert)

или:

CString insert_escape(const CString &originalString, char charFind, char charInsert)

(И вероятно назвал бы параметры немного по-другому),

И поскольку многие указали, нормальная вещь, которую, возможно, сделал разработчик, состояла в том, чтобы проверить документацию API, чтобы видеть если CString уже имел a Replace метод...

Если бы Вы желаете оценку уровня квалификации C++ этого разработчика, я сказал бы, что это демонстрирует более низкий уровень промежуточного звена.

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

Я не предоставлю альтернативный код, как он только добавил бы к коду, уже предоставленному.

Но, мое инстинктивное чувство состоит в том, что существует что-то не так с кодом.

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

• копия: параметры передаются как копия вместо ссылки константы. Это - большое НЕТ НЕ в C++ при рассмотрении объектов.
• ошибка, которую я предполагаю, существует ошибка в "если (occurenceInd> 0)" часть. Путем чтения документа CSTRING о MSDN, CString:: Найдите, что метод возвращается-1, а не 0, когда находка перестала работать. Этот код говорит мне, если бы запятая была первым символом, то этого не оставили бы, который является, вероятно, не точкой функции
• ненужная переменная: "continueLoop" не нужен. При замене кода "continueLoop = ложь" "продолжается" и строка, "в то время как (continueLoop)", "в то время как (верный)" достаточно. Обратите внимание, что, продолжая это обоснование позволяют кодеру изменить внутренние функции (замена... в то время как простым в то время как)
• изменение типа возврата: Вероятно, выбирая в деталях, но я предложил бы альтернативную функцию, которая, вместо того, чтобы возвратить строку результата, примет, как ссылка (один меньше копии по возврату), исходная встраиваемая функция и называющий альтернативу.
• добавление константы, когда это возможно, снова, выбор в детали: два "символьных" параметра должны быть константой, если только постараться не изменять их случайно.
• возможный несколько перераспределение функция полагается на потенциал несколько перераспределений данных CString. Решение Josh's использования станд.:: резерв строки является хорошим.
• использование полностью CString API: Но в отличие от Josh, потому что Вы, кажется, используете CString, я избежал бы станд.:: строка и использование CString:: GetBufferSetLength и CString:: ReleaseBuffer, которые позволяют мне иметь тот же код с одним меньшим количеством объектного выделения.
• Таинственный метод Вставки? это - я, или нет никакого CString:: Вставить??? (см. http://msdn.microsoft.com/en-us/library/aa252634 (По сравнению с 60) .aspx). На самом деле мне даже не удалось найти CString в том же MSDN для Visual C++ 2008 и 2005... Это могло быть то, потому что я должен действительно заснуть, но тем не менее, я предполагаю, что это стоит исследовать

Этот консультант заплатил строкой кода? Несколько человек указали что CString класс уже обеспечивает эту функциональность, поэтому даже если Вы не программист, Вы знаете:

• Функция является ненужной. Это добавляет к сложности, размеру, и возможно время выполнения программы.
• CString функционируйте, вероятно, работает и вероятно эффективен; этот может или не может быть.
• CString функция документируется и поэтому приемлема.
• Консультант любой незнаком со стандартом CString функционируйте или думал, что он мог добиться большего успеха путем записи нового.
  • Можно было бы прийти к заключению, что консультант незнаком с другими стандартными функциями и лучшими практиками.
  • Желание написать новый код для основной характеристики, не полагая, что стандартная версия может существовать, является принятой плохой практикой.

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

Доверяйте своим инстинктам.

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

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

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

Я объявляю:

пустите в ход fMyNumber = 0.00;

И затем я использую это в своем целом приложении. Но затем, позже, я изменяю это на двойное, потому что я понимаю, что мне нужно больше точности. Теперь я имею:

удвойте fMyNumber = 0.00;

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

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

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

Это находится в Visual Studio C++ 6.0.

Реакция пищеварительного тракта: отрыжка. Серьезно! Компилятор C++, поставленный с VC ++ 6, как известно, является багги и обычно работающий очень плохо, и этому 10 лет.

@Downvoters: рассмотрите это! Я имею в виду это во всей серьезности. VC6 просто сравнительно непроизводителен и не должен больше использоваться! Тем более, что Microsoft прекратила свою поддержку программного обеспечения. Существуют случаи, где этого нельзя избежать, но они редки. В большинстве случаев обновление кодовой базы экономит деньги. VC ++ 6 просто не позволяет использовать потенциал C++, который делает объективно нижний инструмент.

Я всегда волную, когда я вижу.. цикл с условием продолжения; IMO их всегда более трудно понять.