1. C++ / Говнокод #7742

    +164

    1. 01
    2. 02
    3. 03
    4. 04
    5. 05
    6. 06
    7. 07
    8. 08
    9. 09
    10. 10
    11. 11
    12. 12
    13. 13
    14. 14
    15. 15
    16. 16
    17. 17
    18. 18
    19. 19
    20. 20
    21. 21
    22. 22
    23. 23
    24. 24
    25. 25
    26. 26
    27. 27
    28. 28
    29. 29
    30. 30
    31. 31
    32. 32
    33. 33
    34. 34
    35. 35
    36. 36
    37. 37
    38. 38
    39. 39
    40. 40
    41. 41
    42. 42
    43. 43
    ULONG LCard791::SetChn(int _gain,int _channel)
    {
    		ULONG ret;
    		if(isDiff)
    				ret=_channel&15;
    		else
    		{
    				ret=_channel&31;
    				ret|=1<<5;
    		}
    		int gain;
    		switch(_gain)
    		{
    		case 1:
    				gain=0;
    				break;
    		case 2:
    				gain=1;
    				break;
    		case 4:
    				gain=2;
    				break;
    		case 8:
    				gain=3;
    				break;
    		case 16:
    				gain=4;
    				break;
    		case 32:
    				gain=5;
    				break;
    		case 64:
    				gain=6;
    				break;
    		case 128:
    				gain=7;
    				break;
    		default:
    				gain=0;
    		}
    		ret|=gain<<6;
    		return(ret);
    }

    Есть у нас один мужик, которые такие шедевры творит. Хакер сновидений, РАГ - мы с тобой!

    Запостил: phys-tech, 02 Сентября 2011

    Комментарии (55) RSS

    • Настоящий программист может писать ассемблерный код на любом языке.
      Ответить
    • оууу...
      Ответить
    • А вот теперь представьте что такого кода у него 1 400 000 строк в программе, начальство сначала хотело чтобы мы с ней разобрались но в итоге пришлось писать все с нуля.

      P.S. работаю с phys-tech в одной компании
      Ответить
      • Вам положена корпоративная скидка на ГК.
        Ответить
      • т.е. уже три сотрудника одной фирмы ... один говнокодит, два других ржут :)
        Ответить
    • Роман что такое ГК ?Говно код? к счастью этого говнокодера больше нет в нашей компании.Теперь он хакер сноведений, ему не до этого. Кстати нас 3е кодеров тут))
      Ответить
    • А как вы этот кейс замените? Циклом некрасиво, а bsr в языке нету, только инлайноасмофункцию писать.
      Ответить
      • log
        Ответить
      • Math::Log2( (float) _gain ) . :-)
        Ответить
        • а как же default ветка?
          можно сделать if (gain_ != 1 << gain) gain = 0;
          и еще log не сможет 0 обработать.
          еще вариант цикл со сдвигом вправо на 1
          Ответить
    • Говна тут не много и оно в форме, а не в содержании.
      Если это функция из 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 - лиспер (каламбур?) и у него чувство декомпозиции, возможно, развито лучше.
          Да, код некрасив, название не соответствует содержанию, но воспроизвести поведение этой функции в точности меньшим числом строк довольно сложно. Я бы тоже вынес часть функционала в маленькие функции, предоставляющие хоть какую-то абстракцию.
          Ответить
          • > в точности меньшим числом строк довольно сложно

            Почему бы не использовать цикл для вычисления логарифма по основанию 2. Быстро и сердито:

            gain = 0;
            while( _gain )
            {
                _gain >>= 1;
                ++gain;
            }
            Ответить
            • Это не в точности тоже поведение. _gain имеет тип int, и у него могут быть не занулены разряды выше 7. switch не обнуляет gain только если _gain - точная степень двойки от 1 до 7, а в цикл можно передать любое число.
              Ответить
              • Согласен. А так:

                int local_gain = _gain & 0x0000007F;
                ...
                gain = 0;
                while( local_gain )
                {
                    local_gain >>= 1;
                    ++gain;
                }
                Ответить
                • а еще _gain может быть 23, например
                  Ответить
                • _gain = 112. Ваш код и код в посте дадут разные результаты.
                  Ответить
                • Те же самые грабли. И маска не та. Для _gain = 128 Ваш код вернет 0.
                  Ответить
                  • int local_gain = _gain & 0x000000FF;
                     ...
                    gain = 0;
                    while( _gain && !(_gain & 1) )
                    {
                       local_gain >>= 1;
                        ++gain;
                    }
                    
                    if( local_gain > 1 && gain )
                       gain = 0;


                    Код "навырост". Можно маской регулировать кол-во возможных значений логарифма.
                    Ответить
                    • Теперь этот код примерно приравнялся по числу строк к исходному и стал в несколько раз сложней для чтения и понимания.
                      Ответить
                    • А сейчас неудачное условие для while(). Скорее всего опечатка.
                      Вот вроде эквивалентный код. Приятной медитации.
                      long    calculate_gain_new (int gain){
                          if((gain <= 0) || (gain >= (1<<8))){
                              return 0;
                          }else if ((~gain ^ (gain - 1)) & gain){
                              return 0;
                          }else{
                              int ret = 0;
                              for(int i = (1<<4); i != 0; i>>=1){
                                  if(gain>>i){
                                      ret += i;
                                      gain >>= i;
                                  }
                              }
                              return ret;
                          }
                      }
                      Ответить
                      • Где тут опечатка?
                        Ответить
                        • Условие внутри while() зависит только он _gain. Но сама _gain в теле цикла не меняется. Т.е. Если _gain = 2, то получаем while(true){}.
                          Ответить
                          • Да, конечно, имелось ввиду local_gain

                            // говно-комментатор
                            Ответить
                    • int gain = 7;
                      while( gain && _gain != ( 1 << gain ) )
                          --gain;
                      Ответить
          • "Ну, положим, функции должны быть маленькими..."
            но они не должны. Стороннему человеку, чтобы понять логику работы, легче будет прочитать одну функцию, чем кучу инлайновых.

            Можно вот так сократить строки:
            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;
            }

            Не хочу сказать что читабельность улучшилась, но все же
            Ответить
            • Вы забываете, что у функции есть такой атрибут, как название. Правильно подобранное название функции говорит о том, что она делает.
              Кстати, название обсуждаемого метода неудачно.
              Ответить
            • А теперь сравните с таким вариантом:
              ULONG LCard791::SetChn(int _gain,int _channel)
              {
                  return calcChannel(_channel) | calcGain(_gain);
              }
              
              ULONG LCard791::calcChannel(int _channel)
              {
                  return isDiff ? (_channel & 15) : (_channel & 31 | 32)
              }
              
              ULONG LCard791::calcGain(int _gain)
              {
                  ULONG newGain = 0;
                  switch(_gain) {
                      case 2:   newGain = 1; break;
                      case 4:   newGain = 2; break;
                      case 8:   newGain = 3: break;
                      case 16:  newGain = 4: break;
                      case 32:  newGain = 5: break;
                      case 64:  newGain = 6: break;
                      case 128: newGain = 7: break;
                  }
                  return newGain << 6;
              }
              Разумеется, функциям нужно дать более подходящие имена.
              Ответить
              • упс, вместо двоеточий перед break, разумеется, нужны точки с запятой.
                Ответить
              • опять же, не понимаю зачем тратиться на вызов функции, которая выполняет десяток ассемблерных команд. И скрывать-абстрагировать одну строчку.
                Ответить
                • Это и называется преждевременной оптимизацией. Вызовы функций относительно дешёвы, тем более, по заверению автора, код вызывается один раз за всё время работы приложения, поэтому вопрос производительности тут не стоит. Различные части результата SetChn формируются совершенно по-разному, поэтому логично их вынести в отдельные функции. Согласитесь, мой код читается легче и последовательней, чем ваш. К тому же, поведение различных функций проще протестировать.
                  Ответить
                  • да, ваш читается легче, хотя бы из-за switch) Я же переписал только чтоб сократить.
                    Дешево, да, но если каждую пару логических операций засовывать в функцию - этих функций будет куча и это скажется на производительности.
                    И тестировать одинаково легко, разве ошибешься с последним сдвигом и маски наползут друг на друга
                    Ответить
            • Я работал в проекте на C, в котором были функции по 500-800 строк, и так примерно 500.000 строк кода. Поверьте, разбираться в этом было очень уныло.
              Ответить
              • я же не говорю, что длинные функции - это хорошо, но если умещается на одном экране (а ваш вариант и на половине) зачем разбивать?
                Ответить
                • На самом деле это больше дело вкуса и привычки. Мне кажется, здесь имеет смысл разбить код на отдельные функции. Вы считаете иначе. Спорить об этом особого смысла нет, код то писать не нам. Да и кусок кода не такой важный, чтобы о нём спорить.
                  Ответить
    • no painin the ass no _gain
      Ответить
    • И почему-то никто не заметил, что делать брать по маске 31, а потом устанавливать 5-й бит тоже не очень умно. Строки 3-10 можно сократить:
      ULONG ret=_channel&15;
                      if(!isDiff)
                                      ret|=1<<5;

      Строки 11-40 тоже сокращаются к трём.
      Ответить
      • Для isDiff = false и _channel = 16 оригинальный код даст 48, а Ваш - 32.
        Ответить
    • ULONG Diff(int _channel)
      {
      	ULONG ret = _channel & 0xf;
      	
      	if(!isDiff)
      		ret |= (_channel & 0x10) | 0x20;
      
      	return ret;
      }
      
      unsigned int power_of_two(unsigned long number)
      {
      	/* <number> must be a power of 2 */
      	assert( !(number & (number - 1)) );
      	
      	unsigned int power = 0;
      	while(number >>= 1)
      		++power;
      		
      	return power;
      }
      
      ULONG SetChn(int _gain, int _channel)
      {
      	return Diff(_channel) | (power_of_two(_gain & 0xff) << 6);
      }
      Ответить
    • ну надо же, я даже и не ожидал, что это вызовет такое бурление говн. Молодцы! :-)
      Ответить

    Добавить комментарий