Какие вещи мгновенно звонят в тревогу при взгляде на код? [закрыто]

98

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

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

Я начну с простого:

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

FinnNk
источник
61
Иногда я встречаю людей, которые комментируют код, регистрируются и говорят: «Возможно, он понадобится мне снова в будущем - если я удалю его сейчас, я потеряю его». Я должен противостоять "Э-э ... но для этого нужен контроль источников".
talonx
6
Иногда, особенно при оптимизации, удобно оставлять старый код в качестве комментария, чтобы вы знали, что заменяет неясный оптимизированный код. Подумайте о том, чтобы оставить трехстрочный своп с temp на месте, заменив его однострочным твиплинг-свопом. (Хотя я не вижу необходимости использовать однострочный своп - НИКОГДА, если только размер программы не имеет решающего значения.)
Крис Кадмор
4
Я поддерживаю / очищаю код, написанный одним из наших инженеров, который написал несколько полезных вещей, но признает, что он не программист. Когда я собираю информацию, я закомментирую его старый код, а затем мы рассмотрим изменения и покажу ему, как я заменил его чем-то меньшим / более эффективным / более простым для понимания. После этого я удаляю эти блоки, затем проверяю их. Наличие старого кода имеет свои преимущества, потому что он видит, как все можно сделать проще, и я могу вспомнить, почему я что-то изменил, когда мы разговариваем.
Жестянщик
8
Я оставляю материал, который «может быть использован», для 1 коммита, а затем, если что-то не выходит из строя или необходимость не обнаруживается, он удаляется при следующем коммите.
Пол Натан
24
Хм. printf("%c", 7)как правило, звонит мне в тревогу. ;)

Ответы:

128
/* Fuck this error */

Обычно находящийся внутри бессмысленного try..catchблока, он привлекает мое внимание. Примерно так же, как и /* Not sure what this does, but removing it breaks the build */.

Еще пара вещей:

  • Несколько вложенных сложных ifоператоров
  • Блоки try-catch, которые используются для регулярного определения логического потока
  • Функции с родовыми названиями process, data, change, rework,modify
  • Шесть или семь разных стилей крепления в 100 линий

Тот, который я только что нашел:

/* Stupid database */
$conn = null;
while(!$conn) {
    $conn = mysql_connect("localhost", "root", "[pass removed]");
}
/* Finally! */
echo("Connected successfully.");

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

Josh K
источник
19
Если бы я только мог проголосовать за это 6 раз! Все хорошие примеры. Мне также не нравятся надменные / смешные комментарии (особенно если они содержат ругательства) - это может быть немного забавно, когда вы впервые читаете их, но очень быстро стареете (и, следовательно, отвлекаете).
FinnNk
5
Мне нравится ваш пример, хотя я бы сказал, что в определенных контекстах множественные вложенные операторы неизбежны. При большом количестве бизнес-логики код может немного сбивать с толку, но если сам бизнес с самого начала сбивает с толку, упростить код будет не так точно, как моделировать процесс. Как сказал Эйнштейн: «Все должно быть максимально просто и ни на что не проще».
Морган Херлокер
2
@Prof Plum - Какой пример вы можете привести? Обычно альтернативой нескольким вложенным if является разбить его на (много) методов. Младшие разработчики склонны избегать этого, как будто это менее желательно, чем если бы; но обычно, когда нажимают, они говорят «если это сделать в несколько строк». Нужно, чтобы кто-то уверенный в ООП вмешался и напомнил им, что меньше строк! = Лучший код.
STW
2
@STW Это хороший момент, но я бы сказал, что это зависит от глубины вложенности. Я бы, конечно, согласился, что что-то более трех гнезд в глубине часто нуждается в рефакторинге, потому что это будет довольно волосатым. Тем не менее, цитирование страховых полисов является хорошим примером, когда множественное вложение может действительно достаточно хорошо моделировать реальный мир. За исключением определенных ставок / премий, руководство будет буквально читать что-то вроде «если это собственность, и если она имеет минимальную ставку ниже 5,6 и если она находится в Северной Каролине, и если у нее есть лодка в помещении, то сделайте это и такие «. со многими другими вариантами.
Морган Херлокер
4
@josh, если бы «они» были коллегами, я бы подумал, почему ты не сказал «мы» ...
104

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

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

