2 признака кода с душком: убей его и лови всё молча
Знакомы ли вы с понятием “Код с душком”? Если нет, то коротко говоря - это плохой код. Термин был введен Мартином Фаулером в его книге Рефакторинг. Улучшение существующего кода и с тех пор очень активно используется в программерских кругах. Да и менеджерам, управляющим разработкой ПО, было бы неплохо знать признаки кода с душком, чтобы уметь распознавать качество кода, создаваемого командой.
У Фаулера в книге целая большая глава посвящена признакам кода с душком.
Сейчас я хотел бы с вами обсудить два не совсем классических признака такого кода, с которыми сталкиваюсь постоянно и которые создают большие проблемы.
1. Код с TerminateThread или аналогами. Условное название “Убей его”
2. Код с пустым блоком catch(…) или аналогами типа catch (Throwable t) в Java. Условное название “Лови всё молча”
Кажется, что эти два признака не имеют ничего общего, но на самом деле имеют. Их связывает причина, по которой они применяются - их применяют, чтобы скрыть плохой код.
“Плохость” кода обычно значит, что программист не понимает, что происходит или будет происходить в коде, поэтому он оборачивает его в catch(…). Либо же он сомневается, что поток достаточно хорошо написан и он может зависнуть, так что программист использует TerminateThread. Причина в обоих случаях - нежелание разбираться и делать все правильно.
Вначале кажется, что и TerminateThread и catch(…) справляются со своей задачей и все отлично. Но на самом деле они просто скрывают реальные проблемы и эти проблемы в итоге остаются неисправленными и, например, программа начинает падать или зависать в самых неожиданных местах.
Рассмотрим эти два признака по отдельности.
1. TerminateThread с аналогами.
Вариантов того, как может навредить TerminateThread - множество.
MSDN нам внушает:
TerminateThread is a dangerous function that should only be used in the most extreme cases. You should call TerminateThread only if you know exactly what the target thread is doing, and you control all of the code that the target thread could possibly be running at the time of the termination. For example, TerminateThread can result in the following problems:
* If the target thread owns a critical section, the critical section will not be released.
* If the target thread is allocating memory from the heap, the heap lock will not be released.
* If the target thread is executing certain kernel32 calls when it is terminated, the kernel32 state for the thread’s process could be inconsistent.
* If the target thread is manipulating the global state of a shared DLL, the state of the DLL could be destroyed, affecting other users of the DLL.
Но даже эти 4 страшных последствия не останавливают многих программистов от применения TerminateThread (а может просто никто их не читает).
Недавно я столкнулся с деадлоком, потому что был убит один из потоков, который владел критической секцией. А другие потоки пытались войти в эту критическую секцию и зависали. При этом зависали нормальные потоки и из дампа понять причину может быть непросто.
Но проблема была даже не в этом.
Зависший и потом уничтоженный c помощью TerminateThread поток уже убит и нет никаких шансов понять, что же там пошло не так! При этом то, что остальные потоки зависли - это отлично, так как указало на проблему. Обычно же этот поток убивался безболезненно и никто даже не знал о проблеме. Но как всегда в одном из миллиона запусков дедлок случался.
В итоге, если убрать TerminateThread и дождаться зависания потока, то можно исправить реальную проблему в этом самом потоке и программа перестает зависать при каждом милионном запуске.
Если вы думаете, что зависание на каждом милионном запуске - это нечасто, то представьте, что ваш продукт установлен на 20 млн. компьютеров и программа запускается 10 раз в день - в итоге у вас будет 200 зависаний в день и 70000 в год, многие из которых трансформируются в звонки в техподдержку или в баг-репорты, которые кто-то будет изучать и пытаться исправить.
Много работы из-за временной слабости программиста.
При этом иногда TerminateThread должен применяться и может быть полезным. Например, если вы разрабатываете сетевой сервис или серверное приложение. Там надо быть уверенным, что ничего не будет падать и, если что-то пошло не так и поток завис, то правильнее его убить.
Но даже в этом случае обязательно надо сохранять состояние процесса. Например, создавать дамп или хотя бы сохранять в лог калстэк и состояние убиваемого потока - это сократит вам время отладки в разы. Но создаст другие проблемы. Например, дампы могут занимать очень много места и заполнить весь диск. А получение калстэка зависшего процесса в C++ может быть нетривиальной задачей и источником ошибок.
В общем, как всегда - выбор за вами.
2. Код с catch(…) или аналогами типа catch (Throwable t) в Java.
Про использование catch(…) я встречал несколько холиваров, где кто-то доказывал его полезность, а другие - вредность.
В целом могу согласиться, что иногда это может быть полезным. Например, у вас завтра непереносимый релиз, а какой-то кусок кода кидает непонятные исключения и вы не можете их понять и исправить - можно понять catch(…) в этом месте на пару дней. Но его нужно сразу же убрать после релиза. Почему?
Потому что catch(…) прячет ошибки. Даже если вы напишете вывод чего-то в лог в этом блоке catch, то это ничего не даст - контекст ошибки уже потерян. Да, программа не упадет, но станет работать дальше неправильно.
Если вы в каждом блоке catch(…) сохраняете полное состояние ошибки (дамп, калстэк, регистры и т.п.) - это поможет с отладкой, но опять же, сколько места на диске это потребует и какие другие баги и тормоза это добавит в ваш продукт?
Что лучше - мгновенное падение в месте ошибки или продолжение работы в неопределенном состоянии и возможное падение где угодно, если что-то испортилось? Выбирать вам.
Я помню один случай, когда такой блок catch(…) с выводом в лог привел к тому, что у одного из игроков в нашу игру лог заполнил весь жесткий диск - 120Гб. При этом понять из этого лога причину ошибки было невозможно, была видна только функция, где была ошибка.
Еще помню, мы как-то использовали catch(…) повсеместно в коде одной игры во время разработки и получали изредка репорты о падениях в самых разных функциях, в которых глюков не было и не могло быть. При этом реальные ошибки маскировались этими catch(…) и в принципе казалось, что все работает стабильно. Но в итоге было принято решение все catch(…) убрать, после чего была пара ужасных недель, когда все начало падать постоянно, зато через эти две недели все реальные ошибки были найдены и исправлены. И код стал гораздо стабильнее. И, главное, мы после этого знали, что отсутствие падений - это не заслуга кучи catch(…), а именно отсутствие критических глюков. Это знание многого стоит!
В качестве заключения хочу предложить вам прямо сейчас открыть свои проекты и поискать там catch(…), TerminateThread, TerminateProcess и т.п. Найдёте - задумайтесь и исправьте.
А также подумайте о внесении запрета на их использование в свои стандарты кодирования. Они же у вас есть, правда?
P.S.:
Признаков кода с душком можно написать еще очень много. Я выбрал эти два, так как они достаточно спорные и в то же время очевидные. С удовольствием подискутирую на эту тему.
Как вы относитесь к применению TerminateThread и catch(…) в коде?
Понравилась статья? Подпишись на RSS!
Февраль 19th, 2009 | | Category: Программирование, Разработка ПО, Средний уровень, Статья | Комментариев: 58
Я как то на коленках написал монстра - snmp/syslog коллектор, потом докрутил к нему sql и всё это дело жило в операторе связи (по моему живо и щас). Но жуть полнейшая, как щас помню написал на delphi 7, работало нормально(функции исполнялись то ТЗ от и до), только я там окошко для дебага создал, куда все сообщения кидались, а вот чистить я его забыл. При 200 - 300 msg per sec сервер моментально умирал
Ошибки молодости 
Но самое смешное, что я 3 раза софтинку переписывал, добавляя функционал, с начала, код терял постоянно, ну конечно про окошко с дебагом не забывал, каждый раз его лепил.
Да, точно Tmemo поле было.
На самом деле я бы не высказывался столь категорично. В играх и десктопных приложениях это возможно и так,а вот насчет серверных (если уж ты тут жабу упомянул) - нет.
у нас используется оба подхода, т.к. неотловленное исключение ни в коем случае не должно приводить к падению/повреждению всего сервера. т.е. мы как раз должны либо тихо прибить конкретеный трэд/поток, либо штатно завершить обработку клиенентского запроса, чтобы это не затронуло другие обрабатываемые запросы.
Прибивая поток ты тоже порождаешь баги. Только эти баги скрыты и тебе их будет гораздо сложнее обнаружить. Как вы в итоге отлаживаете такие зависшие потоки? В яве есть способы снять колстэк, например, перед прибиванием?
И про catch(…) то же самое. Только помнится мне, что в яве можно из эксепшена всю инфу о нем вытащить легко. Калстэк и т.п. То есть ты не теряешь контекст ошибки в catch. Так это?
В любом случае интересно было бы почитать, как вы хэндлите такие ошибки и как вы их потом исправляете
На тему catch(…) вставлю слово. Я, например, не вижу ничего особо дурного в catch(…) в функции main.
int main() {
try {
…
} catch (…) {
log(”Unhandled exception”);
}
}
Если программа является долгоиграющим сервисом, например, сетевым, то по мне так лучше такая запись в логе (которую, в принципе можно проанализировать), чем падение и, как следствие, отвал все текущих конненкшенов. Такой же защитный try-catch(…) с обязательным логгированием я использую для потоков. Если упает main() - это хоть как-то видно, а вот если втихую умрет поток из-за непойманого исключения, это фиг найдешь потом.
Но это пожалуй единственное по мне нормальное применение catch(…).
>>Я, например, не вижу ничего особо дурного в catch(…) в функции main.
Как только в функции main появилась эта строчка - вы уже не контролируете свой код. Он может падать 100 раз в секунду, а вы этого не увидите. Или он будет падать только при особых условиях у юзера, а вы по логу не сможете понять где и как. А дампа не будет. И в итоге все равно приложение упадет со временем, только уже совсем в другом месте, например, если ваш “плохой” код испортил кучу, а хороший к ней обратился и упал.
Это все очень трудно отлаживать.
Я готов согласиться, что в некоторых случаях это нужно. Но на этипе разработки и отладки все эти catch(…) ДОЛЖНЫ быть убраны - нужно сразу получать фидбек от ошибок с контекстом ошибки, а не просто запись в лог.
А вот включать ли catch(…) в релиз - это уже вопрос сложный. Если важное сетевое или серверное приложение - может и стоит. Но тогда надо озаботиться обязательно тем, чтобы в лог писать не просто Unhandled, а какую-то информацию о контексте ошибки.
Либо же еще лучше - сделать такой catch(…) отключаемым в реестре. Тогда важный юзер может отключить его и репродьюсить ошибку - вы получите дамп и исправите ее.
Все зависит от всего. Точно так же можно рассуждать о пользе и вреде goto.
У меня, к примеру, в “домашнем” проекте используется не просто запись в лог “Unhandled exception”, а запись стека вызовов функций. Первое время, я пробовал снимать минидамп, используя библиотеки Microsoft, но это решение очень сложно переносить. А запись в текстовой файл можно реализовать на любых платформах.
Грубо говоря, вызов функций обрамлен в макрос, который записывает имя функции (адрес строковой константы - 4 байта) в стек. Когда функция завершает работу, последнее “описание” выталкивается. Таким образом, если внутри функции что-то упадет, и возникнет Unhandled exception, можно увидеть полный стек вызовов.
Понятное дело, что это только страховка, и вообще таких ситуаций происходить не должно. Но ИМХО часто (тем более - для графических приложений) нужен механизм, который позволяет проводить отладку в промышленной сборке. Дело не только в разных конфигурациях. Исключение может порождаться отсутствием библиотек на машине пользователя (Microsoft Visual C++ redistributable package, к примеру).
Да, если в каждой функции есть этот макрос, то ты не теряешь “контекст”, а значит можешь и catch(…) использовать - по крайней мере калстэк у тебя будет. Но это же куча работы и не у всех такое есть.
Оба правила сводятся к одному мега-правилу “не замалчивайте ошибки!”.
Все настолько очевидно, что даже и добавить не чего…
зы. Хотя убиение молотком трида своего рода “оптимизация” кода потока, т.к. в него не нужно встраивать механизм завершения его работы извне.
код типа
int main() {
try {
…
} catch (…) {
log(”Unhandled exception”);
}
}
можно писать только так:
int main() {
try {
…
}
catch(const exception &e;)
{
log(”Unhandled exception ” + e.what()); // + добавить typeid info
}
catch (…)
{
log(”Unhandled exception”);
}
}
В целом согласен.
Но хотел бы спросить, а как быть с функциями, которые используют catch(…) намеренно ?
Например у меня есть “стандартная” функция Assign()
// checking assigned pointerinline bool Assigned(const void *p)
{
if (!p) return false;
/*
* in debug mode checking readable of data from pointer,
* in release don't checked
*/
# if defined(_DEBUG)
void *l_ptr = const_cast (p);
uint32_t l_ui32Temp = 0x00;
uint32_t *l_pui32Temp = reinterpret_cast (l_ptr);
try
{
// try read data from pointer
l_ui32Temp = *l_pui32Temp;
}
catch (...)
{
return false;
}
# endif /* _DEBUG */
return true;
} /* Assigned */
Я знаю про WinAPI функции, проверяющие память на читаемость/запись, но мне использовать свою функцию удобнее.
Если знаете правильную альтернативу - буду признателен.
>>inline bool Assigned(const void *p)
Такие функции - это как раз причина множества падений и непоняток.
Что если p==0×1?
Что если p указывает на системную память?
Попробуйте написать тест для Assigned, куда просто передавайте все указатели от 0 до MAX_INT - не упадет?
В любом случае не факт, что оно не будет падать со временем - новые ОС, новые компиляторы и т.п. - все меняется.
Выше ответил, копипастю сюда тоже:
Любой код, где есть проверка на валидность указателя в памяти - уже с душком, так как программист как бы сам себе не доверяет и перепроверяет
Причем обычно такой код появляется после непонятных падений. Когда программист не может исправить ошибку и начинает защищаться от нее такими функциями. Но они опять же не помогают, а только прячут реальную ошибку и позволяют жить с ней.
Любой код должен относится к передаваемым ему извне указателям как к потенциально неверным. Программист не доверяет тем, кто вызывает его код, себе-то он вполне доверяет
Проверки на валидность указателей и блоков вставляют в _DEBUG-версии, некоторым это помогает (тэм, кто умээт…. sorry 
)
VirtualQuery
Имхо запросить атрибуты региона дешевле, чем обрабатывать исключение.
Как-то я писал приложение, где один поток принимал входящие соединения, а другой их обрабатывал. При этом в постановке задачи значилось, что при ошибках должна быть запись в лог и выход из приложения. Как тут обойтись без pthread_kill? (Система - *nix)
Про catch(…) есть ещё один интересный момент - тот же pthread_kill вызывает исключение в отменяемом потоке, который при помощи catch(…) отлично отлавливается. Т.е. отлов исключений в этом случае вообще вреден, ибо нарушает ожидаемое поведение.
В юниксе можно отменить pthread_kill? Клево
Под виндой нельзя.
>>При этом в постановке задачи значилось, что при ошибках должна быть запись в лог и выход из приложения
Ну, если такая задача, то нет вопросов. Вопросы только к тем, кто задачу ставил, а не к программисту
Да, клево иметь pthread_kill(), который не всегда thread kill. Еще иметь kill(), который тоже не всегда kill, или realloc, который не всегда realloc, тоже клево.
Брр… Не дай бог с таким кодом работать(
У нас в PHP тоже есть средство для игнорирования ошибок, но я предпочитаю отлавливать и записывать все ошибки, и чуть что пошло не так — завершать скрипт. В правильно написанной программе ошибок быть не должно (тем более в такой простой, как веб-скрипт).
А что, в C++ нет аналога TerminateTread, который корректно отпускает все блокировки — или это невозможно в принципе?
>>А что, в C++ нет аналога TerminateTread, который корректно отпускает все блокировки — или это невозможно в принципе?
Не вижу ничего невозможного. Уж отпустить все критические секции - это несложно было бы сделать.
Восстановить состояние кучи - сложнее, но тоже наверняка возможно.
Недоработали просто, наверное.
Я так понимаю, этот код работает при использовании опции компилятора /EHa? В общем, я считаю этот код то-же с запашком, так-как в вашей программе существует возможность разименования невалидного указателя.
Любой код, где есть проверка на валидность указателя в памяти - уже с душком, так как программист как бы сам себе не доверяет и перепроверяет
Причем обычно такой код появляется после непонятных падений. Когда программист не может исправить ошибку и начинает защищаться от нее такими функциями. Но они опять же не помогают, а только прячут реальную ошибку и позволяют жить с ней.
Вот кстати, из комментов виден один из недостатков C++ — он дает слишком много свободы в плане использования указателей, в результате мы получаем ненадежный (надежный код не разыменовывает кривые указатели, и вообще их не использует), непереносимый (так как в Линуксе я подозреваю такая конструкция вызовет Access Violation) и просто кривой код. Извраты какие-то короче.
2Egor:
Для отладки программы и проверки утечек памяти/неправильного освобождения/кривых указателей, есть средства вроде Valgrind (под Windows думаю должен быть какой-то аналог), стоит ли сво
2Egor:
Для отладки программы и проверки утечек памяти/неправильного освобождения/кривых указателей, есть средства вроде Valgrind (под Windows думаю должен быть какой-то аналог), стоит ли свой велосипед городить?
Хмм…. не знаю, не знаю… TerminateThread ваш друг, не оказывайте сопротивления
Все мои потоки завершаются по евенту, и я ожидаю их завершения, если через какое-то время они не завершаются, вызываю TerminateThread, пишу в лог id зависшего потока, сохраняю дамп, генерирую исключение. Это даст пользователю понять что именно произшло (завершение приложения произошло некорректно). Он отправит в саппорт лог и dump, и имея под рукой логи, pdb или map файлы можно вполне быстро понять, что же именно у человека произошло. А если у человека просто тупо зависнет приложение, что ему делать, как понять что произошло?
Если вы сохранили дамп, написали в лог и предложили дамп выслать - вы не скрыли ошибку. Как раз наоборот - вы ее сохранили для себя, но для юзера сделали все, чтобы ошибка не создала ему проблем - Win-Win.
В этом варианте TerminateThread очень даже полезен.
не согласен насчет try…catch(Throwable _thr)
думаю что тут надо уточнить, что плохим будет именно пустой блок в catch, т.е. если человек просто тупо закрыл глаза на возможные проблемы, но про плохое качество уже не приходится говорить если в этом блоке о проблемах или сигнализируется или они отрабатываются
А можно ли в Яве легко получить полную информацию о проблеме (калстек, состояние регистров и т.п.) внутри catch(Throwable _thr)?
Если да, то действительно это не проблема, если catch(Throwable _thr) не пустой, а обрабатывает это и пишет в лог или еще куда.
>А можно ли в Яве легко получить полную информацию о проблеме (калстек, состояние регистров и т.п.) внутри catch(Throwable _thr)
регистров в яве нет, так что такое не унаешь, но можно распечатать стек вызовов приведший к падению и узнать в какой строке проблема, так же можно узнать класс эксепшена, т.е. объект Throwable несет достаточно приличный объем информации в себе
Тогда в Яве все гораздо проще - достаточно не оставлять пустым блок catch и сохранять контекст. Поправил статью чуток.
Прибивая поток ты тоже порождаешь баги. Только эти баги скрыты и тебе их будет гораздо сложнее обнаружить.
Как вы в итоге отлаживаете такие зависшие потоки?
возможно порождаю, возможно нет - в любом случае в системе, работающей в режиме 24×7, другого выбора у нас нет. Ронять весь сервер мы не можем.
В яве есть способы снять колстэк, например, перед прибиванием?
Да не вопрос абсолютно.
пример кода : http://pastebin.com/f5af7f3f7
А вот то, что будет в консоли: http://pastebin.com/d701e28a0
Только помнится мне, что в яве можно из эксепшена всю инфу о нем вытащить легко. Калстэк и т.п. То есть ты не теряешь контекст ошибки в catch. Так это?
ну, как водится, есть пара ньюансов(тм), но вообще это так.
В любом случае интересно было бы почитать, как вы хэндлите такие ошибки и как вы их потом исправляете
Ну общий путь у нас примерно такой :
0. Пишем мессагу и стектрейс в лог.
1. По возможности закрываем ресурсы, требующие закрытия - соединения с БД, сокеты/файлы и т.д.
2. Заверщаем обработку запроса.
Собственно на рабочей системе, как правило, единственный источник инфы о том, где что произошло - это лог(логи). Изредка кастомер может внятно пояснить что он делал перед тем, как произошел сбой.
ну а дальше я думаю понятно - используя знания, опыт, бубен и чОрную магию выявляем и исправляем
если известны шаги по воспроизведению бага, то стараемся его воспроизвести на локальном или на тестовом сервере(что не всегда возможно кстати). тут все, естественно, проще - можно повтыкать отладочного логгинга, расставить брейкпоинтов в отладчике … ну короче ты знаешь
Я помню один случай, когда такой блок catch(…) с выводом в лог привел к тому, что у одного из игроков в нашу игру лог заполнил весь жесткий диск - 120Гб.
Не у нас такого ужоса не бывает. ибо с помощью System.io.println в логи пишут тока либо нубы, либо совсем неадекватные челы
Для логгинга в жабе народ обычно юзает Apache log4j(http://en.wikipedia.org/wiki/Log4J) - там настраивается и набор файлов куда писать и транкейт по размеру и много чего еще. Если интересно у этой либы есть порт на С++ : http://logging.apache.org/log4cxx/
Прибивая поток ты тоже порождаешь баги. Только эти баги скрыты и тебе их будет гораздо сложнее обнаружить.
Как вы в итоге отлаживаете такие зависшие потоки?
Да, только сейчас заметил… с чего ты решил, что в жабе поток, где мы в блоке try…catch(Throwable) словили неожиданное исключение будет непременно зависшим ?
Не, catch(Throwable) и прибивание зависшего потока - это разные проблемы.
С зависшего потока никакой эксепшен ты уже не поймаешь
Не, catch(Throwable) и прибивание зависшего потока - это разные проблемы.
С зависшего потока никакой эксепшен ты уже не поймаешь
Тогда понятно. в случае с подвисшими потоками или дедлоками какого-то стандартного пути нет. тут по части разного рода ватчдогов и хелсмониторов народ изголяется кто во что горазд. да, в жабе с подвисшего потока тоже полезной инфы по большому счету не нарыть (ну кроме имени разве что). соотв. если бага всплыла на рабочем сервере - то разбираться только по логам если получится. так что если такой поток обнаружен - остается его только прибить. Если проблема вылезла на тестовом или локальном серваке, то есть спецтулзы которые дедлоки и подвисания секут. Например вот - http://www.yourkit.com/features/index.jsp
Если проблема вылезла на тестовом или локальном серваке, то есть спецтулзы которые дедлоки и подвисания секут.
Если что-то на локальном можно повторить, то это уже почти значит, что исправил 90% дела сделано
К сожалению реальные проблемы обычно удаленные и не репродуцируются.
по поводу catch(…) - как быть с автоматизацией взаимодействия со сторонней программой, которая ведёт себя плохо предсказуемым образом?
Для примера: у нас был проект с СОМ-плагином к MS Office Outlook. при этом нужно было поддерживать версии начиная с 2000 и заканчивая 2007… Учитывая весь зоопарк версий и локализаций - предсказать поведение плагина было не всегда возможно - иногда у аутлука случались критические дни и падал он от простого изменения фазы луны, и опасные для падения места приходилось просто латать catch(…). А учитывая то. что ком-интерфейсы с 2к до 2007 поменялись значительно, то это был ещё тот цирк. Когда отказались от поддержки офиса 2к все catch(…) просто ушли.
Кстати, про аутлук - у нас сейчас парни мучаются с WinMail-ом под висту и плагином к нему. Там не просто в каждой версии разница - даже один и тот же аутлук под разными ОС по-разному работает.
Маскировать там баги через catch - опасно, хоть их и много - лучше исправить постепенно.
Но у нас нет задачи поддерживать 2K - там, уверен, все было гораздо хуже и catch может и был нужен.
И опять же, ваш пример подтверждает правило - catch(…) применяется, когда программист не уверен в правильности работы кода и просто “прикрывает” дыру :). Иногда это даже может быть оправдано.
catch(…) вовсе не зло, если в нем производят запись в лог и проброс эксепшена.
Проброс эксепшена - это не маскирование ошибки. Это нормальный подход.
Я написал про те catch(…) которые используются именно для маскирования ошибки - “лови всё молча”.
Написал у себя в блоге новый пост
http://gineer.livejournal.com/7299.html
Какраз подходящий для обсуждения вопросов программировани (имхо).
Хотелось бы пообсуждать (мой блог пока не так раскачан, немного людей его посещает).
Так что если можно/интересно сдублировать его у вас в блоге и пообсуждать?
Я вчера еще в жж хотел откомментировать, но не стал
На самом деле там в обоих вариантах есть проблемы. В первом - слишком длинная функция. Антипаттерн “божественный метод”
Во втором - или оформление слетело. Или большие проблемы с coding standards. Несколько операндов в одной строке - жуть. Да и все равно метод длинноват.
Я бы его еще уменьшил и разбил на 2-3
//Во втором - или оформление слетело. Или большие проблемы с coding standards. Несколько операндов в одной строке - жуть. Да и все равно метод длинноват.
Так и задумано. Не могу жить без контраверсии.
Значит есть предмет для обсуждения?
У меня какраз есть куча коментариев к длинному методу. и буду раз выслушать коментарии в сторону короткого.
//Я бы его еще уменьшил и разбил на 2-3
Э-хэх. Думаете я об этом не подумал. Там не все так просто (хотя, если приведете пример).
Да и в общем контексте бессмысленно — незачем там плодить дополнительные функции.
ЗЫ Я что-то не пойму, это настройки поменялись или что, что нет теперь возможности прямо коментировать ваш коментарий?
Э-хэх. Думаете я об этом не подумал. Там не все так просто (хотя, если приведете пример).
Да и в общем контексте бессмысленно — незачем там плодить дополнительные функции.
Попробую вечером заняться этой функцией вплотную и напишу, как бы я ее изменил
Пару часов работы не гарантирую, но посмотрим, что получится минут 15-20.
ЗЫ Я что-то не пойму, это настройки поменялись или что, что нет теперь возможности прямо коментировать ваш коментарий?
хм, странно. У меня есть такая возможность. Изменил немного настройки - проверьте сейчас.
Интерес к разбору кода уже совсем пропал?
Времени и сил реально нет на это. Но у меня запомнено
//хм, странно. У меня есть такая возможность. Изменил немного настройки - проверьте сейчас.
Угу. Теперь появилась ссылочка “Ответить”, но возле любых постов кроме ваших.
//Попробую вечером заняться этой функцией вплотную и напишу, как бы я ее изменил
В том файлике она не одна такая.
Я уже отправил разрабам патчик, если примут.
И не думайте, там нет того коде-стайлового “безобразия” на которое вы мне указали. И в ЖЖ-посте уже исправил и поместил все под кат, от греха подельше.
Я не идиот, скорее оригинал — пробую смотреть нестандартно на стандартные вещи. Но от соответствия стандартам не отказываюсь.
Угу. Теперь появилась ссылочка “Ответить”, но возле любых постов кроме ваших.
Отключил плагин древовидных комментариев вообще. Древовидные комментарии остались, а глюки исчезли
Я не идиот, скорее оригинал — пробую смотреть нестандартно на стандартные вещи. Но от соответствия стандартам не отказываюсь.
Стандарты разные бывают. Не удивлюсь, если где-то поощряют написание нескольких операций на одной строке. Все люди разные и нравится всем свое - главное, чтобы все в компании соблюдали единый стандарт.
Дык… в том то и дело, что это объединенные единым смыслом операции, а не просто экономия места.
А стандарты они ведь и устаревают бывает.
Например те же что у Макконела. Поправьте меня, но я так понимаю что разрабатывались они с учетом как выглядит код на распечатке.
А кто сейчас в здравом уме код распечатывает?
Да и возможностей оформления у современных редакторов поболе стало.
Тот же коде-фолдинг.
Плюс, мониторы то сейчас, особенно ТФТ, стали большие и широкие.
И стандартно форматированный код, рассчитаный на узкие и длинные Алисты А4, на мониторе 16:10, а то и 16:9 даже смотрится глупо — занимает только одну половинку экрана.
Не, распечатка тут ни при чем. Основное правило - чтобы код легко читался, выглядел одинаково и не было глупых ошибок.
И кстати, я иногда распечатываю код
И в стандартах Макконнелла вроде как про 80 символов на строке для печати ничего не было. А если и было - это не основное. По крайней мере в моих стандартах такого пункта нет.
1) Легко читался
Это вопрос к привычкам и уровню читателя.
Например, я не люблю Паскаль, но кое-что там рациональное есть — слова begin/end не теряются так легко при редактировании и прочтении, как тоненькая скобка.
Но тем не менее я лично не согласен терпеть такое “удобство”.
2) Выглядел одинаково.
С одной стороны опять же оценочное суждение.
а с другой, тот же еще оксюморон. Как может выглядеть одинаково, то что по определению должно выглядеть различно — програмисты же, не школяры оставленные после уроков писать в наказание одну и ту же фразу — не так ли?
3) Не было глупых ошибок (сиречь опечаток, я так понимаю?)
Это уже немного другой вопрос. Я бы даже сказал интересный. И такой в отношении которого много чего можно было бы сделать, и в принцыпе и делается.
Правда это мои мысли — не уверен что вы имели в виду то же самое…
Да не вопрос. Думаю современные редакторы достаточно умны чтобы любой код красиво втиснуть на листе бумаги.
В крайнем случае есть программы форматирования.
Кстати, еще один момент, почему необъязательно всем-всем насильно писать в одном и том же стиле.
Просто подход к этому вопросу нетехнологичный.
Пусть будет у каждого свой стиль, и перед открытием по файлу проходит програмка-форматер, которая подстраивает под твой стиль, а когда нужно заливать в репозиторий, прогоняется в другой раз на соответствие стилю репозитория.
Пусть будет у каждого свой стиль, и перед открытием по файлу проходит програмка-форматер, которая подстраивает под твой стиль, а когда нужно заливать в репозиторий, прогоняется в другой раз на соответствие стилю репозитория.
Идея интересная. Только кодинг стандарт - это не только оформление кода. Это и правила наименования классов и переменных, и правила, как использовать конструкции языка и многое другое. У гугла, например, выложен отличный кодинг стандарт - там все по пунктам и с объяснениями почему они так сделали.
Так что одним плагином, меняющим оформление, не обойтись.
Нужен плагин, проверяющий код на соответствие стандартам перед комитом!
С ходу могу назвать как минимум пару проектов где что-то подобное делали… жалко правда без конкретного результата.
http://bf-xor.livejournal.com/41346.html
Вот еще один пример “умного кода”
А что на счет обсуждения?
У меня прям руки чешутся, могу накатать кучу претензий к той длинной функции, высветить моменты неправильного дизайна (с моей точки зрения естественно).Но сдерживает то, что а кто же его будет читать — мне очень интересно было бы получить фидбэк на такое, для саморазвития.
Я тут приболел чуток - не добрался вчера до кода.
Про catch(…) зря так категорично. Ведь есть класс ошибок порождаемые не логикой программы, а из-за внешних обстоятельств таких как: разрыв связи, падение сервисов, отказ в обслуживании и тут catch(…) никак не заменим.
Категорично про необработанный catch(…).
К тому же внешние обстоятельства все генерят известные исключения - их и надо ловить и на них реагировать, а не ловить catch(…) для всего подряд. Другое дело, если вы не знаете, что именно может вернуться. Тут можно и … поставить, но нужно тогда полностью продумать, как сохранять статус и описание ошибок, чтобы не потерять контекст.
TerminateThread у нас используется после попытки завершения потока. Устанавливаем событие, ждем 1-2 сек, если поток не завершился, завершаем извне. Т.к. это происходит при завершении процесса, проблем я не вижу… Хотя не отрицаю, что они могут быть.
>Я помню один случай, когда такой блок catch(…) с выводом в лог привел к тому, что у одного из игроков в нашу игру лог заполнил весь жесткий диск - 120Гб. При этом понять из этого лога причину ошибки было невозможно, была видна только функция, где была ошибка.
Грешно кивать на инструмент, тов. епископ. Я ударил молотком по гвоздю, попал в травмпункт.
catch(…) применялся на работе в большой комании, когда использовались “не очень хорошо написанные” библиотечные функции, которые, в принципе, могли кидать большое количество исключений, не унаследованных от общего базового класса
то есть при пересечении лени или неопытности авторов библиотеки и отсутствия в C++ статической проверки спецификации исключений для функций