1. Си / Говнокод #2039

    +132.3

    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
    char *uart_readln_timeout(UART_HANDLE h, char *buf, uint16_t len, clock_tick_t to)
    {
        clock_tick_t finish_time;
     
        char *datap = buf;
        char *datae = buf + len - 1;
    
        if( len == 0 ) return buf;
        if( len == 1 ) {
            buf[0] = 0;
            return buf;
        }
    
        finish_time = clock_get_millis() + to;
    
        // FIXME!!!
        while( datap < datae && ( to == 0 || clock_get_millis() < finish_time ) ) {
            if( uart_read_char(h, datap) ) {
                if( *datap++ == '\n' ) break;
            }
        }
        *datap = 0;
    	return buf;
    }

    читает строку из UART. есть подозрение, что это говнокод.

    Запостил: dmzlj.livejournal.com, 24 Октября 2009

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

    • И что плохого?

      Хотя... контроля длины строки нет...
      Ответить
      • контроль длины как раз есть. есть переполнение таймера, но есть подозрение, что есть что-то еще.
        Ответить
    • ага давайте сюда ещё весь подозрительный код, в котором самому не разобраться постить) в каментах же объяснят что к чему, фигле. не говнокод, а фёрстстепс какой-то пл*
      Ответить
    • Автор, ты сам читал из UART? Не пости код, в котором не уверен.
      ================================
      Заметил вот что вызов clock_get_millis() в while, хотя судя по названию функции, быть может так и надо.

      Так вроде ничего не заметил, минусую.
      Ответить
    • Код - отстой:
      1) В условных выражениях константы должны быть слева (тут же: to == 0 и т.д.).
      2) манипуляции с datap, datae и создание finish_time происходят слишком рано. Должны быть после первых двух if'ов, а то напрасная трата ресурсов.
      3) datap, datae - ваще говорящие имена.
      4) buf не проверяется на 0, а надо бы.
      5) if( *datap++ == '\n' ) break; - экономисты, блин, строк кода. Нечитаемая конструкция.

      Нормально так заметил, плюсую.
      Ответить
      • Вы, видимо, никогда не писали для embedded.
        1) Это чисто ваши личные причуды. На сгенерированный код никак не влияет.
        2) Насчет datap и datae согласен, они инициализируются слишком рано. А про finish_time не забывайте, что это C, а не C++. К тому же, если все переменные создаются в начале функции, то место в стеке выделяется, грубо говоря, одной инструкцией, а если в разных местах - то уже несколькими.
        3) Нормальные имена, опять же, это ваши личные причуды.
        4) Проверять указатель на 0? В embedded? Уже смешно. Ну допустим, а что вы сделаете, если он таки ноль?
        5) Нормальная компактная конструкция, а вы учитесь читать.
        Ответить
        • Интереснее, почему его надо проверять на ноль, а на 0x666 например, не надо.
          Ответить
          • Да, согласен. Это я по привычке С ++. Надо на NULL проверять.
            Ответить
          • На 0x666 тоже нада. Если в буфере оказалось такое число, то пользователю нада сообщий что по уарту к нему пришел сам дьявол ))) бухахахааа
            Ответить
            • Да не содержимое буфера, а сам указатель.
              Ответить
            • Если я не ошибаюсь, проверять надо на 0x29a. Или дьявол зарегил 666 во всех системах счисления? )))
              Ответить
        • 1) В любом языке константы должны юыть слева в условных выражениях:
          а) в С/С++ что бы не было if (a=0)
          б) в Java/C# что бы не было NPE при if (a.eqals("QQQ")) (правильно: if ("QQQ".eqals(a))
          в) выражения вида if (someFunctionName(operandName + opN ) == 0 ) не всегда легко читаются из-за своей длины, а если константа слева её сразу видно.

          2) finish_time - да, забыл, что С, а не ++. Хотя под солярисом gcc и такое в "сишнном" коде съедал.

          3) имя переменной должно говорить о том, чем она является. Что такое datap? Это даже не слово английского языка.

          4) Программисты embedded не делают ошибок? Уже смешно. Если думаете, что от if упадёт производительность, то поставьте assert и пусть компилятор вырежет его в "релизе".

          5) Да! Даёшь код всей программы в одну строку :D
          Ответить
          • > в Java/C# что бы не было NPE при if (a.eqals("QQQ")) (правильно: if ("QQQ".eqals(a))

            Поправка!
            в Java, но не в C#, где достаточно a == "QQQ"
            Ответить
          • Этот код породит нехилое бурление жидких масс.
            1а) как бы защищает, но нигде чёта не видел такого.
            2) да и хуй с этим с finish_time
            3) автор кода лентяй ))
            4) ещё как делают, ток выявлять их дольше. if( uart_read_char(h, datap) ) нихуя не понял зачем, буффер параметром ведь передается

            5) Нииееет, всё ок с такой конструкцией. Это чтоб шпиону сложней было разбираться ;)
            Ответить
            • 4) Скорее всего, uart_read_char() возвращает результат операции чтения байта из UARTа в буфер. Если вернул нулевое значение - значит, байт не считан и содержимое памяти по адресу datap осталось без изменений
              Ответить
          • кстати, а как быть, если нужно две переменные сравнить?
            видимо, так: if(0 == a - b)
            но для строк не катит...
            Ответить
      • Вот ведь, а я думал, что тут нет больше меня повернутых на правилах :-)
        1. Слово "должны" тут неуместно. Крайне полезный совет для начинающих писать на С, С++ - ставьте константы справа, это перевалит на компилятор проверку конструкций вида if (to = 0). Однако через три месяца программирования такие конструкции цепляются взглядом и без помощи компилятора. Это не доктрина, это совет.
        2. Согласен, чем меньше время жизни переменной, тем легче читать код
        3. Не только эти. Вообще имена в коде говеные. Не буду голословным: что такое len? Длина принятых данных? Не, ошибка, это размер буфера. Что такое buf? Буфер? Опять ошибка - выходная С-строка. Хуже нечитаемых имен переменных, только имена переменных вводящие читателя в заблуждение.
        4. Все верно, при передаче в функцию указателя, ответственность по проверке актуальности указателя лежит на функции. Если эта проверка может быть сделана, она должна быть сделана.
        5. Действительно, плохо читаемая конструкция. В основном засчет изменения переменной в области проверки оператора if. Почти так же плохо как if (to = 0) ...

        от себя добавлю:
        6. Тут много говорили о встроенной платформе и оптимизации кода. Но тогда проверка to==0 в while не лезет ни в какие ворота: если ждать до времени - она лишняя.
        7. Возвращаемое функцией значение - лишнее: buf не модифицируется и возвращается всегда.
        8. Проверка len == 1 - выделение частного случая. Без нее вполне можно обойтись.

        Плюсую и говнокод и замечания.
        Ответить
        • описался, конечно же константы слева
          Ответить
        • Насчет проверки to==0: это условие отключает проверку тайм-аута. Другим вариантом было бы использование очень большого значения тайм-аута, при условии, что разрядность таймера это позволяет.
          Ответить
          • Это замечание касалось ссылок на оптимизацию: если говорить об оптимизации нужно было сделать внешний if (to == 0) или (0 == to), как тут предлагали, а внутри сделать 2 разных while, один без проверки на (clock_get_millis() < finish_time).
            Это позволило бы исключить одну проверку из цикла.
            Хотя, ИМХО, обращать внимание на такие детали во время написания кода - большое зло. Оптимизировать код нужно тогда и только тогда, когда доказана необходимость оптимизации и найдено узкое место.
            Ответить
      • Вы очевидно не писали для embedded
        1) Схуя константы слева?? Таково нигде нет, иф (0 == т) - хуита, Номад отдыхает. Это на троллинг похоже.
        2) насчёт datap и datae как бы формально вы правы. Но что, если я хочу отдебажить и всегда видеть, что в буфере. При оптимицации, думаю лишние ресурсы не уйдут.
        3) datap - data_pos, datae - data_end, что непонятного?
        4) Нафиг буф на 0 проверять, если есть len, который увеличивается в обработчике прерывания уарта. После раздрочки буффера или таймауте обнуляеся len в оддельной функции, и не надо тратить ресурсы на обнуление всего буффера.
        5) В принципе да, на первый взгляд нечитаемо. Но когда в следующий раз видишь (*some_ptr++ == some_const) сразу становится понятно.

        Вроде не говнокод. Минусовать нада? Я тут новенький.
        Ответить
    • Ну вы нашли к чему придраться. Ведители порядок аргументов при сравнении в if им не нравится. Или ведители "datap" и "datae" им ничего не говорит (код хоть и не мой, но мне почему сразу понятно).

      Вообще, если человек пишет так, как понятно ему, но не понятно вам, это не значит, что человек не прав. Самое главное в коде - это результирующий код после компиляции (безглючный, быстрый и т.п.) и возможность поддерживать. Лично я не вижу в данном стиле написания ничего такого, что например мне мешало бы поддерживать данный код, если бы он перешёл мне. Да есть непривычные на глаз вещи, как, например, эти "(len == 0)", ибо я привык писать "(!len)"; но это лишь из-за того что я редко встречаю "(len==0)", а не потому что это является уродством.
      Ответить
    • Мне даже впадлу ругаться на всё что тут понаписали в комментах (хоть и ещё раз напомню, что код цитирован не мой).
      Вот к примеру, вам не нравятся названия переменных "buf" и "len", а ведь лично я достаточно точно понимаю их смысл в коде, если вы нет - вероятно у нас с вами просто разное восприятие. "buf" - это буффер, в нём есть какие-то данные, которые в следствии как-то обрабатывались, и к концу работы функции, так получилось, что стали результирующей строкой. Зачем вводить ещё один поинтер указывающий туда же называя её "result"? Лично я как вижу функцию, понимаю, что buf - это просто указатель на начало буффера, а что в это буффере храниться этой переменной никак не касается, ибо повторюсь, buf - это просто указатель на начало какого-то буффера. А len - это опять очень очевидная по её применению переменная, это просто дополнительная переменная характеризующая состояние переменной "buf", этому len тоже пофиг как используют данные зарытые в данном буффере, у этой переменной основная функция указывать лишь на длинну буффера и всё. Дак вот после этих и так очевидных тезисов идём дальше, названия переменных вполне нормальные, новые создавать смысла нет, а самое оптимальное - итогую строку реализовать прямо в этом же буффере, отсюда и "return buf;". Я вот вообще не вижу никакой проблемы в данном названии переменной buf.
      Ответить
    • Поймите, ведь я не должен считать всё говнокодом, что написано не в той стилистике, как привык писать я.

      Вот например, я и кто-нибудь из вас пишем в разных стилистиках. Допустим оба пишем хороший безглючный, шустрый код, в которых оба сможем легко разобраться спустя несколько лет. Не будем же мы считать код друг друга - говнокодом? Это бред. Ибо если считать в данной ситуации код друг друга - говнокодом, то на любой код найдётся человек, для которого этот код будет говнокодом.
      Ответить

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