Бен Хьюз
источник
1
+1: я видел некоторый код от коллеги, который рекламировал себя как эксперта по Linux, который написал простое зацикливающееся приложение как одну длинную функцию, все это снова и снова в main (). Хлоп.
KFro
4
@KFro, это развертывание цикла. Вот что делают компиляторы все время :) ОЧЕНЬ эффективно!
3
@ Torbjorn: Иногда вам нужно немного помочь компилятору; в конце концов, вы умный человек, а он просто тупой компьютер.
yatima2975
3
Еще одна причина, которую я видел: консультанту заплатили, чтобы внедрить эту функцию как можно быстрее (поэтому, конечно, тоже отсутствуют тесты и документация). Копировать / вставить быстрее, чем думать о том, как все сделать правильно.
LennyProgrammers
4
Избегание дублирования кода может быть навязчивой идеей. В таких языках, как C ++, это не всегда так просто, как следует выделять различные части, но при этом иметь надежный и эффективный код. Иногда немного вырезать и вставить является более практичным вариантом. Кроме того, может применяться принцип оптимизации - вырезать и вставить может дать вам быстрое и простое решение, которое вы можете изменить в будущем, если это необходимо. Вы можете экономить головную боль обслуживания для последующего использования , но вы знаете наверняка , что вы избежать задержек прямо сейчас.
Steve314
74

Код, который пытается показать, насколько умен программист, несмотря на то, что он не добавляет реальной ценности:

x ^= y ^= x ^= y;
Рей Миясака
источник
12
Ого, это ооочень намного читабельнее, чемswap(x, y);
JBRWilkinson
8
если x и y являются указателями, и это назначение происходит в течение разумного промежутка времени, это разумно нарушает консервативные сборщики мусора, такие как Boehm GC.
SingleNegationElimination
12
Кстати, этот код не определен в C и C ++ из-за множественных изменений без промежуточной точки последовательности.
fredoverflow
9
Глядя на это, единственное, что приходило на ум, были смайлики:^_^
Darien
62
  • 20 000 (преувеличение) линейных функций. Любая функция, которая занимает более пары экранов, нуждается в перефакторинге.

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

  • неописательные, нетривиальные переменные или слишком много тривиальных неописательных переменных. Это делает вывод о том, что на самом деле происходит загадка.

Доминик Макдоннелл
источник
9
Я стараюсь ограничить функции до 1 экрана, когда это возможно.
Мэтт ДиТролио
20
1 экран - это даже натяжка. Я начинаю чувствовать себя грязным после примерно 10 строк.
Брайан Роу
54
Хорошо, я собираюсь высказать то, что может быть непопулярным мнением. Я говорю, что пахнуть кодом - писать функции, которые являются атомарными, сверху вниз процессами, которые разбиты на отдельные функции, потому что разработчик цепляется за некоторые «функции должны быть короткими». Функции должны быть разбиты по функциональным линиям, а не только потому, что они должны иметь какой-то мифический «правильный размер». Вот почему они называются ФУНКЦИЯМИ.
Дэн Рэй
7
@ Дан, функции не должны быть короткими ради краткости, но есть только так много информации, которую вы можете держать в своей голове за один раз. Возможно, у меня маленький мозг, так как для меня этот предел - пара экранов :). Разделение функций на несколько функций, когда они начинают тестировать эту границу, необходимо, чтобы избежать ошибок. С одной стороны, он обеспечивает инкапсуляцию, так что вы можете думать на более высоком уровне, а с другой - скрывает происходящее, что затрудняет понимание того, как работает эта функция. Я думаю, что разбивка функций должна быть сделана, чтобы улучшить читаемость, а не соответствовать «идеальной длине».
Доминик Макдоннелл
6
@ Доминик, @ Петер, я думаю, что мы трое на самом деле говорим одно и то же. Когда есть веская причина для преобразования кода в меньшую функцию, я за это. То, что я отвергаю, это краткость ради краткости в функциональном дизайне. Вы знаете, стек вызовов в три раза длиннее, чем нужно, но по крайней мере эти функции короткие. Я бы предпочел отладить сложную функцию, которая делает одно хорошо и четко, чем преследовать путь выполнения через дюжину связанных функций, каждая из которых вызывается только из предыдущей.
Дэн Рэй
61
{ Let it Leak, people have good enough computers to cope these days }

Что еще хуже, это из коммерческой библиотеки!

