Нашли или выдавили из себя код, который нельзя назвать нормальным,
на который без улыбки не взглянешь?
Не торопитесь его удалять или рефакторить, — запостите его на
говнокод.ру, посмеёмся вместе!
А вот теперь представьте что такого кода у него 1 400 000 строк в программе, начальство сначала хотело чтобы мы с ней разобрались но в итоге пришлось писать все с нуля.
Роман что такое ГК ?Говно код? к счастью этого говнокодера больше нет в нашей компании.Теперь он хакер сноведений, ему не до этого. Кстати нас 3е кодеров тут))
Говна тут не много и оно в форме, а не в содержании.
Если это функция из namespace'а, то мне не нравится глобальный isDiff, хотя скорее всего это метод класса. Лучше было бы сделать три функции: первая - из if'а, вторая - из case'а, третья зовет первые две и "лепит" возвращаемое значение, а сам метод зовет третью функцию.
По всему автор кода - неплохой кодер, а вот тот, кто запостил, со своими корешами насрут немало.
Esper, вам с автором надо в одну команду. Он тоже обожает лепить 100 функций, каждая из которых делает наитупейшую операцию, да еще и вызывается единожды за всю программу. Таким образом, выставление усиления у платы, которое выполняется один раз в начале работы, заняло бы у вас 3 бесполезных функции, одна другой краше! Браво, плюсую!
Эта функция не выставляет усиление, а "собирает" значение, которое потом запишет в какой-то аппаратный регистр. Значение состоит из двух битовых полей, каждое из которых вычисляется по-своему. Схема с тремя функциями отражает dataflow этих вычислений. В простой мелкой функции труднее ошибиться и ее легче отлаживать. Да и static никто не отменял.
Ваша сентенция про разницу в отношении к большому числу простых функций лишь подтверждает мой предыдущий вывод.
это называется оверинжиниринг. Эта функция легко умещается на одном экране и понять что она делает совсем не сложно. Но с вашими тремя функциями и одним методом суть будет размазана - нужно будет "докапываться" до логики, собственно, формирования битовых полей.
static ULONG compute_channel_field(int _ch, bool is_diff){
// код из if'а
}
static ULONG compute_gain_field(int _gain){
// код из case'а. Ниже его уже переписывают. Пока - безуспешно :)
}
ULONG compute_amplification(int _ch, bool is_diff, int _gain){
return compute_channel_field(_ch, is_diff) | (compute_gain_field(_gain) << 6);
}
ULONG LCard791::SetChn(int _gain,int _channel)
{ // А этому вообще можно стать inline'ом
return compute_amplification(_channel, isDiff, _gain);
}
Это не оверинжиниринг. Вы не знаете, что такое оверинжиниринг. Да и я его, к счастью, только на картинках видал.
хорошо, может это не оверинжиниринг, но точно ненужное усложнение кода. Я, конечно, понимаю, что декомпозиция функций хорошо выглядит с функциональной точки зрения, но конкретную логику тяжелей рассмотреть. Абстрагирование - зачем здесь? Переписывать части исходной функции не сложнее, чем нескольких ваших. И вызовы - наоборот, инлайновыми все, кроме метода, зачем тратить ресурсы на вызовы из одного места только это ж эмбеддед.
Ну, положим, функции должны быть маленькими (3-7 строк) и делать примитивные операции.
К тому же, @Esper - лиспер (каламбур?) и у него чувство декомпозиции, возможно, развито лучше.
Да, код некрасив, название не соответствует содержанию, но воспроизвести поведение этой функции в точности меньшим числом строк довольно сложно. Я бы тоже вынес часть функционала в маленькие функции, предоставляющие хоть какую-то абстракцию.
Это не в точности тоже поведение. _gain имеет тип int, и у него могут быть не занулены разряды выше 7. switch не обнуляет gain только если _gain - точная степень двойки от 1 до 7, а в цикл можно передать любое число.
"Ну, положим, функции должны быть маленькими..."
но они не должны. Стороннему человеку, чтобы понять логику работы, легче будет прочитать одну функцию, чем кучу инлайновых.
Можно вот так сократить строки:
ULONG LCard791::SetChn(int _gain,int _channel)
{
ULONG ret;
if(isDiff)
ret=_channel&15;
else
{
ret=_channel&31;
ret|=1<<5;
}
if (_gain == 2)
return ret | 1<<6;
else if (_gain == 4)
return ret | 2<<6;
else if (_gain == 8)
return ret | 3<<6;
else if (_gain == 16)
return ret | 4<<6;
else if (_gain == 32)
return ret | 5<<6;
else if (_gain == 64)
return ret | 6<<6;
else if (_gain == 128)
return ret | 7<<6;
else
return ret;
}
Не хочу сказать что читабельность улучшилась, но все же
Вы забываете, что у функции есть такой атрибут, как название. Правильно подобранное название функции говорит о том, что она делает.
Кстати, название обсуждаемого метода неудачно.
Это и называется преждевременной оптимизацией. Вызовы функций относительно дешёвы, тем более, по заверению автора, код вызывается один раз за всё время работы приложения, поэтому вопрос производительности тут не стоит. Различные части результата SetChn формируются совершенно по-разному, поэтому логично их вынести в отдельные функции. Согласитесь, мой код читается легче и последовательней, чем ваш. К тому же, поведение различных функций проще протестировать.
да, ваш читается легче, хотя бы из-за switch) Я же переписал только чтоб сократить.
Дешево, да, но если каждую пару логических операций засовывать в функцию - этих функций будет куча и это скажется на производительности.
И тестировать одинаково легко, разве ошибешься с последним сдвигом и маски наползут друг на друга
На самом деле это больше дело вкуса и привычки. Мне кажется, здесь имеет смысл разбить код на отдельные функции. Вы считаете иначе. Спорить об этом особого смысла нет, код то писать не нам. Да и кусок кода не такой важный, чтобы о нём спорить.
P.S. работаю с phys-tech в одной компании
можно сделать if (gain_ != 1 << gain) gain = 0;
и еще log не сможет 0 обработать.
еще вариант цикл со сдвигом вправо на 1
Если это функция из namespace'а, то мне не нравится глобальный isDiff, хотя скорее всего это метод класса. Лучше было бы сделать три функции: первая - из if'а, вторая - из case'а, третья зовет первые две и "лепит" возвращаемое значение, а сам метод зовет третью функцию.
По всему автор кода - неплохой кодер, а вот тот, кто запостил, со своими корешами насрут немало.
Ваша сентенция про разницу в отношении к большому числу простых функций лишь подтверждает мой предыдущий вывод.
К тому же, @Esper - лиспер (каламбур?) и у него чувство декомпозиции, возможно, развито лучше.
Да, код некрасив, название не соответствует содержанию, но воспроизвести поведение этой функции в точности меньшим числом строк довольно сложно. Я бы тоже вынес часть функционала в маленькие функции, предоставляющие хоть какую-то абстракцию.
Почему бы не использовать цикл для вычисления логарифма по основанию 2. Быстро и сердито:
Код "навырост". Можно маской регулировать кол-во возможных значений логарифма.
Вот вроде эквивалентный код. Приятной медитации.
// говно-комментатор
но они не должны. Стороннему человеку, чтобы понять логику работы, легче будет прочитать одну функцию, чем кучу инлайновых.
Можно вот так сократить строки:
Не хочу сказать что читабельность улучшилась, но все же
Кстати, название обсуждаемого метода неудачно.
Разумеется, функциям нужно дать более подходящие имена.
Дешево, да, но если каждую пару логических операций засовывать в функцию - этих функций будет куча и это скажется на производительности.
И тестировать одинаково легко, разве ошибешься с последним сдвигом и маски наползут друг на друга
Строки 11-40 тоже сокращаются к трём.