Здравствуйте, grosborn, Вы писали:
>> То есть нечто, больше относящееся к форматированию. Имена классов типа "BinRes", "a", "b", "ptr" и тп. это в моем понимании просто кривой код, подлежащий рефакторингу. >> Использование хедера #include <string> и при этом параметры типа wchar_t скорее всего тоже кривость. Но тут уже надо разбираться с конкретным случаем.
G>Оппа. Рефакторинг это оказывается переименование идентификаторов. Чудненько, а то я думаю чего это народ все обсуждает, митингует. Теперь я в теме тоже могу обсуждать рефакторинг
А что вас удивляет? Да, рефакторинг, это в том числе и переименование идентификаторов
Здравствуйте, Ulin, Вы писали: U>То есть нечто, больше относящееся к форматированию. Имена классов типа "BinRes", "a", "b", "ptr" и тп. это в моем понимании просто кривой код, подлежащий рефакторингу.
рефакторинг это пустая трата времени. это лишь адаптация одного стиля под другой. если программа работает, значит она выполняет свои ф-ии и не надо в нее лезть своими ручками. если настолько кривая, что не работает, то ее и не существует.
Здравствуйте, __kot2, Вы писали:
__>рефакторинг это пустая трата времени. это лишь адаптация одного стиля под другой. если программа работает, значит она выполняет свои ф-ии и не надо в нее лезть своими ручками. если настолько кривая, что не работает, то ее и не существует.
Я бы, все таки, различал понятия другого стиля и кривого кода. Другой стиль рефакторить, конечно же, смысла нет совершенно никакого. А вот кривой код резко увеличивает стоимость внесения изменений. И его рефакторить часто просто приходится. Особенно когда добавление фичи все равно может много чего поломать, тут уж грех не воспользоваться и не отрефакторить изменяемый модуль.
Здравствуйте, __kot2, Вы писали: __>Здравствуйте, Ulin, Вы писали: U>>То есть нечто, больше относящееся к форматированию. Имена классов типа "BinRes", "a", "b", "ptr" и тп. это в моем понимании просто кривой код, подлежащий рефакторингу. __>рефакторинг это пустая трата времени. это лишь адаптация одного стиля под другой. если программа работает, значит она выполняет свои ф-ии и не надо в нее лезть своими ручками. если настолько кривая, что не работает, то ее и не существует.
добавлю, чтобы правильно поняли
в программировании есть сущности, которые взаимодействуют по определенным интерфейсам. сущности группируются в другие сущности, взаимодействующих с другими группами по другим интерфейсам.
если у нас нет какого-то нужного нам интерфейса, значит нам нужно его сделать, введя новые обьекты и использовав интерфейсы к существующим обьектам
если какой-то интерфейс не работает, значит у нас нет рабочего интерфейса — мы либо правим уже существующий код, либо пишем свой код, смотря что быстрее.
если интерфейс работает, то какая разница как он работает?
расширение интерфейса — дело обычно гиблое, так как обьекты должны быть атомарными. и если кто-то навалил все в одну кучу, то нам не стоит заниматься тем же.
Здравствуйте, Ulin, Вы писали: U>Я бы, все таки, различал понятия другого стиля и кривого кода. Другой стиль рефакторить, конечно же, смысла нет совершенно никакого. А вот кривой код резко увеличивает стоимость внесения изменений. И его рефакторить часто просто приходится. Особенно когда добавление фичи все равно может много чего поломать, тут уж грех не воспользоваться и не отрефакторить изменяемый модуль.
давайте вы приведете пример "некривого кода", а я скажу почему он по-моему мнению кривой и как его нужно изменить? мне аж самому интересно, что из этого получится.
Здравствуйте, __kot2, Вы писали:
__>давайте вы приведете пример "некривого кода", а я скажу почему он по-моему мнению кривой и как его нужно изменить? мне аж самому интересно, что из этого получится.
То, что в моем понимании является стилем — тело функций в хедере, нет пробелов между параметрами функций — не совпадает с тем, как пишу, например, я. Тем не менее рефакторить тут в моем понимании нечего — это действительно просто другой стиль. И если девелопер приходит на проект, где все сорцы написаны так — он просто должен писать так же, хоть это и может противоречить его стилю.
Здравствуйте, Ulin, Вы писали: U>Ну вот, например, вполне ровный, на мой взгляд, код из либы JRTPLIB (http://research.edm.uhasselt.be/~jori/page/index.php?n=CS.Jrtplib):
пример, конечно, мал, я бы хотел некую реализацию какого-то класса,
поэтому давайте возьму лучше rtcpcompoundpacket
начнем с rtcpcompoundpacket.h
>>/** Creates an RTCPCompoundPacket instance from the data in \c rawpack, installing a memory manager if specified. */
куски кода с такими комментами неудобно комментить — они всегда рождают дополнительные маты при портировании и отладке, так как обычно при портировании ничего сразу не собирается, надо это делать частями потихоньку
>>RTCPCompoundPacket(uint8_t *packet, size_t len, bool deletedata = true, RTPMemoryManager *memmgr = 0);
так это С++ или С? почему packed голым указателем? почему указатель = 0? хотя бы NULL нужно писать
составные имена переменных стоило бы для лучшей читабельности разделять подчеркиваниями deletedata -> delete_data
>> int GetCreationError() { return error; }
о, ужас, они про исключения слышали?
получается, можно создать класс имеющий невалидное состоние. зачем? это очень неудобно
что это за паскалевское именование?
>> /** Starts the iteration over the individual RTCP packets in the RTCP compound packet. */ >> void GotoFirstPacket() { rtcppackit = rtcppacklist.begin(); } >> RTCPPacket *GetNextPacket() { if (rtcppackit == rtcppacklist.end()) return 0; RTCPPacket *p = *rtcppackit; rtcppackit++; return p; }
сделали свой интерфейс для итератора. а зачем?
>> size_t GetCompoundPacketLength() { return compoundpacketlength; }
почему метод неконстантный?
>> void ClearPacketList(); — этот метод вообще не нужен, если сделать контейнер не std::list<RTCPPacket *>, а std::list<SmartPtr<RTCPPacket> >
судя по коду реализации RTCPCompoundPacket можно было бы сделать константным классом — он инициализируется данными в конструкторе и всегда остается неизменным. везде забыты константные идентификаторы, соотв-но вместо GotoFirstPacket(), GetNextPacket() можно было просто выдавать константный референс на контейнер
>>#ifdef RTPDEBUG >> void Dump(); >>#endif // RTPDEBUG <- коммент для дебилов?
>>std::list<RTCPPacket *>::const_iterator
слабо было обявить сразу typedef'ом, чтобы по 100 раз не писать std::list<RTCPPacket *> ?
переходим на С++ файл >>RTCPCompoundPacket::RTCPCompoundPacket(RTPRawPacket &rawpack, RTPMemoryManager *mgr) : RTPMemoryObject(mgr) >>{ >> compoundpacket = 0; >> compoundpacketlength = 0; >> error = 0;
народу бы почитать про списки инициализации
>> error = ParseData(data,datalen); >> if (error < 0) >> return;
суперработа с исключительными ситуациями
rawpack.ZeroData();
он в конструкторе изменяет состояние передаваемого обьекта?????
>> compoundpacket = packet; >> compoundpacketlength = packetlen; >> deletepacket = deletedata;
такие повторяющиеся блоки иногда имеет смысл выделить в структурку. вообще, там три конструктора с дублированием инициализации этой структурки. если кто-нить добавить туда еще что-то, то надо будет не забыть подписать инициализацию о всех конструкторах — неудобно
>> switch (rtcphdr->packettype) >> { >> case RTP_RTCPTYPE_SR: >> p = RTPNew(GetMemoryManager(),RTPMEM_TYPE_CLASS_RTCPSRPACKET) RTCPSRPacket(data,length); >> break; >> case RTP_RTCPTYPE_RR: >> p = RTPNew(GetMemoryManager(),RTPMEM_TYPE_CLASS_RTCPRRPACKET) RTCPRRPacket(data,length); >> .........
это стоило бы выделить в отдельный метод
+ по дурацки построена работа с менеджером памяти. что-то мне подсказывает что в С++ его не нужно тащить в каждый класс
даже простое удаление жутко выглядит >> RTPDelete(*it,GetMemoryManager());
вот забудешь так, удалишь как обычный обьект — и никто ведь не предупредит даже! или выделишь просто через new. потенциальное море граблей.
вывод — код — полное гавно. но оно (скорее всего) рабочее! а, значит, код годный и все замечательно. ну писал его сишник, что ж теперь. главное, чтобы все работало
Здравствуйте, __kot2, Вы писали:
__>вывод — код — полное гавно. но оно (скорее всего) рабочее! а, значит, код годный и все замечательно. ну писал его сишник, что ж теперь. главное, чтобы все работало
Ну, некоторые замечания можно оспорить, конечно Но это даже и не важно. В целом соглашусь с вами — конкретно этот код, несмотря на замечания, рефакторить просто так смысла нет.
Но, вот, например, повторяющихся куска, которых было изначально два, резко понадобилось двадцать. Что будем делать? Продолжать копипаст, или, все-таки, вынесем этот код в одно место? То есть, таки сделаем рефакторинг?
В целом могу подытожить так: любому рефакторингу есть свое время и место.
1. Если на проекте затишье — почему-бы не замутить рефакторинг по, например, вашим замечаниям, в отдельном бранче, стабилизировать его и подлить в транк?
2. Если рынок требует фичи или завтра, или никогда — лепим копипасты, всячески портим себе карму, но успеваем в срок, в надежде на п.1
3. Если новая фича никак не лезет в существующую реализацию — делаем минимально возможный рефакторинг. Тут просто и выхода другого может не быть. Альтернатива — заплатки на заплатки, и, как следствие, время стабилизации умножаем на 3, хрупкость системы еще больше увеличена — следующие фичи уже умножат время дебага на 10. Если, конечно, не работает п.2
Здравствуйте, Ulin, Вы писали: U>Но, вот, например, повторяющихся куска, которых было изначально два, резко понадобилось двадцать. Что будем делать? Продолжать копипаст, или, все-таки, вынесем этот код в одно место? То есть, таки сделаем рефакторинг?
я стараюсь вообще не делать повторов ни кода ни данных. соотв-но в моей части повторов не будет. у других — да хоть 20.
U>1. Если на проекте затишье — почему-бы не замутить рефакторинг по, например, вашим замечаниям, в отдельном бранче, стабилизировать его и подлить в транк?
рефакторинг рождает геморрой. всегда. прямо как рекламный лозунг. особенно если он происходит в общей части кода с каким-то другим проектом. люди, которые "рефакторят" сейчас две используемые мною библиотеки рождают мне только геморрой, они тратят мое время на то, что после каждого апдейта приходится постоянно что-то править. времени уходит обычно день-два. день-два на ровном месте. взамен — ничего. ни багфиксов, ни нужных фия. поубивал бы. меня библиотеки полностью устраивают во всем, кроме того, что они меняются, оставаясь тем же, чем и были. ну и даже если нет, после рефакторинга всегда появляются новые косяки — работа для тестеров.
U>2. Если рынок требует фичи или завтра, или никогда — лепим копипасты, всячески портим себе карму, но успеваем в срок, в надежде на п.1
это вам решать, копипастить код или нет. если он где-то был скопипастен, то вам это делать необязательно.
U>3. Если новая фича никак не лезет в существующую реализацию — делаем минимально возможный рефакторинг. Тут просто и выхода другого может не быть. Альтернатива — заплатки на заплатки, и, как следствие, время стабилизации умножаем на 3, хрупкость системы еще больше увеличена — следующие фичи уже умножат время дебага на 10. Если, конечно, не работает п.2
я делал рефакторинг. тогда, когда не понимал, как проект вообще делать. попробовал одним способом, не получилось, понял, что нужно с другого угла зайти. рефакторинг говорит о том, что проект делают недостаточно компетентные люди. периодический или хронический рефакторинг — проект не будет доделан до нормального состояния никогда.
Здравствуйте, alexsoff, Вы писали:
A>Я создал эту тему, чтобы услышать, что у кого-то был опыт (успешный или не успешный) в переписывании проектов с нуля, а не о том, что у меня низкая квалификация и я вообще должен провалить проект.
Ну а как ты хотел? "О работе" это же не совсем профильный форум, тут не надо быть экспертом, чтобы отвечать. Так что не стоит удивляться ответам в стиле "у тебя кривые руки", хорошо хоть мудаком не назвали. Вон Octothorp-у в недавней теме про МММ повезло меньше. см. тут
Здравствуйте, iiice, Вы писали:
I>Товарищ Маузер, очевидно Вы не в полной мере осознвёте все последствия использования глобальных переменных. Размер программы тут не при чём. А вот портабельность кода убивается на раз — на некоторых платформах глобальность, а порой и синглтоны, технологически недопустимы. Пример — android. И пожалуйста, поосторожнее с поучениями молодёжи.
Если мне придется писать на Андроид, то глобальные переменные не будут самой большой проблеммой.
Там вообще Ява, а я пишу на С++
I>PS. О себе. Вырос, набрался опыта, осознал, ужаснулся, весь опыт выкинул нахрен как вредный, и больше накапливать не буду. Темпы сейчас такие...
Это значит у Вас не тот опыт. Вероятно он у Вас изначально был "низкоуровневый"
Он у вас был уровня "глобальные переменные вредны", а должен быть "не кидайся какашками в коллегу".
Здравствуйте, Erop, Вы писали:
E>Здравствуйте, MxMsk, Вы писали:
MM>>Мне-то как раз ясно, что ты хочешь сделать хорошо.
E>Мне, например, тоже ясно, что он хочет сделать хорошо. Но чем боьше alexsoff описывает своё видинее ситуации, тем боьше я сомневаюсь, что у него получится сделать лучше, а не хуже...
Имея в качестве мотивации усложнение проекта для увеличения порога вхождения можно улучшить проект? Тут изначально позиция ухудшить проект и за счёт этого стать "незаменимым".
Здравствуйте, __kot2, Вы писали:
>>>RTCPCompoundPacket(uint8_t *packet, size_t len, bool deletedata = true, RTPMemoryManager *memmgr = 0); __>так это С++ или С? почему packed голым указателем?
А что уже в С++ отменили голые указатели?
>>почему указатель = 0? хотя бы NULL нужно писать
Хотя бы потому что это по стандарту
>>> int GetCreationError() { return error; } __>о, ужас, они про исключения слышали?
Расскжите это Гуглу
__>что это за паскалевское именование?
CamelCase ничем не хуже других конвенций и человек может писать так как это принято в код-снадарте той конторы где он работает.
__>судя по коду реализации RTCPCompoundPacket можно было бы сделать константным классом — он инициализируется данными в конструкторе и всегда остается неизменным. везде забыты константные идентификаторы, соотв-но вместо GotoFirstPacket(), GetNextPacket() можно было просто выдавать константный референс на контейнер
>>>#ifdef RTPDEBUG >>> void Dump(); >>>#endif // RTPDEBUG <- коммент для дебилов?
Если будут куча вложенных #ifdef, то "дебилами" уже можно назвать кто так не делает.
"For every complex problem, there is a solution that is simple, neat,
and wrong."
Здравствуйте, AndrewJD, Вы писали: AJD>Здравствуйте, __kot2, Вы писали: >>>>RTCPCompoundPacket(uint8_t *packet, size_t len, bool deletedata = true, RTPMemoryManager *memmgr = 0); __>>так это С++ или С? почему packed голым указателем? AJD>А что уже в С++ отменили голые указатели?
вот видите, у вас свое мнение насчет правильного кода. и так со всеми. в чем отличие моего мнения от вашего?
я считаю, что в С++ необходимости в голых указателях, тем более на uchar* нету вообще. для этого есть контейнеры — там внутри и len и проверка индексов (если ее дописать ).
>>>почему указатель = 0? хотя бы NULL нужно писать AJD>Хотя бы потому что это по стандарту
NULL лучше глазами выделяется, сразу видно что это указатель. к тому же проще будет привести к новому стандарту просто заменив все NULL на nullptr, а = 0 придется глазами выглядывать везде
>>>> int GetCreationError() { return error; } __>>о, ужас, они про исключения слышали? AJD>Расскжите это Гуглу
зачем? тем более гугл большой. каждому рассказывать? если обьект может принимать невалидное состояние это всегда гемор. тогда иногда обусловленный чем-то, а иногда так сделали, потому что не понимают, что это гемор.
__>>что это за паскалевское именование? AJD>CamelCase ничем не хуже других конвенций и человек может писать так как это принято в код-снадарте той конторы где он работает.
удобно бывает по именованию идентификатора сразу понимать что это такое. внтурениий парсер в голове так быстрее работает. мое именование — классы — с большой буквы, методы и данные с маленькой.
AJD>Если будут куча вложенных #ifdef, то "дебилами" уже можно назвать кто так не делает.
я думаю коммент написал какой-нить вижуал ассист, потому что человеку бы он в голову никогда не пришел — соотв-ующий ему ifdef всего строчкой выше.
Здравствуйте, Undying, Вы писали:
U>Здравствуйте, alexsoff, Вы писали:
A>>Разве применение паттернов, может превратить мешанину кода в еще большее гОвно,
U>С легкостью. При этом простое дублирование устраняется рефакторингом на раз, т.е. это рутинная, иногда занимающая много времени, но в общем несложная работа. Код с кучей лишних слоев абстракций (в частности ненужных паттернов) рефакторингу поддается очень плохо. Причем если код с дублированием можно переписывать по частям, то код со слоями лишних паттернов обычно нет. Соответственно плохой код без кучи слоев абстракций и паттернов я всегда предпочту плохому коду с паттернами.
Подписываюсь. Рефакторить один большой класс в стиле процедурного программирования намного проще и быстрее, чем ковыряться в туче понятных классов с непонятным смыслом. Неоднократно проходили. Фактически во втором случае скорее всего будет действительно проще и быстрее заново переписать.
A>>неужели ты ставишь мнение таких метров программирования как Фаулер, Эванс,Бэк по структурированию и паттерированию программ в грош? U>Паттерны это инструмент. Инструмент можно применить правильно, тогда он упростит код и вхождение в код в том числе. Можно неправильно, тогда код кардинально усложнится.
Чем сильнее инструмент, тем больше последствий от ошибок и неверного применения.
Здравствуйте, Wolverrum, Вы писали:
W>Здравствуйте, alexsoff, Вы писали:
A>>Разве применение паттернов, может превратить мешанину кода в еще большее гОвно
W>ДА!!! W>Для применения паттернов еще и меру знать надо, которой не имеет большинство девов.
Паттерны сами собой появляются в процессе шлифовки кода. Сама система паттернов как таковая имеет смысл только для структуризации существующих навыков и знаний разработчика, а не целенаправленного "внедрения паттернов".
Здравствуйте, Klatu, Вы писали:
K>Здравствуйте, Олег К., Вы писали:
K>>>Рефакторить или отказаться с этим работать, других вариантов для уважающего себя профи просто нет
ОК>>Виден совет непрофессионала. Код которому 10-20-30 лет и который писала куча людей которые больше не работают в компании ты тоже начнешь сразу переписывать или отказываться с ним работать?
K>Точно, как это я не сообразил. Настоящие "профи" код никогда не рефакторят K>Я всегда переписываю код, который крив. Неважно кто его писал и когда.
Ты представляешь объём работ для переписывания кода, который писала команда людей лет этак 10? И не паттерны внедряла, а выполняла бизнес задачи? И когда полное знание о работе ряда компонент потеряны во времени? Ты думаешь майкрософт очень довольна кодом NT и с огромным удовольствием не выкинула бы нафик её на помойку, переписав на новые рельсы с учётом текущих условий?
A>Имея в качестве мотивации усложнение проекта для увеличения порога вхождения можно улучшить проект? Тут изначально позиция ухудшить проект и за счёт этого стать "незаменимым".
Ну по всякому бывает. Зависит от того, что чел сможет после "вхождения" делать.
Все эмоциональные формулировки не соотвествуют действительному положению вещей и приведены мной исключительно "ради красного словца". За корректными формулировками и неискажённым изложением идей, следует обращаться к их автором или воспользоваться поиском
Здравствуйте, __kot2, Вы писали:
>>>/** Creates an RTCPCompoundPacket instance from the data in \c rawpack, installing a memory manager if specified. */ __>куски кода с такими комментами неудобно комментить — они всегда рождают дополнительные маты при портировании и отладке, так как обычно при портировании ничего сразу не собирается, надо это делать частями потихоньку
откройте для себя #if 0
>>>RTCPCompoundPacket(uint8_t *packet, size_t len, bool deletedata = true, RTPMemoryManager *memmgr = 0); __>так это С++ или С? почему packed голым указателем? почему указатель = 0? хотя бы NULL нужно писать __>составные имена переменных стоило бы для лучшей читабельности разделять подчеркиваниями deletedata -> delete_data
да, в C++ используются raw non-owning указатели
>>> int GetCreationError() { return error; } __>о, ужас, они про исключения слышали? __>получается, можно создать класс имеющий невалидное состоние. зачем? это очень неудобно
исключения могут быть запрещены в требованиях к проекту, такое бывает.
>>> /** Starts the iteration over the individual RTCP packets in the RTCP compound packet. */ >>> void GotoFirstPacket() { rtcppackit = rtcppacklist.begin(); } >>> RTCPPacket *GetNextPacket() { if (rtcppackit == rtcppacklist.end()) return 0; RTCPPacket *p = *rtcppackit; rtcppackit++; return p; }
__>сделали свой интерфейс для итератора. а зачем?
итератор и "итератор STL" это разные вещи.
>>> size_t GetCompoundPacketLength() { return compoundpacketlength; } __>почему метод неконстантный?
потому что он завтра он будет менять члены класса
>>> void ClearPacketList(); — этот метод вообще не нужен, если сделать контейнер не std::list<RTCPPacket *>, а std::list<SmartPtr<RTCPPacket> >
SmartPtr — это какой? shared_ptr? так он тяжелый и время жизни объекта будет недетерминированное
>>>#ifdef RTPDEBUG >>> void Dump(); >>>#endif // RTPDEBUG <- коммент для дебилов?
так в coding style guide написано
>>>std::list<RTCPPacket *>::const_iterator __>слабо было обявить сразу typedef'ом, чтобы по 100 раз не писать std::list<RTCPPacket *> ?
а что, сложно читать?
__>переходим на С++ файл >>>RTCPCompoundPacket::RTCPCompoundPacket(RTPRawPacket &rawpack, RTPMemoryManager *mgr) : RTPMemoryObject(mgr) >>>{ >>> compoundpacket = 0; >>> compoundpacketlength = 0; >>> error = 0; __>народу бы почитать про списки инициализации
вопрос вкуса, в данном случае список инициализации ничего не поменял бы
>>> error = ParseData(data,datalen); >>> if (error < 0) >>> return; __>суперработа с исключительными ситуациями
__> rawpack.ZeroData(); __>он в конструкторе изменяет состояние передаваемого обьекта?????
ну и что? rawpack.GetData() тоже могло бы менять состояние rawpack
>>> compoundpacket = packet; >>> compoundpacketlength = packetlen; >>> deletepacket = deletedata; __>такие повторяющиеся блоки иногда имеет смысл выделить в структурку. вообще, там три конструктора с дублированием инициализации этой структурки. если кто-нить добавить туда еще что-то, то надо будет не забыть подписать инициализацию о всех конструкторах — неудобно
не критично
>>> switch (rtcphdr->packettype) >>> { >>> case RTP_RTCPTYPE_SR: >>> p = RTPNew(GetMemoryManager(),RTPMEM_TYPE_CLASS_RTCPSRPACKET) RTCPSRPacket(data,length); >>> break; >>> case RTP_RTCPTYPE_RR: >>> p = RTPNew(GetMemoryManager(),RTPMEM_TYPE_CLASS_RTCPRRPACKET) RTCPRRPacket(data,length); >>> ......... __>это стоило бы выделить в отдельный метод
+1
__>+ по дурацки построена работа с менеджером памяти. что-то мне подсказывает что в С++ его не нужно тащить в каждый класс __>даже простое удаление жутко выглядит >>> RTPDelete(*it,GetMemoryManager()); __>вот забудешь так, удалишь как обычный обьект — и никто ведь не предупредит даже! или выделишь просто через new. потенциальное море граблей.
delete statement должен быть запрещен вне библиотек, по coding style guide
__>вывод — код — полное гавно. но оно (скорее всего) рабочее! а, значит, код годный и все замечательно. ну писал его сишник, что ж теперь. главное, чтобы все работало