Reallyethical
источник
32
Это не звонит в тревогу. Это практически пинает вас между ног.
Стивен Эверс
15
Дочь является наркоманом мета. Кого волнует, по крайней мере, она не станет ожирением. :: вздох ::
Эван Плайс
17
Когда я попадаю в трудные времена, ко мне приходит мама Бунту. Говоря слова мудрости, пусть течет. ДАВАЙТЕ, ЧТОБЫ УТЕЧАТЬ. ДАВАЙТЕ, УТЕЧУЙ, ДАВАЙТЕ, УТЕЧИВАЙ. Если он просто один раз протекает, значит, это не лаааааак. (если вы прочитали это далеко, +1). Мне действительно нужно рассмотреть без кофеина.
Тим Пост
13
«Когда приближается крайний срок, УТЕЧКИ - это все, что я вижу, где-то кто-то шепчет:« Напишите на C ...... ээээээ !!! »
chiurox
9
Когда-то были две ОС. Один просочился и разбился, если он работал более 49 дней, другой был идеален и работал бы вечно. Один был запущен в 1995 году для огромной фанатской ярмарки и использовался миллионами людей - другой никогда не доставлялся, потому что они все еще проверяют, что он не содержит ошибок. Существует разница между философией и разработкой.
Мартин Беккет
53

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

//Copy the value of y to temp.
temp = y;
//Copy the value of x to y.
y = x;
//Copy the value of temp to x.
x = temp;

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

//Set the age of the user to 13.
a = 13;
Rei Miyasaka
источник
15
Это называется COBOL :-)
Gaius
26
Второй случай не самый худший. Худшее: / * Установите возраст пользователя 13 * / a = 18;
PhiLho
4
@PhiLho - нет, еще хуже, когда отсутствует значение /from */, поэтому весь код до конца следующего */игнорируется. К счастью, подсветка синтаксиса делает такие вещи редкими в наши дни.
Steve314
3
Что еще хуже, aдля user_age? В самом деле?
Glasnt
2
Я имел обыкновение вести стандартный код документа у предыдущего работодателя, один раздел которого был соответствующим комментарием. Мой любимый пример был из MSDN:i = i + 1; //increment i
Michael Itzoe
42

Код, который выдает предупреждения при компиляции.

Рей Миясака
источник
1
Есть кандидат для добавления опции компилятора «Все предупреждения как ошибки» в makefile / project.
JBRWilkinson
3
Я думаю, это может быть полезно, если вы работаете в проекте с несколькими людьми, которым вы просто не доверяете - хотя, если бы я присоединился к проекту, в котором установлена ​​эта опция, это само по себе заставило бы меня беспокоиться о том, насколько способна другая программисты есть.
Рей Миясака
1
Я не согласен с этим. Некоторые предупреждения компилятора (например, сравнение между подписанным и беззнаковым, когда вы знаете, что оба значения являются беззнаковыми, даже если типы различаются) предпочтительнее засорения кода ненужными приведениями. Если я уменьшу код, используя переносимое целое число со знаком, которое функция изменяет, только если целое число имеет значение без знака, я собираюсь это сделать.
Тим Пост
13
Я предпочел бы загромождать мой код почти излишним, (unsigned int)чем загромождать мои списки предупреждений / ошибок мягкими предупреждениями. Я бы не хотел, чтобы список предупреждений стал слепым пятном. Это также намного больше ЛАВАШ объяснить другим людям , почему вы игнорируете предупреждение , чем это объяснить , почему вы делаете бросок естественно intsдля unsigned ints.
Рей Миясака
Иногда вам приходится работать с API, который издает ошибки независимо от того, что вы делаете. Классические примеры - это когда API определяется с точки зрения вещей, которые разбиты по проекту (некоторые старые константы ioctl () были такими, а иногда разработчики ОС настаивают на использовании неверного типа в своих заголовках) или когда они что-то осуждали без оставив хорошую замену (спасибо, Apple).
Donal Fellows
36

Функции с числами в имени вместо описательных имен , например:

void doSomething()
{
}

void doSomething2()
{
}

Пожалуйста, сделайте так, чтобы имена функций что-то значили! Если doSomething и doSomething2 делают схожие вещи, используйте имена функций, которые различают различия. Если doSomething2 является прорывом функциональности от doSomething, назовите его для его функциональности.

