Re[6]: Низкая квалификация работников
От: Abyx Россия  
Дата: 18.11.11 18:37
Оценка:
Здравствуйте, SkyDance, Вы писали:

A>>скажите, вот это — стиль?


SD>Да, стиль. Что именно вас возмущает в приведенном кусочке кода?


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

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

видимо автора научили что в *любом* классе должен быть конструктор и виртуальный деструктор, вот он так и пишет.
он бы и type traits с виртуальным деструктором писал, если бы знал что это такое

и еще меня возмущает #include <string> потому что в этом файле он не используются
In Zen We Trust
Re[13]: Низкая квалификация работников
От: __kot2  
Дата: 19.11.11 04:33
Оценка: -1
Здравствуйте, Abyx, Вы писали:
A>Здравствуйте, __kot2, Вы писали:
>>>>/** Creates an RTCPCompoundPacket instance from the data in \c rawpack, installing a memory manager if specified. */
__>>куски кода с такими комментами неудобно комментить — они всегда рождают дополнительные маты при портировании и отладке, так как обычно при портировании ничего сразу не собирается, надо это делать частями потихоньку
A>откройте для себя #if 0
вот видите, еще одно мнение. но конкретно с вами бы я работать не стал. во-первых вы слишком пафосны. когда я писал, что такие комменты неудобно комментить, я именно имел в виду, что их неудобно комментить, а не то, что я не знаю, как.

>>>>RTCPCompoundPacket(uint8_t *packet, size_t len, bool deletedata = true, RTPMemoryManager *memmgr = 0);

__>>так это С++ или С? почему packed голым указателем? почему указатель = 0? хотя бы NULL нужно писать
__>>составные имена переменных стоило бы для лучшей читабельности разделять подчеркиваниями deletedata -> delete_data
A>да, в C++ используются raw non-owning указатели
для совместимости с С. если что-то не запрещено, то не значит, что это нужно везде использовать, там где можно грамотнее. вы наверное удивитесь, но ездить задом на машине по дороге тоже не запрещено и вы имеете на это полное право.

>>>> int GetCreationError() { return error; }

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

>>>> /** 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; }
__>>сделали свой интерфейс для итератора. а зачем?
A>итератор и "итератор STL" это разные вещи.
зачем использовать свой, если есть stl?

>>>> size_t GetCompoundPacketLength() { return compoundpacketlength; }

__>>почему метод неконстантный?
A>потому что он завтра он будет менять члены класса
завтра метод ВзятьДлиннуПакета станет неконстантным?

>>>> void ClearPacketList(); — этот метод вообще не нужен, если сделать контейнер не std::list<RTCPPacket *>, а std::list<SmartPtr<RTCPPacket> >

A>SmartPtr — это какой? shared_ptr? так он тяжелый и время жизни объекта будет недетерминированное
любой смартпойнтер адаптированный под нужды проекта

>>>>std::list<RTCPPacket *>::const_iterator

__>>слабо было обявить сразу typedef'ом, чтобы по 100 раз не писать std::list<RTCPPacket *> ?
A>а что, сложно читать?
писать длинно

__>>переходим на С++ файл

>>>>RTCPCompoundPacket::RTCPCompoundPacket(RTPRawPacket &rawpack, RTPMemoryManager *mgr) : RTPMemoryObject(mgr)
>>>>{
>>>> compoundpacket = 0;
>>>> compoundpacketlength = 0;
>>>> error = 0;
__>>народу бы почитать про списки инициализации
A>вопрос вкуса, в данном случае список инициализации ничего не поменял бы
"вкус" это и есть часть стиля

__>> rawpack.ZeroData();

__>>он в конструкторе изменяет состояние передаваемого обьекта?????
A>ну и что? rawpack.GetData() тоже могло бы менять состояние rawpack
нажатие педали тормоза в машине тоже может включать правый поворотник. но зачем? код должен преследовать принцип минимальной неожиданности. это часть стиля. у вас скорее всего такого принципа нет. я бы с вами в проекте работать не захотел бы.

>>>> compoundpacket = packet;

>>>> compoundpacketlength = packetlen;
>>>> deletepacket = deletedata;
__>>такие повторяющиеся блоки иногда имеет смысл выделить в структурку. вообще, там три конструктора с дублированием инициализации этой структурки. если кто-нить добавить туда еще что-то, то надо будет не забыть подписать инициализацию о всех конструкторах — неудобно
A>не критично
это классическая копипаста. вы говорите, что избегать ее некритично. еще минус вам в желание с вами работать.

__>>+ по дурацки построена работа с менеджером памяти. что-то мне подсказывает что в С++ его не нужно тащить в каждый класс

__>>даже простое удаление жутко выглядит
>>>> RTPDelete(*it,GetMemoryManager());
__>>вот забудешь так, удалишь как обычный обьект — и никто ведь не предупредит даже! или выделишь просто через new. потенциальное море граблей.
A>delete statement должен быть запрещен вне библиотек, по coding style guide
какой-то странный этот coding style guide. он в сбербанке случаем был разработан? надо же голову включать что разумно, а что нет и для каждого проекта его адаптировать.
Re[14]: Низкая квалификация работников
От: Banned by IT  
Дата: 21.11.11 00:18
Оценка:
Здравствуйте, __kot2, Вы писали:

__>я считаю, что в С++ необходимости в голых указателях, тем более на uchar* нету вообще. для этого есть контейнеры — там внутри и len и проверка индексов (если ее дописать ).

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

>>>>> int GetCreationError() { return error; }

__>>>о, ужас, они про исключения слышали?
AJD>>Расскжите это Гуглу
__>зачем? тем более гугл большой. каждому рассказывать? если обьект может принимать невалидное состояние это всегда гемор. тогда иногда обусловленный чем-то, а иногда так сделали, потому что не понимают, что это гемор.
Есть такая широко распространённая практика как запрет на использование исключений. Весьма разумная между прочим когда применяется там где реально оно того стоит. Поэтому опять таки критика мимо.
... << RSDN@Home 1.1.4 stable SR1 rev. 568>>
Re[14]: Низкая квалификация работников
От: Banned by IT  
Дата: 21.11.11 00:18
Оценка:
Здравствуйте, __kot2, Вы писали:

A>>итератор и "итератор STL" это разные вещи.

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

A>>а что, сложно читать?

__>писать длинно
Это проблемы пишущего.

__>>> rawpack.ZeroData();

__>>>он в конструкторе изменяет состояние передаваемого обьекта?????
A>>ну и что? rawpack.GetData() тоже могло бы менять состояние rawpack
__>нажатие педали тормоза в машине тоже может включать правый поворотник. но зачем? код должен преследовать принцип минимальной неожиданности. это часть стиля. у вас скорее всего такого принципа нет. я бы с вами в проекте работать не захотел бы.
Ну например будет инкрементить какой нить counter внутри, для сбора какой нить статистики или ещё чего.
Пропуск const на самом деле тут показывает что писал код сишник, не ++сник. Ошибка не фатальная, так что нет смысла так тщательно к ней придираться.

A>>delete statement должен быть запрещен вне библиотек, по coding style guide

__>какой-то странный этот coding style guide. он в сбербанке случаем был разработан? надо же голову включать что разумно, а что нет и для каждого проекта его адаптировать.
Полноценно судить о codestyle без знаний всей специфики проекта не представляется возможным.
... << RSDN@Home 1.1.4 stable SR1 rev. 568>>
Re[15]: Низкая квалификация работников
От: __kot2  
Дата: 21.11.11 01:41
Оценка:
Здравствуйте, Banned by IT, Вы писали:
__>>я считаю, что в С++ необходимости в голых указателях, тем более на uchar* нету вообще. для этого есть контейнеры — там внутри и len и проверка индексов (если ее дописать ).
BBI>Ты видишь интерфейс но понятия не имеешь как и откуда его используют. Поэтому судить надо тут голый указатель или нет просто не имеешь права. Вызывающая сторона к примеру вполне может передавать кусок из середины какого либо буфера.
значит нужен интерфейс доступа, а не просто голый указатель

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

BBI>Есть такая широко распространённая практика как запрет на использование исключений. Весьма разумная между прочим когда применяется там где реально оно того стоит. Поэтому опять таки критика мимо.
с этим я согласен. вообще, никогда ни в одном проекте за 11 лет с этим не сталкивался на практике, но да, слышал, что бывает такое.
Re[16]: Низкая квалификация работников
От: Banned by IT  
Дата: 21.11.11 01:45
Оценка:
Здравствуйте, __kot2, Вы писали:

__>>>я считаю, что в С++ необходимости в голых указателях, тем более на uchar* нету вообще. для этого есть контейнеры — там внутри и len и проверка индексов (если ее дописать ).

BBI>>Ты видишь интерфейс но понятия не имеешь как и откуда его используют. Поэтому судить надо тут голый указатель или нет просто не имеешь права. Вызывающая сторона к примеру вполне может передавать кусок из середины какого либо буфера.
__>значит нужен интерфейс доступа, а не просто голый указатель
Например?
... << RSDN@Home 1.1.4 stable SR1 rev. 568>>
Re[15]: Низкая квалификация работников
От: __kot2  
Дата: 21.11.11 01:46
Оценка:
Здравствуйте, Banned by IT, Вы писали:
A>>>итератор и "итератор STL" это разные вещи.
__>>зачем использовать свой, если есть stl?
BBI>Потому как STL суть неподконтрольная сторонняя библиотека, версию которую трудно зафиксировать без волевого манагерского решения и которая уже подкидывала подляны наподобие разных характеристик своей работы в разных версиях.
итератор — наипримитивнейшая вещь. но если stl ной версии не доверяешь, то можно и свой итератор написать. а вот вносить ф-ал итератора в класс это смесь бульдога с носорогом. но с пивом канает.

A>>>а что, сложно читать?

__>>писать длинно
BBI>Это проблемы пишущего.
и модифицируюещго. это тоже вид копипасты. захотел сменить вид контейнера — сиди правь везде ошибки компиляции

__>>>> rawpack.ZeroData();

__>>>>он в конструкторе изменяет состояние передаваемого обьекта?????
A>>>ну и что? rawpack.GetData() тоже могло бы менять состояние rawpack
__>>нажатие педали тормоза в машине тоже может включать правый поворотник. но зачем? код должен преследовать принцип минимальной неожиданности. это часть стиля. у вас скорее всего такого принципа нет. я бы с вами в проекте работать не захотел бы.
BBI>Ну например будет инкрементить какой нить counter внутри, для сбора какой нить статистики или ещё чего.
насколько я помню это решается указанием volatile у counter. или че-то в этом роде. в общем, решение есть

BBI>Пропуск const на самом деле тут показывает что писал код сишник, не ++сник. Ошибка не фатальная, так что нет смысла так тщательно к ней придираться.

согласен. но код-то от этого лучше не становится

A>>>delete statement должен быть запрещен вне библиотек, по coding style guide

__>>какой-то странный этот coding style guide. он в сбербанке случаем был разработан? надо же голову включать что разумно, а что нет и для каждого проекта его адаптировать.
BBI>Полноценно судить о codestyle без знаний всей специфики проекта не представляется возможным.
зато по уровню владения языком того сишника, который этот код писал мне кажется понятно, что он не знает, как кастомные менеджеры памяти в С++ делаются и тут в явном виде городит свой велосипед
Re[17]: Низкая квалификация работников
От: __kot2  
Дата: 21.11.11 01:48
Оценка:
Здравствуйте, Banned by IT, Вы писали:
BBI>Здравствуйте, __kot2, Вы писали:
__>>>>я считаю, что в С++ необходимости в голых указателях, тем более на uchar* нету вообще. для этого есть контейнеры — там внутри и len и проверка индексов (если ее дописать ).
BBI>>>Ты видишь интерфейс но понятия не имеешь как и откуда его используют. Поэтому судить надо тут голый указатель или нет просто не имеешь права. Вызывающая сторона к примеру вполне может передавать кусок из середины какого либо буфера.
__>>значит нужен интерфейс доступа, а не просто голый указатель
BBI>Например?
некий класс имеющий метод или поле length (или size) и имеющий оператор [] или, если нужно, ф-ию которая дает указатель на кусок. возможно, еще что-то умеющий делать. может быть в стиле стрима
Re[18]: Низкая квалификация работников
От: Banned by IT  
Дата: 21.11.11 02:40
Оценка:
Здравствуйте, __kot2, Вы писали:

__>Здравствуйте, Banned by IT, Вы писали:

BBI>>Здравствуйте, __kot2, Вы писали:
__>>>>>я считаю, что в С++ необходимости в голых указателях, тем более на uchar* нету вообще. для этого есть контейнеры — там внутри и len и проверка индексов (если ее дописать ).
BBI>>>>Ты видишь интерфейс но понятия не имеешь как и откуда его используют. Поэтому судить надо тут голый указатель или нет просто не имеешь права. Вызывающая сторона к примеру вполне может передавать кусок из середины какого либо буфера.
__>>>значит нужен интерфейс доступа, а не просто голый указатель
BBI>>Например?
__>некий класс имеющий метод или поле length (или size) и имеющий оператор [] или, если нужно, ф-ию которая дает указатель на кусок. возможно, еще что-то умеющий делать. может быть в стиле стрима
ИМХО это будет уже overkill.
Для публичной либы важно выдать наружу максимально простой и эффективный интерфейс. Максимум что тут можно сделать: wrapper над const void* + size_t. Практической пользы правда от такого будет ноль.
Либа, которая будет заставлять переоборачивать данные или же использовать только свои контейнеры имеет высокие шансы быть отправленной "на помоечку" (тм)
... << RSDN@Home 1.1.4 stable SR1 rev. 568>>
Re[16]: Низкая квалификация работников
От: Banned by IT  
Дата: 21.11.11 02:40
Оценка:
Здравствуйте, __kot2, Вы писали:

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

BBI>>Ну например будет инкрементить какой нить counter внутри, для сбора какой нить статистики или ещё чего.
__>насколько я помню это решается указанием volatile у counter. или че-то в этом роде. в общем, решение есть
mutable, но это костыль в разы покруче отсутствия const. Ибо только запутывает: вроде как функция декларирует что ничего не меняет но всё таки меняет. Уж лучше пусть её const не объявлять, будет куда честнее.
... << RSDN@Home 1.1.4 stable SR1 rev. 568>>
Re[19]: Низкая квалификация работников
От: __kot2  
Дата: 21.11.11 05:59
Оценка:
Здравствуйте, Banned by IT, Вы писали:
BBI>Для публичной либы важно выдать наружу максимально простой и эффективный интерфейс. Максимум что тут можно сделать: wrapper над const void* + size_t. Практической пользы правда от такого будет ноль.
ну если такую цель преследовать, то это итератор над char * + size_t. это если в С++ стиле писать.
конкретно я ничего не имею против кода этой библиотеки. по сравнению со среднестатистическим кодом он прекрасен. самое главное — чтобы код работал. но я хочу показать, что код далеко не идеальный. вообще, я эту тему поднял говоря, что любой хороший код неидеален. конкретно эта библиотека содержит достаточное кол-во косяков, велосипедов и возможных проблем на ровном месте. но она отличная по сути.

BBI>Либа, которая будет заставлять переоборачивать данные или же использовать только свои контейнеры имеет высокие шансы быть отправленной "на помоечку" (тм)

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

RTCPSRPacket::RTCPSRPacket(uint8_t *data,size_t datalength)
: RTCPPacket(SR,data,datalength)
{
knownformat = false;

RTCPCommonHeader *hdr;
size_t len = datalength;
size_t expectedlength;

hdr = (RTCPCommonHeader *)data;
if (hdr->padding) БАБАХ!!!
{
uint8_t padcount = data[datalength-1]; БАБАХ!!
Re[17]: Низкая квалификация работников
От: __kot2  
Дата: 21.11.11 06:01
Оценка:
Здравствуйте, Banned by IT, Вы писали:
__>>>>нажатие педали тормоза в машине тоже может включать правый поворотник. но зачем? код должен преследовать принцип минимальной неожиданности. это часть стиля. у вас скорее всего такого принципа нет. я бы с вами в проекте работать не захотел бы.
BBI>>>Ну например будет инкрементить какой нить counter внутри, для сбора какой нить статистики или ещё чего.
__>>насколько я помню это решается указанием volatile у counter. или че-то в этом роде. в общем, решение есть
BBI>mutable, но это костыль в разы покруче отсутствия const. Ибо только запутывает: вроде как функция декларирует что ничего не меняет но всё таки меняет. Уж лучше пусть её const не объявлять, будет куда честнее.
const пишут те, кто понимает концепцию неизменяемых обьектов. неизменяемые обьекты это очень круто и удобно. но конечно же в команде найдутся те, кто будет "собирать статистику вызовов" или искать другое оправдание не использовать const — вот специально для них ввели mutable
Re[11]: Низкая квалификация работников
От: Aviator  
Дата: 21.11.11 11:34
Оценка: +1
Здравствуйте, Abyx, Вы писали:

A>Здравствуйте, grosborn, Вы писали:


>>> Это зависит только от квалификации. Если у вас не получается сделать лучше — ... (ну вы поняли).


G>>А может быть дело в том, что у вас недостаточно квалификации для поддержки и рефакторинга чужого кода? Не кричать "шеф! все пропало!!!", когда копипастинг можно просто спокойно отрефакторить и вынести в отдельный класс?


A>в том то и дело, что рефакторинг без тестов (и без тестирования вообще) опасен по определению.

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

A>но тут возникает вопрос, не быстрее ли выкинуть код и переписать с нуля?

Для этого и нужен опыт, что бы уметь исправить небольшую часть легаси кода, не поломав при этом всё вокруг. Для этого держат опытных специалистов и платят им хорошие деньги. Новичики обычно не умеют работать с чужим кодом, поэтому предлагаю всё быстро переписать. В итоге быстро не получается, да и вообще не очень получается, ибо всплывает куча деталей, на которые глобально мыслящий инноватор не удосужился обратить внимание. Оценка трудоёмкости и рисков это тоже искусство, которое приходит только с опытом. Нафигачить кучу нового кода несложно, даже не особо сложно написать его аккуратно (хотя даже это далеко не все могут). Действительно сложно суметь удержать код от превращения в помойку в процессе многолетней эволюции требований.
Re[18]: Низкая квалификация работников
От: Banned by IT  
Дата: 21.11.11 12:31
Оценка:
Здравствуйте, __kot2, Вы писали:

BBI>>mutable, но это костыль в разы покруче отсутствия const. Ибо только запутывает: вроде как функция декларирует что ничего не меняет но всё таки меняет. Уж лучше пусть её const не объявлять, будет куда честнее.

__>const пишут те, кто понимает концепцию неизменяемых обьектов. неизменяемые обьекты это очень круто и удобно. но конечно же в команде найдутся те, кто будет "собирать статистику вызовов" или искать другое оправдание не использовать const — вот специально для них ввели mutable
Нет. mutable в таком контексте использовать — себе дороже.
Его вообще крайне редко следует использовать, примерно как goto — только если без него ещё хуже получается.
... << RSDN@Home 1.1.4 stable SR1 rev. 568>>
Re[19]: Низкая квалификация работников
От: __kot2  
Дата: 21.11.11 13:32
Оценка:
Здравствуйте, Banned by IT, Вы писали:
BBI>Нет. mutable в таком контексте использовать — себе дороже.
BBI>Его вообще крайне редко следует использовать, примерно как goto — только если без него ещё хуже получается.
я никогда в жизни не использовал mutable
но и методы которые по логике вещей не должны менять состояние обьектая всегда делаю const и никогда в жизни не было таких проблем, что "однажды оно может стать неконстантным". это кажется таким же бредом, как если бы кто-то писал код так, что однажды плюс может стать минусом
Re[16]: Низкая квалификация работников
От: SkyDance Земля  
Дата: 21.11.11 22:01
Оценка:
__>насколько я помню это решается указанием volatile у counter. или че-то в этом роде. в общем, решение есть

не volatile, a mutable. С volatile аккуратнее там, очень спорная штука в С++.
Re[17]: Низкая квалификация работников
От: __kot2  
Дата: 22.11.11 06:34
Оценка:
Здравствуйте, SkyDance, Вы писали:
__>>насколько я помню это решается указанием volatile у counter. или че-то в этом роде. в общем, решение есть
SD>не volatile, a mutable. С volatile аккуратнее там, очень спорная штука в С++.
вообще, я бы не стал включать mutable в язык. если кому-то нужна такая возможность, то кто мешает конст кастом откастить this и спокойно менять хоть таблицу виртуальных методов? это ничуть не хуже будет смотреться
Re[18]: Низкая квалификация работников
От: SkyDance Земля  
Дата: 22.11.11 22:49
Оценка:
__>вообще, я бы не стал включать mutable в язык. если кому-то нужна такая возможность, то кто мешает конст кастом откастить this и спокойно менять хоть таблицу виртуальных методов? это ничуть не хуже будет смотреться

Не-не-не. Когда применяется какая-то "магия", там сразу и не видно, зачем все эти "касты" и прочая лабуда.
Для того и нужны keywords, чтобы explicitly показать — "вот они, грабли" (С) Большинство программистов при виде необычных keywords (тех же mutable, volatile, static) сразу напрягаются и начинают быть более внимательными. А при виде г-кода просто вздыхают "ну вот опять", и не уделяют ему дОлжного внимания.
Re[19]: Низкая квалификация работников
От: __kot2  
Дата: 23.11.11 04:21
Оценка:
Здравствуйте, SkyDance, Вы писали:

__>>вообще, я бы не стал включать mutable в язык. если кому-то нужна такая возможность, то кто мешает конст кастом откастить this и спокойно менять хоть таблицу виртуальных методов? это ничуть не хуже будет смотреться


SD>Не-не-не. Когда применяется какая-то "магия", там сразу и не видно, зачем все эти "касты" и прочая лабуда.

SD>Для того и нужны keywords, чтобы explicitly показать — "вот они, грабли" (С) Большинство программистов при виде необычных keywords (тех же mutable, volatile, static) сразу напрягаются и начинают быть более внимательными. А при виде г-кода просто вздыхают "ну вот опять", и не уделяют ему дОлжного внимания.
в общем-то да, но коммент в стиле
************************************************************
*********** ЭТО WORKAROUND ДЛЯ БАГА GCC — 78642!!! *********
********** НЕ ТРОГАТЬ!! ***********************************
************************************************************

обходятся без keywords и заметнее
Re[8]: Низкая квалификация работников
От: uncommon Ниоткуда  
Дата: 24.11.11 06:44
Оценка:
Здравствуйте, __kot2, Вы писали:

__>Здравствуйте, Ulin, Вы писали:

U>>То есть нечто, больше относящееся к форматированию. Имена классов типа "BinRes", "a", "b", "ptr" и тп. это в моем понимании просто кривой код, подлежащий рефакторингу.
__>рефакторинг это пустая трата времени. это лишь адаптация одного стиля под другой. если программа работает, значит она выполняет свои ф-ии и не надо в нее лезть своими ручками. если настолько кривая, что не работает, то ее и не существует.

Как тебе такой код? Тоже ведь работает. Зачем его рефакторить? Такой миленький кодик...

HRESULT LoadIcons(
    UINT cVals,                 // In: Count of property values
    const CEPROPVAL* pVals,     // In: Property values.
    DWORD dwFlags               // In: flags that control how we load the icons
)
{
    UNREFERENCED_PARAMETER(cVals);
    HRESULT hr = S_OK;
    DWORD dwPhoneFeatureLevel = 0;
    DWORD dwMLFeatureLevel = 0;
    DWORD dwVoIPFeatureLevel = 0;

    SetRssiBitmapFunctions();

    ASSERT(pVals);
    ASSERT(cVals == ipropLast);

    static const UINT c_AllVMFlags = (NSF_NEW_VM_LINE1 | NSF_NEW_VM_LINE2 | NSF_NEW_VM_VOIP | _NSF_GSM_VMAIL_ONELINE);

    CHRA(GetDeviceFeatureLevel(DFLI_ML_EX, &dwMLFeatureLevel));
    CHRA(GetDeviceFeatureLevel(DFLI_PHONE, &dwPhoneFeatureLevel));
    CHRA(GetDeviceFeatureLevel(DFLI_VOIP , &dwVoIPFeatureLevel));

    // Get property values.
    UINT uPhoneStatus, uSignalStrength, cActiveCalls, uInputState, uBatteryStr, uBatteryFlags, uNotify, uSynchronize, uBluetooth, uSpeakerphone, uSignalStrengthRaw, uCellSystemAvailable, uCellSystemConnected, uWiFi, uPhoneStatusEx;
    uPhoneStatus = (pVals[ipropPhoneStatus].wFlags & CEDB_PROPNOTFOUND) ? 0 : pVals[ipropPhoneStatus].val.ulVal;
    uBatteryStr = (pVals[ipropBattery].wFlags & CEDB_PROPNOTFOUND) ? 0 : BATTERY_STRENGTH(pVals[ipropBattery].val.ulVal);
    uBatteryFlags = (pVals[ipropBattery].wFlags & CEDB_PROPNOTFOUND) ? 0 : BATTERY_FLAGS(pVals[ipropBattery].val.ulVal);
    uSignalStrength = (pVals[ipropSignalStrength].wFlags & CEDB_PROPNOTFOUND) ? 0 :pVals[ipropSignalStrength].val.ulVal;
    uSignalStrengthRaw = (pVals[ipropSignalStrengthRaw].wFlags & CEDB_PROPNOTFOUND) ? 0 :pVals[ipropSignalStrengthRaw].val.ulVal;
    cActiveCalls = (pVals[ipropIconCallCount].wFlags & CEDB_PROPNOTFOUND) ? 0 :pVals[ipropIconCallCount].val.ulVal;
    uInputState = (pVals[ipropInputState].wFlags & CEDB_PROPNOTFOUND) ? 0 : pVals[ipropInputState].val.ulVal;
    uNotify = (pVals[ipropNotify].wFlags & CEDB_PROPNOTFOUND) ? 0 : pVals[ipropNotify].val.ulVal;
    uSynchronize = (pVals[ipropSynchronize].wFlags & CEDB_PROPNOTFOUND) ? 0 : pVals[ipropSynchronize].val.ulVal;
    uBluetooth = (pVals[ipropBluetooth].wFlags & CEDB_PROPNOTFOUND) ? 0 : pVals[ipropBluetooth].val.ulVal;
    uSpeakerphone = (pVals[ipropSpeakerphone].wFlags & CEDB_PROPNOTFOUND) ? 0 : pVals[ipropSpeakerphone].val.ulVal;
    uCellSystemAvailable = (pVals[ipropCellSystemAvailable].wFlags & CEDB_PROPNOTFOUND) ? 0 : pVals[ipropCellSystemAvailable].val.ulVal;
    uCellSystemConnected = (pVals[ipropCellSystemConnected].wFlags & CEDB_PROPNOTFOUND) ? 0 : pVals[ipropCellSystemConnected].val.ulVal;
    uWiFi = (pVals[ipropWiFi].wFlags & CEDB_PROPNOTFOUND) ? 0 : pVals[ipropWiFi].val.ulVal;
    uPhoneStatusEx = (pVals[ipropPhoneStatusEx].wFlags & CEDB_PROPNOTFOUND) ? 0 : pVals[ipropPhoneStatusEx].val.ulVal;

    //If we are in data call assume that connected flag is set
    ASSERT( ((uPhoneStatus & PSF_DATA_CALL) && uCellSystemConnected) || (!(uPhoneStatus & PSF_DATA_CALL) && !uCellSystemConnected) );
    ASSERT( ((uPhoneStatus & PSF_GPRS_CONNECTION) && 
            (uCellSystemConnected & (CSCF_GPRS | CSCF_EDGE | CSCF_HSDPA | CSCF_UMTS))) 
            || (!(uPhoneStatus & PSF_GPRS_CONNECTION) && 
            !(uCellSystemConnected & ( CSCF_GPRS | CSCF_EDGE | CSCF_HSDPA | CSCF_UMTS))) 
          );
    
//...

        BOOL fCaseHit;
        fCaseHit = ((pc->uPhoneStatus == NA || (pc->uPhoneStatus != NA && ((uPhoneStatus & pc->uPhoneStatus) == pc->uPhoneStatus))) &&
                    (pc->uPhoneStatusEx == NA || (pc->uPhoneStatusEx != NA && ((uPhoneStatusEx & pc->uPhoneStatusEx) == pc->uPhoneStatusEx))) &&
                    (pc->cActiveCalls == NA || (pc->cActiveCalls != NA && NULL != pfncCallCompare && pfncCallCompare(cActiveCalls, pc->cActiveCalls))) &&
                    (pc->uBatteryStr == NA || (pc->uBatteryStr != NA && uBatteryStr >= pc->uBatteryStr)) &&
                    (pc->uBatteryFlags == NA || (pc->uBatteryFlags != NA && ((uBatteryFlags & pc->uBatteryFlags) == pc->uBatteryFlags))) &&
                    (pc->uSignalStr == NA || (pc->uSignalStr != NA && uSignalStrength >= pc->uSignalStr)) &&
                    (pc->uInputState == NA || (pc->uInputState != NA && ((uInputState & pc->uInputState) == pc->uInputState))) &&
                    (pc->uNotify == NA || (pc->uNotify != NA && ((uNotify & pc->uNotify) == pc->uNotify))) &&
                    (pc->uSynchronize == NA || (pc->uSynchronize != NA && ((uSynchronize & pc->uSynchronize) == pc->uSynchronize))) &&
                    (pc->uBluetooth == NA || (pc->uBluetooth != NA && ((uBluetooth & pc->uBluetooth) == pc->uBluetooth))) &&
                    (pc->uCellSystemAvailable == NA || (pc->uCellSystemAvailable != NA && ((uCellSystemAvailable & pc->uCellSystemAvailable) == pc->uCellSystemAvailable))) &&
                    (pc->uCellSystemConnected == NA || (pc->uCellSystemConnected != NA && ((uCellSystemConnected & pc->uCellSystemConnected) == pc->uCellSystemConnected))) &&
                    (pc->uWiFi == NA || (pc->uWiFi != NA && ((uWiFi & pc->uWiFi) == pc->uWiFi)))
                   );

        ASSERT(pc->iiDestGrp < m_cIconsMax && pc->iiDestGrp >= 0
            && pc->iiDefault < m_cIconsMax && pc->iiDefault >= (int)NA); // Invalid index in table

        if (fCaseHit)
        {
            if ((dwFlags & SSDDF_BLINK_OFF) && pc->fBlink)
            {
                m_rgiIcons[pc->iiDestGrp] = pc->iBmpDefault;
            }
            else if (pc->iBmpSet == iBmpSHNotif)
            {
                if (m_iSHNotifIcon[pc->iBmpDefault] >= 0)
                {
                    // Put index of m_hilSHNotifIcons to HIWORD.
                    m_rgiIcons[pc->iiDestGrp] = (int)MAKELONG(pc->iBmpSet, m_iSHNotifIcon[pc->iBmpDefault]);
                }
            }
            else
            {
                if (pc->iBmpSet == iBmpCustom)
                {
                    // HIWORD(uInputState) is the index into m_hilCustomIcons of the icon to be displayed
                    // Put the index of m_hilCustomIcons to HIWORD.
                    m_rgiIcons[pc->iiDestGrp] = (int)MAKELONG(pc->iBmpSet, HIWORD(uInputState));
                }
                else
                {
                    m_rgiIcons[pc->iiDestGrp] = pc->iBmpSet;
                }
            }
        }

// and so on and so forth
Подождите ...
Wait...
Пока на собственное сообщение не было ответов, его можно удалить.