Wonko the Sane
источник
Точно так же @Parm for SQL
Дэйв
2
+1 - Войти mshtml- это разбивает мне глаза :(
Кайл Розендо
3
Исключением является код GUI. Например, если у формы обычной почты было два адреса; address1 и address2 более разумны, чем address и alternateAddress. Ложные метки, которые являются только статическими, также являются разумным исключением.
Эван Плейс,
@Evan - достаточно справедливо, хотя я больше отличался функциональностью.
Вонко вменяемый
1
+1 - я даже видел, как это используется как метод контроля псевдо-версии.
EZ Hart
36

Волшебные числа или Волшебные струны.

   if (accountBalance>200) { sendInvoice(...); }

   salesPrice *= 0.9;   //apply discount    

   if (invoiceStatus=="Overdue") { reportToCreditAgency(); }
JohnFx
источник
4
Не так уж и плохо со вторыми двумя, по крайней мере, скидка объясняется, и «Просрочено» интуитивно понятно. С 200другой стороны ...
Тарка
9
@Slokun - интуитивно понятный вопрос не столько в удобстве обслуживания, сколько в хрупкости. Например, рассмотрим, что происходит, когда размер скидки изменяется, а 0,9 жестко задано в шести разных местах. Кроме того, использование строк вместо констант / перечислений вызывает проблемы в языках с чувствительными к регистру строками.
JohnFx
+1 Я потратил слишком много времени на отладку проблемы, вызванной строкой 'timeout = 15;' похоронен в программе.
Обриродес
Я думаю, что последнее иногда хорошо, в зависимости от того, откуда эти данные для invoiceStatus. Если он только что поступил из общедоступного API, который возвращает JSON, который был только что декодирован, проверка на жестко закодированную константу String, вероятно, подойдет. Согласитесь, однако, что «200» и «0,9» являются просто магическими константами и не должны жестко кодироваться таким образом. Даже если они используются только в этом одном месте, обслуживание будет проще, если вы определите их в разделе конфигурации отдельно, а не добавите их в логический код. И если они используются в нескольких местах, обслуживание становится намного проще.
Бен Ли
36
  • Возможно не худший, но наглядно показывает уровень реализации:

    if(something == true) 
  • Если язык имеет цикл for или конструкцию итератора, то использование цикла while также демонстрирует уровень понимания языка разработчиками:

    count = 0; 
    while(count < items.size()){
       do stuff
       count ++; 
    }
    
    for(i = 0; i < items.size(); i++){
      do stuff 
    }
    //Sure this is not terrible but use the language the way it was meant to be used.
  • Плохое написание / грамматика в документации / комментариях кушает у меня почти столько же, сколько сам код. Причина этого в том, что код предназначен для чтения людьми и запуска машин. Вот почему мы используем языки высокого уровня, если ваша документация трудна для прохождения, это заставляет меня упреждающе формировать негативное мнение о базе кода, не глядя на нее.

Крис
источник
29

Тот, который я сразу заметил, - это частота глубоко вложенных блоков кода (если, то, и т. Д.). Если код часто имеет глубину более двух или трех уровней, это признак проблемы дизайна / логики. И если он идет как 8 гнезд глубиной, то должна быть чертовски веская причина, чтобы он не разбился.

GrandmasterB
источник
6
Я знаю, что некоторые люди узнали, что каждый метод должен иметь только одно returnутверждение, но когда он вызывает более 6 уровней вложенности, то я думаю, что он приносит гораздо больше вреда, чем пользы.
Дариен
28

Оценивая студенческую программу, я иногда могу сказать, что это мгновение. Это мгновенные подсказки:

  • Плохое или противоречивое форматирование
  • Более двух пустых строк подряд
  • Нестандартные или противоречивые соглашения об именах
  • Повторный код, чем больше дословно повторяется, тем сильнее предупреждение
  • То, что должно быть простым фрагментом кода, слишком сложно (например, проверка аргументов, переданных main, сложным образом)

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

При рассмотрении более сложного кода, это мои другие предупреждения:

  • Наличие многих классов Java, которые являются только «структурами» для хранения данных. Неважно, являются ли поля общедоступными или частными и используют геттеры / сеттеры, это вряд ли является частью хорошо продуманного дизайна.
  • Классы с плохими именами, такие как простое пространство имен и в коде нет единого смысла
  • Ссылка на шаблоны проектирования, которые даже не используются в коде
  • Пустые обработчики исключений без объяснения
  • Когда я открываю код в Eclipse, сотни желтых «предупреждений» выстраивают код, в основном из-за неиспользованного импорта или переменных

С точки зрения стиля я вообще не люблю видеть:

  • Javadoc комментирует, что только повторяет код

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

оборота Макнейл
источник
Я не вижу, как использование стиля из одного языка в другом является серьезной ошибкой (2, 4, 8 пробелов на отступ ...). Если у ученика мало другого стиля, которому нужно следовать, нет ничего плохого в самосогласованности. Как грейдер вы видите миллиард программ, так что вы находитесь на противоположном конце этого спектра, но это не причина для полного отклонения другого (но последовательного) стиля.
Ник Т
18
Я не вижу ничего плохого в классах, которые просто агрегируют данные - т.е. структуры. Это то, чем являются объекты передачи данных (DTO), и они могут сделать код гораздо более читабельным, чем, скажем, например, передав 20 методов в метод.
Неми
1
Ваш комментарий о структурах на месте. Это нормально, когда данные в исходном виде и не будут изменены каким-либо образом. Но в 95% случаев у вас должны быть некоторые функции в классе для форматирования / обработки данных. Я только что вспомнил часть своего кода, который по сути использует этот шаблон и должен быть улучшен.
Рассерженная шлюха
2
+1 за непоследовательный стиль и дополнительные разрывы строк (я видел случайные отступы, никаких отступов, случайные и меняющиеся соглашения об именах и многое другое - и это в рабочем коде!). Если вы даже не можете получить это право, то что еще вы не можете сделать?
Дин Хардинг
1
Я ищу строку объявления метода с слишком большим отступом относительно тела. Это знак, который они копируют и вставляют из чужого файла.
Барри Браун
25

Персональный фаворит / любимая мозоль: IDE генерирует имена, которые вводятся. Если TextBox1 является основной и важной переменной в вашей системе, у вас есть еще одна вещь, которая произойдет, после проверки кода.

Уайетт Барнетт
источник
25

Полностью неиспользуемые переменные , особенно когда переменная имеет имя, подобное именам переменных, которые используются.

Пересекать
источник
21

Многие люди упомянули:

//TODO: [something not implemented]

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

//TODO: [something that is already implemented]

Todo бесполезны и сбивают с толку, если вы никогда не удосужились удалить их!

Morgan Herlocker
источник
1
Я только что выполнил упражнение на необходимость составить отчет обо всех TODO в нашем выпуске продукта, а также план их размещения. Почти половина оказалась устаревшей.
AShelly
1
-1 Комментарии TODO используются в MS Visual Studio для отслеживания частей проекта, которые все еще нуждаются в работе. IE, IDE хранит список, который отслеживает TODO, чтобы вы могли легко нажимать на них и переходить непосредственно к строке, на которой существует TODO. Я бы предпочел, чтобы TODO были размещены в явном виде, чтобы увидеть, какую работу еще предстоит сделать. Смотрите dotnetperls.com/todo .
Эван Плайс
3
@Evan Plaice: Вау, вы имеете в виду, что VS IDE распознает, когда вы что-то реализовали, и удаляет //TODOкомментарий? Потрясающие!
JBRWilkinson
4
@Prof Plum Почему бы просто не создать политику, в которой лицо, ответственное за TODO, вводит свое имя в комментарии. Таким образом, если есть остатки
Эван Плейс
3
Лучше, чем планировать // TODO , используйте свой трекер ошибок, вот для чего он!
SingleNegationElimination
20

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

BradB
источник
14
Хм .. это зависит от того, что реализуется. Это не было бы неразумно для реализации некоторых сложных алгоритмов, так как они определены. Конечно, если большинство методов таковы, это красный флаг.
Билли ОНил
9
В качестве общего заявления я не согласен, тратя время на постоянный рефакторинг, чтобы все, что соответствует такому навязанному правилу, фактически увеличивало общую стоимость проекта.
анонимный тип
1
Я не согласен, что это правило может увеличить общую стоимость проекта, но я думаю, что это субъективно, потому что это будет зависеть от разработчика (ов). Если вы будете придерживаться общего принципа «разделения интересов» при разработке, то рефакторинг будет меньшей задачей, если вы решите это сделать. Стоит подумать, сколько будет стоить три года, когда разработчики, которые не написали исходный проект, пытаются исправить ошибку с помощью множества методов с 300+ строками кода? Сколько это стоит?
BradB
18
Меня больше раздражает прокрутка вправо, чем прокрутка вниз. Пробелы "свободны".
Вонко вменяемый
4
Я бы лучше прокрутил, чем пропустил весь файл (или несколько файлов), чтобы выяснить, что делает код.
TMN
20

Соединения в именах методов:

public void addEmployeeAndUpdatePayrate(...) 


public int getSalaryOrHourlyPay(int Employee) ....

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

JohnFx
источник
Хммм ... если имя функции точно определяет, что функция делает, то я не согласен. Если метод делает две разные вещи, которые было бы лучше разделить, то я могу согласиться, в зависимости от контекста.
Вонко вменяемый
2
В этом-то и дело. Соединение подразумевает, что, скорее всего, оно делает больше, чем одну вещь. На вопрос, это только то, что повышает мое понимание того, что что-то МОЖЕТ быть неправильным.
JohnFx 25.10.10
3
А что если вам нужно добавить сотрудника и обновить его заработную плату в нескольких местах? Если этот метод содержит два вызова метода ( addEmployee(); updatePayrate();), то я не думаю, что это проблема.
Мэтт Гранде
13

Связывание явно исходного кода GPL в коммерческую программу с закрытым исходным кодом.

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

Боб Мерфи
источник
6
Хотя ваша точка зрения отличная, ваш тон не нужен.
JBRWilkinson
@JBRWilkinson: Спасибо, ты прав. Мои извинения всем.
Боб Мерфи
Я думаю, что ваш заголовок нуждается в переписывании. Как статическое, так и динамическое соединение с исходным кодом GPL является нарушением GPL ...
Гэвин Коутс
Хорошие моменты. Я переписал весь пост. Спасибо всем.
Боб Мерфи
9

Языковая независимость:

  • TODO: not implemented
  • int function(...) { return -1; } (так же, как "не реализовано")
  • Бросать исключение по неисключительной причине.
  • Неправильное использование или непоследовательное использование 0, -1или nullкак исключительные возвращаемые значения.
  • Утверждения без убедительного комментария, объясняющего, почему это никогда не должно потерпеть неудачу.

Определенный язык (C ++):

  • Макросы C ++ в нижнем регистре.
  • Статические или глобальные переменные C ++.
  • Неинициализированные или неиспользованные переменные.
  • Любой, array newкоторый, очевидно, не является безопасным для RAII.
  • Любое использование массива или указателя, которое явно не является безопасным для границ. Это включает в себя printf.
  • Любое использование неинициализированной части массива.

Microsoft C ++ специфично:

  • Любые имена идентификаторов, которые сталкиваются с макросом, уже определенным любым из заголовочных файлов Microsoft SDK.
  • В общем, любое использование Win32 API является большим источником тревожных звонков. Всегда открывайте MSDN и ищите определения аргументов / возвращаемых значений, если сомневаетесь. (Edited)

C ++ / OOP специфично:

  • Наследование реализации (конкретный класс), когда родительский класс имеет как виртуальные, так и не виртуальные методы, без четкого очевидного концептуального различия между тем, что должно / не должно быть виртуальным.
rwong
источник
18
// TODO: Прокомментируйте этот ответ
johnc 25.10.10
8
Я полагаю, «независимость от языка» означает «C / C ++ / Java» сейчас?
Инамати
+1 «Бросить исключение по неисключительной причине» не могу не согласиться!
billy.bob
2
@Inaimathi - комментарии TODO, заглушки функций, злоупотребление исключениями, смешение нулевой / нулевой / пустой семантики и бессмысленные проверки работоспособности присущи всем императивным / ООП-языкам и в некоторой степени всем языкам программирования в целом.
Рей Миясака
Я считаю , что C макросы препроцессора в нижнем регистре в порядке, но только тогда , когда они оценивают свои аргументы только один раз , и в результате только одного оператора.
Джо Д
8

Странный стиль отступа.

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

кругозор
источник
2
или просто признак того, что они являются высоко ценимым индивидуалистическим талантом, который не попал в сеть гомогонизированных практик кодирования, которые никоим образом не связаны с «лучшими практиками».
анонимный тип
1
Я надеюсь, что вы саркастичны. Если кто-то кодирует таким необычным способом, это должно вызвать тревогу. Они могут быть настолько артистичными, насколько захотят, но все же ... тревожные звонки.
Кен
2
Есть несколько необычный стиль (я думаю, что это стиль Утрехта), который я считаю довольно полезным, несмотря на то, что он необычен вне Haskell / ML / F #. Прокрутите вниз до раздела «Формы модуля»: learnyouahaskell.com/making-our-own-types-and-typeclasses . Что хорошо в этом стиле, так это то, что вам не нужно изменять разделитель в предыдущей строке, чтобы добавить новый - что я часто забываю сделать.
Рей Миясака
@ReiMiyasaka Семь лет спустя, но ... Утрехтский стиль действительно раздражает меня. Я полагаю, что ошибка в спецификации Haskell не в том, чтобы навязывать другое «правило размещения» в вертикально организованных списках. Таким образом, синтаксический анализатор может обнаружить новую запись списка, просто проверив отступы, так как каждый в любом случае организует свои списки.
Райан Райх
@RyanReich Странно, семь лет спустя, и мне все еще нравится это. Я согласен, хотя; при всей своей синтаксической неловкости и недостатках, F # также позволяет разделять элементы только новой строкой и отступом (в большинстве случаев), что делает код аккуратным.
Рей Миясака
8

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

Фигово:

if (itemType == "Student") { ... }

Лучше:

private enum itemTypeEnum {
  Student,
  Teacher
}
if (itemType == itemTypeEnum.Student.ToString()) { ... }

Лучший:

private itemTypeEnum itemType;
...
if (itemType == itemTypeEnum.Student) { ... }
Яаков Эллис
источник
Или лучше всего: используйте полиморфизм.
Лстор
8

Слабо типизированные параметры или возвращаемые значения в методах.

public DataTable GetEmployees() {...}

public DateTime getHireDate(DataTable EmployeeInfo) {...}

public void FireEmployee(Object EmployeeObjectOrEmployeeID) {...}
JohnFx
источник
2
+1: мне приходится работать с некоторыми «REST» веб-сервисами, которые возвращают все в виде псевдо-HTML-таблиц, даже если вы передаете вещи, которые являются явной синтаксической ошибкой. Не авторизован? Ввод полный мусор? Сервер перегружен? 200 (плюс сообщение в ужасном формате в один столбец, одну таблицу строк). AAaaaaaaaargh!
Donal Fellows
7
  • Несколько троичных операторов соединены вместе, поэтому вместо того, чтобы напоминать if...elseблок, он становится if...else if...[...]...elseблоком
  • Длинные имена переменных без подчеркивания или camelCasing. Пример из некоторого кода, который я вытащил:$lesseeloginaccountservice
  • Сотни строк кода в файле, комментарии практически отсутствуют, а код очень неочевиден
  • Слишком сложные ifзаявления. Пример из кода: к if (!($lessee_obj instanceof Lessee && $lessee_obj != NULL))которому я стучалif ($lessee_obj == null)
Тарка
источник
7

Запах кода: не следование передовым методам

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

Вот новость для вас: у 50% населения мира уровень интеллекта ниже среднего. Хорошо, так что некоторые люди будут обладать средним интеллектом, но давайте не будем придирчивыми. Кроме того, одним из побочных эффектов глупости является то, что вы не можете распознать свою собственную глупость! Вещи выглядят не так хорошо, если объединить эти утверждения.

Какие вещи мгновенно звонят в тревогу при взгляде на код?

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

Лучшие практики, как правило, не придуманы случайно, и часто существуют по какой-то причине. Много раз это может быть субъективно, но по моему опыту они в основном оправданы. Следование лучшим практикам не должно быть проблемой, и если вам интересно, почему они такие, как есть, изучите это, а не игнорируйте и / или жалуйтесь на это - возможно, это оправдано, а может, и нет.

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

if (something) {
    // whatever
}

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

Помните: «Потому что это лучшая практика» никогда не является приемлемым ответом на вопрос «почему вы это делаете?» Если вы не можете сформулировать, почему что-то является лучшей практикой, тогда это не лучшая практика, это суеверие.

Vetle
источник
2
Это может быть педантично, но я думаю, что имеет значение, какой средний вы выбираете. Насколько я понимаю, 50% населения мира находится ниже среднего уровня интеллекта (по определению); но другие средние не работают так же. Например, возьмем популяцию (1, 1, 1, 5) со средним арифметическим значением 2.
Фламинговый пингвин 25.10.10
ммм, вы процитировали пост "что-суеверия-делать-программисты-есть", где пользователь сделал смелое заявление о фигурных скобках без источника. Я не считаю это хорошим примером передового опыта.
Эван Плейс
@Evan: да, ты прав. Я добавил немного больше об этом, надеюсь, это поможет.
Ветле
4
Обратная сторона этого - люди, которые рабски следуют «лучшим практикам», не задумываясь о том, почему что-то является «лучшим опытом». Вот почему мне сильно не нравится термин «лучшая практика», потому что для некоторых людей это повод перестать думать и следовать за стадом. «Потому что это лучшая практика» никогда не является приемлемым ответом на вопрос «зачем ты это делаешь?» Если вы не можете сформулировать, почему что-то является лучшей практикой, тогда это не лучшая практика, это суеверие.
Дэн Дайер
Очень хороший комментарий, Дэн! Я добавил две последние строки в ответ.
Ветле
6

Комментарии, в которых говорится: «Это потому, что дизайн подсистемы Froz полностью загружен».

Это продолжается весь абзац.

Они объясняют, что должен произойти следующий рефакторинг.

Но не сделал этого.

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

Если руководитель считает, что j.random. программист не может сделать рефакторинг, тогда супервайзер должен это сделать.

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

Правдивая история. Это может произойти с тобой.

Тим Виллискрофт
источник
6

Может кто-нибудь придумать пример, когда код должен законно ссылаться на файл по абсолютному пути?

Рей Миясака
источник
1
XML-схемы рассчитывают?
Ник Т
1
Файлы конфигурации системы. Обычно должен быть установлен с помощью ./configure, но даже для этого нужно где-то значение по умолчанию.
Эсвальд
4
/dev/nullи друзья в порядке. Но даже такие вещи /bin/bashподозрительны - что, если у вас есть какая-то странная система, которая имеет /usr/bin/bash?
Том Андерсон
1
Код клиента веб-службы, сгенерированный инструментами JAX-WS (по крайней мере, JBossWS и Metro), содержит абсолютный путь к файлу WSDL (дважды!). Что, вероятно, является чем-то совершенно неуместным /home/tom/dev/randomhacking/thing.wsdl. Это преступно безумно, что это поведение по умолчанию.
Том Андерсон
4
о /dev/null: У меня есть привычка при разработке под Windows держать приложения и библиотеки под c:\dev. Каким-то образом папка nullвсегда автоматически создается внутри этой папки. Клянусь, я понятия не имею, кто это делает. (Одна из моих любимых ошибок / функций)
Шон Патрик Флойд
6

Ловить общие исключения:

try {

 ...

} catch {
}

или же

try {

 ...

} catch (Exception ex) {
}

Регион чрезмерного использования

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

Erik van Brakel
источник
Поймать общие исключения - это проблема, только если вы выбросите их, ничего не делая. Действительно, для большинства вещей достаточно одного класса исключений. Я склонен использовать runtime_error.
CashCow
+1 за пример «лови и выбрасывай исключение». Если вы ничего не делаете за исключением, не улавливайте это. По крайней мере, зарегистрируйтесь. По крайней мере, добавьте комментарий, объясняющий, почему можно перехватывать все исключения и переходить к следующему этапу кода.
EZ Hart
5

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

Чрезвычайный пример приходит на ум в классе VB, который я когда-то видел и который был назван Dataдлиной более 30 000 строк ... в первом файле. Это был неполный класс, разбитый как минимум на полдюжины других файлов. Большинство методов были обертками вокруг хранимых процедур с именами вроде FindXByYWithZ().

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

Брайан М.
источник
5

Функции, которые переопределяют основные функциональные возможности языка. Например, если вы когда-нибудь увидите метод «getStringLength ()» в JavaScript вместо вызова свойства «.length» строки, вы знаете, что у вас проблемы.

Муравей
источник
5
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...
#define ...

Конечно , без каких - либо документации и иногда вложенных #defineс

Свен
источник
Я точно видел этот «шаблон» вчера ... в производственном коде ... и еще хуже ... в производственном коде C ++: - /
Оливер Вейлер