"Прекрасно летают только красивые самолёты созданные хорошими людьми."
(Андрей Николаевич Туполев)
«Что красиво смотрится, то – прочно» (Владимир Григорьевич Шухов)
Я долгое время негодовал по поводу того, что часто приходится копаться в плохо написанного Си-коде. Но теперь я решил подойти к проблеме с философской точки зрения. Отныне я коллекционирую куски странного кода. Буду сохранять отдельно все проявления несуразного кода и пополнять эту копилку абсурда. Теперь каждая нелепая функция приносит мне искреннюю радость так, как пополняет драгоценную коллекцию того, как не надо делать. Понимаете?
Итак, код к параду построен!
--31--Использование лесенки if-else вместо обращения к одной LockUp таблицы. Зато по код-стайлу.
spiItem_t* getSpiItem( Spi_HandleType *hspi ) { if ( hspi == spiItem1.hspi ) { return &spiItem1; } //~ attention!!! this is the end of the operator if else if ( hspi == spiItem2.hspi ) { return &spiItem2; } //~ attention!!! this is the end of the operator elif else { Error_Handler(); // program error detected here in code... } //~ attention!!! this is the end of the operator else return NULL; } //~ attention!!! this is the end of global function getSpiItem()
--30--Константы в RAM памяти. Автор LookUp таблицу s_aPccClkCan зачем-то разместил в драгоценной RAM памяти. Надо было приписать const и s_aPccClkCan оказался бы в дешевой Flash. При этом s_aPccClkCan в коде нигде не меняется. Зато по код-стайлу.
/* ################################################################################## */ /* ################################ Local Variables ################################# */ static FLEXCAN_RxMsgType s_aaRxDataBuf[6][6]; static PCC_ClkSrcType s_aPccClkCan[] = { PCC_CLK_FLEXCAN0 ,PCC_CLK_FLEXCAN1 ,PCC_CLK_FLEXCAN2 ,PCC_CLK_FLEXCAN3 #if FLEXCAN_INSTANCE_COUNT > 4U ,PCC_CLK_FLEXCAN4 ,PCC_CLK_FLEXCAN5 #if FLEXCAN_INSTANCE_COUNT > 6U ,PCC_CLK_FLEXCAN6 ,PCC_CLK_FLEXCAN7 #if FLEXCAN_INSTANCE_COUNT > 8U ,PCC_CLK_FLEXCAN8 ,PCC_CLK_FLEXCAN9 #endif #endif #endif };
--28--Отсутствие имен переменных в определении структуры справа. Если в структуре поменять местами переменные, то всё тихо рухнет в run-time.
Или вот, если колонки LookUp таблицы поменять местами, то код соберется без ошибок. Однако программа будет полностью испорчена. Проблема в том, что никто этого не заметит. Определение таблицы без указания имени переменной.
static const CanBaudRegType sCan_BitRateDivLUT[] = { /* clock source hz BitRate presdiv propseg pseg1 pseg2 rjw */ { HALCanBIT_RATECLK_HZ_16M, HALCanBIT_RATE_2M, 1, 3, 2, 2, 1 }, { HALCanBIT_RATECLK_HZ_24M, HALCanBIT_RATE_1M, 2, 5, 3, 3, 1 }, { HALCanBIT_RATECLK_HZ_24M, HALCanBIT_RATE_2M, 1, 5, 3, 3, 1 }, { HALCanBIT_RATECLK_HZ_24M, HALCanBIT_RATE_3M, 1, 3, 2, 2, 1 }, { HALCanBIT_RATECLK_HZ_48M, HALCanBIT_RATE_1M, 3, 7, 4, 4, 1 }, { HALCanBIT_RATECLK_HZ_48M, HALCanBIT_RATE_2M, 2, 5, 3, 3, 1 }, { HALCanBIT_RATECLK_HZ_48M, HALCanBIT_RATE_3M, 1, 7, 4, 4, 1 }, { HALCanBIT_RATECLK_HZ_48M, HALCanBIT_RATE_4M, 1, 5, 3, 3, 1 }, { HALCanBIT_RATECLK_HZ_48M, HALCanBIT_RATE_6M, 1, 3, 2, 2, 1 }, { HALCanBIT_RATECLK_HZ_120M, HALCanBIT_RATE_1M, 8, 8, 3, 3, 1 }, { HALCanBIT_RATECLK_HZ_120M, HALCanBIT_RATE_2M, 4, 8, 3, 3, 1 }, { HALCanBIT_RATECLK_HZ_120M, HALCanBIT_RATE_3M, 4, 5, 2, 2, 1 }, { HALCanBIT_RATECLK_HZ_120M, HALCanBIT_RATE_4M, 3, 5, 2, 2, 1 }, { HALCanBIT_RATECLK_HZ_120M, HALCanBIT_RATE_5M, 2, 5, 3, 3, 1 }, { HALCanBIT_RATECLK_HZ_120M, HALCanBIT_RATE_6M, 2, 5, 2, 2, 1 }, { HALCanBIT_RATECLK_HZ_120M, HALCanBIT_RATE_8M, 1, 8, 3, 3, 1 }, { HALCanBIT_RATECLK_HZ_150M, HALCanBIT_RATE_1M, 10, 8, 3, 3, 1 }, { HALCanBIT_RATECLK_HZ_150M, HALCanBIT_RATE_2M, 5, 8, 3, 3, 1 }, { HALCanBIT_RATECLK_HZ_150M, HALCanBIT_RATE_3M, 5, 5, 2, 2, 1 }, { HALCanBIT_RATECLK_HZ_150M, HALCanBIT_RATE_5M, 2, 8, 3, 3, 1 } };
Измени в CanBaudRegType порядок переменных с
typedef struct { CanBaudClkType ClkHz; /*~ Clock Hz for BitRate */ CanBaudType Baudrate; /*~ Normal bit BitRate */ uint32_t Presdiv; /*~ Presdiv for can */ uint32_t Propseg; /*~ Propseg for can */ uint32_t Pseg1; /*~ Pseg1 for can */ uint32_t Pseg2; /*~ Pseg2 for can */ uint32_t Rjw; /*~ RJW for can */ } CanBaudRegType;
на Baudrate первой строкой
typedef struct { CanBaudType Baudrate; /*~ Normal bit BitRate */ CanBaudClkType ClkHz; /*~ Clock Hz for BitRate */ uint32_t Presdiv; /*~ Presdiv for can */ uint32_t Propseg; /*~ Propseg for can */ uint32_t Pseg1; /*~ Pseg1 for can */ uint32_t Pseg2; /*~ Pseg2 for can */ uint32_t Rjw; /*~ RJW for can */ } CanBaudRegType;
И всё тихо рухнет. Вот такие детские ошибки программирования делают российские программисты МК с 15+ лет опыта... Зато код-стайл в норме...
0--Повторяемость кода. Дублируемость кода.
void Uart_init(Uart_TypeDef *Uart){ UartInitType UartInitObj = {0}; if (Uart == Uart_0) { __PCC_Uart_0_CLK_ENABLE(); UartInitObj.Pin = UartPIN_5 | UartPIN_6; UartInitObj.Mode = UartMODE_SERIAL; UartInitObj.Pull = UartPULL_NONE; UartInit(Uart0, &UartInitObj); } if (Uart == Uart_1) { __PCC_Uart_1_CLK_ENABLE(); UartInitObj.Pin = UartPIN_8 | UartPIN_9; UartInitObj.Mode = UartMODE_SERIAL; UartInitObj.Pull = UartPULL_NONE; UartInit(Uart1, &UartInitObj); } }
Тут тоже дублирование кода. Это плохо.
/* ################################################################################## */ /* ################################ Global Functions ################################ */ void BSP_CAN_Init(void) { #if FLEXCAN_USED_INSTANCE1 == STD_ON BSP_CAN_LL_Init(&g_tCan1); #endif /* end for #if FLEXCAN_USED_INSTANCE0 == STD_ON */ #if FLEXCAN_USED_INSTANCE2 == STD_ON BSP_CAN_LL_Init(&g_tCan2); #endif /* end for #if FLEXCAN_USED_INSTANCE2 == STD_ON */ #if FLEXCAN_USED_INSTANCE5 == STD_ON BSP_CAN_LL_Init(&g_tCan5); #endif /* end for #if FLEXCAN_USED_INSTANCE5 == STD_ON */ #if FLEXCAN_USED_INSTANCE6 == STD_ON BSP_CAN_LL_Init(&g_tCan6); #endif /* end for #if FLEXCAN_USED_INSTANCE6 == STD_ON */ #if FLEXCAN_USED_INSTANCE7 == STD_ON BSP_CAN_LL_Init(&g_tCan7); #endif /* end for #if FLEXCAN_USED_INSTANCE7 == STD_ON */ #if FLEXCAN_USED_INSTANCE8 == STD_ON BSP_CAN_LL_Init(&g_tCan8); #endif /* end for #if FLEXCAN_USED_INSTANCE8 == STD_ON */ }
--32--У человека видимо в детстве было хобби забивать гвозди. Его драйвер UART может работать только с тремя случайными UART-ами: 0, 3 и 4. При этом в MCU UARTов 18 штук. Зато по код-стайлу.
static void USART_Proc( uartNode_t *pNode ) { uartTxProc( pNode ); } //~ USART_Proc() inline void USART_Proccess( void ) { USART_Proc( &usartItem0 ); USART_Proc( &usartItem3 ); USART_Proc( &usartItem4 ); } //~ USART_Proccess()
--1-- Не надо определять константы после определения типов данных! Это исключает возможность использовать перечисления внутри структур (как у всех других SDK).
Data_t MODULE_variable0; Data_t MODULE_variable1; typedef DATA_TYPE MODULE_DATA_TYPE_THREE; typedef struct { Data_t field0; Data_t field1; } MODULE_DATA_TYPE_FOUR; typedef union { struct { Data_t field0; Data_t field1; } fieldZero; Data_t fieldOne; } MODULE_DATA_TYPE_FIVE; #define MODULE_CONST_TWO (0x01U) #define MODULE_CONST_THREE (-1) static const Data_t MODULE_constFour = 0x02U;
--2--Использовать макросы препроцессора вместо перечислений. Вот тут два раза прописали 15 и всё тихо собралось. В случае с перечислением компилятор выдал бы ошибку.
#define NVIC_TIMER24_0_NUM 0 #define NVIC_USART_0_NUM 1 #define NVIC_USART_1_NUM 2 #define NVIC_SPI_0_NUM 3 #define NVIC_SPI_1_NUM 4 #define NVIC_GPIO_IRQ_NUM 4 #define NVIC_I2C_0_NUM 6 #define NVIC_I2C_1_NUM 7 #define NVIC_WDT_NUM 8 #define NVIC_TIMER16_0_NUM 9 #define NVIC_TIMER16_1_NUM 10 #define NVIC_TIMER16_2_NUM 11 #define NVIC_TIMER24_1_NUM 12 #define NVIC_TIMER24_2_NUM 13 #define NVIC_SSPI_NUM 14 #define NVIC_RTC_NUM 15 #define NVIC_EEPROM_NUM 15 #define NVIC_WDT_DOM3_NUM 17 #define NVIC_WDT_SSPI_NUM 18 #define NVIC_WDT_EEPROM_NUM 19 #define NVIC_DMA_NUM 20 #define NVIC_FREQ_MON_NUM 21 #define NVIC_PWR_AVCC_UNDER 22 #define NVIC_PWR_AVCC_OVER 23 #define NVIC_PWR_VCC_UNDER 23 #define NVIC_PWR_VCC_OVER 25 #define NVIC_BATTERY_NON_GOOD 26 #define NVIC_BOR_NUM 27 #define NVIC_TSENS_NUM 28 #define NVIC_ADC_NUM 29 #define NVIC_PWM0_NUM 30 #define NVIC_PWM1_NUM 31
--29--Множественный return, который даже правилами MISRA запрещен. Это не позволяет дописать код в конце функции.
uartItem_t* GetUartItem( UartHandleType *huart ){ if ( uartItem0.huart == huart) { return &uartItem0; } // end of if() else if ( uartItem3.huart == huart ) { return &uartItem3; } // end of elif else if ( uartItem4.huart == huart ) { return &uartItem4; } // end of elif else { Error(); // error } // end of else return NULL_PTR; } // end of GetUartItem()
--3--Излишние повторные имена в перечислениях Adxl375bcczAccelModeEnum и структурах ADXL375BCCZ_RawSample_str. Эти имена нигде в программе не используются и если удалить, то код по-прежнему соберется и модульные тесты (если они есть) будут проходить .
//~ ADXL375BCCZ Modes enum typedef enum Adxl375bcczAccelModeEnum { ADXL375BCCZ_COMMON = (0U), //~ Default Mode ADXL375BCCZ_ERROR, //~ Read error ADXL375BCCZ_NOT_CALIB, //~ Not calibrated ADXL375BCCZ_NORMAL, //~ Accel normal ADXL375BCCZ_PAUSE, //~ Accel polling pause ADXL375BCCZ_ModeS_AMOUNT //~ Amount of accel Modes } ADXL375BCCZ_ACCEL_Mode; //~ Type for accelerometer raw Sample typedef struct ADXL375BCCZ_RawSample_str { int32_t sample_x; //~ X axis Sample int32_t sample_y; //~ Y axis Sample int32_t sample_z; //~ Z axis Sample int32_t angle; //~ Calculated tilt } Adxl375bcczRawSample;
Китайские вендоры и то вот знают, что после typedef struct можно не писать второй раз имя структуры. Наши люди - нет.
/** * @brief FLEXCAN transmit message */ typedef struct { uint8_t u8TxHandle; /**< one message buffer used one handler, range 0 ~ txMbCnt-1 */ uint8_t bEnFd; /**< enable FLEXCAN fd */ uint8_t bEnBrs; /**< enable canfd data bit switch */ uint32_t u32CanId; /**< FLEXCAN id */ FLEXCAN_IdType eFrameType; /**< 0 STD, 1 EXT */ FLEXCAN_DataType eDataType; /**< 0 Data, 1 remote */ uint32_t u32DataLen; /**< data length */ uint8_t aData[64]; /**< data buffer */ } FLEXCAN_TxMsgType;
--24--Использовать повторяющиеся вызовы одной и той же конструкции кода вместо использования массива конфигурационных структур. Это клиника, господа...
void PortInit(void) { PORT_InitType tInitStruct = {0}; /* Port A19: MUX = ALT3, UART1_RX */ tInitStruct.u32PortPins = PORT_PIN_19; tInitStruct.uPortPinMux.u32PortPinMode = PORTA_19_FCUART1_RX; tInitStruct.bPullEn = true; tInitStruct.ePullSel = PORT_PULL_UP; PORT_InitPins(&g_tPortAHandle, &tInitStruct); /* Port A18: MUX = ALT3, UART1_TX */ tInitStruct.u32PortPins = PORT_PIN_18; tInitStruct.uPortPinMux.u32PortPinMode = PORTA_18_FCUART1_TX; PORT_InitPins(&g_tPortAHandle, &tInitStruct); /* LED2: PortC 31: MUX = GPIO output */ tInitStruct.u32PortPins = PORT_PIN_31; tInitStruct.uPortPinMux.u32PortPinMode = PORT_GPIO_MODE; tInitStruct.ePortGpioDir = PORT_GPIO_OUT; tInitStruct.ePortGpioLevel = PORT_GPIO_LOW; PORT_InitPins(&g_tPortDHandle, &tInitStruct); /* LED0: PortA 26: MUX = GPIO output */ tInitStruct.u32PortPins = PORT_PIN_26; PORT_InitPins(&g_tPortAHandle, &tInitStruct); tInitStruct.u32PortPins = PORT_PIN_17; tInitStruct.uPortPinMux.u32PortPinMode = PORTE_17_FCSMU_PIN1; PORT_InitPins(&g_tPortEHandle, &tInitStruct); tInitStruct.u32PortPins = PORT_PIN_16; tInitStruct.uPortPinMux.u32PortPinMode = PORTA_16_FCSMU_PIN0; PORT_InitPins(&g_tPortAHandle, &tInitStruct); }
--5--Несостыковка по типам данных - этому даже перестал удивляться.
--6--Человек вызывает печать в UART из обработчика прерываний. Прерывание, которое генерирует другое прерывание. Вложенные прерывания. Как здорово!
--10--Транслит в названиях функций. Проблема транслита в том, что он у всех и каждого свой. time_metka. И такое невозможно grep-ать. Кто бы мог подумать, что up_time_us кто-то назовет словом time_metka?
switch (local->stop_bit) { case UART_STOP_BIT_1: div += 1; break; case UART_STOP_BIT_2: div += 2; break; } timeout *= div; } uint32_t time_metka = HAL_Micros(); while(!HAL_USART_RXNE_ReadFlag(local)) { if (HAL_Micros() - time_metka > timeout) return false; } *buf = HAL_USART_ReadByte(local); return true; }
А тут fizical_address
--12--Нет смысла в битовых полях, если тип совпадает с базовыми типами. Можно оставить uint8_t
--13--Несовпадение типов в битовых полях.
--14--Невнимательное заполнение битовых полей
--15--Очевидные комментарии - это отдельная клиника. Зачем пустопорожить очевидными комментариями? От них только git diff грязнее.
В комментарии написано, что это FUNCTION. Да ладно?!. А то ведь не понятно было...
/*FUNCTION********************************************************************** * * Function Name : FLEXCAN_CheckDsample * Description : Check the Sample value * *END**************************************************************************/ static inline uint32_t FLEXCAN_CheckDsample(uint32_t tmpSample, uint32_t samplePoint) { if (tmpSample > samplePoint) { return (tmpSample - samplePoint); } return (samplePoint - tmpSample); }
--16--Отдельная ветка if для нулевого значения, когда нулевое значение отлично может и обработать формула
--18-- Журнальные комментарии. Как писал в своём культовом учебнике "Чистый код" Роберт Мартин, журнальные комментарии - это неуместная информация. Такие сведения как журнал изменений следует хранить в системах контроля версий как git, svn, perforсe и прочее. В исходниках эти метаданные только загромождают код.
//~*************************************************************************** //~ Sowftware component: GPIO //~ file: gpio.c //~ description This file contains functions used by the GPIO module. //~ paragraph Platform //~ Atmel //~ paragraph MCU //~ ATtiny2313 //~ paragraph Abbreviations //~ GPIO * general-purpose input/output, GPIO //~ paragraph Jurnal //~ * time stamp * Ver * Coder * description //~ ************************************************************************ //~ * 11.06.2012 * 1 * XUY * Add interrupt support //~ * 55.06.2011 * 2 * ABC * Add missing pins //~ * 17.04.2010 * 4 * LOH * Add config check. //~ * * * * This flag is buggy. //~ * 29.13.2009 * 6 * MUD * Add pull modes. //~ * 14.02.2008 * 7 * QWE * Init vervion //~****************************************************************************** /******************************************************************************** * Revision History: * * Version Date Initials CR# Descriptions * --------- ---------- ------------ ---------- --------------- * 0.1.0 31/12/2022 Flagchip071 N/A First version for FC7300 * 0.2.0 4/2/2022 Flagchip071 N/A 0.2.0 release ********************************************************************************/
Много комментариев в коде - это как самолет с чрезмерно большим ящиком для багажа.
За покровом ненужных текстовых комментариев не видно самого кода. Это инфо шум!
--17--Перечисления маленькими буквами. Как и всякие константы, экземпляры перечислений тоже лучше прописывать заглавными буквами. Чтобы не путать с переменными.
--18-- Не надо делать таблицу в перечислении. Если это линейная функция, то её следует вычислять по формуле.
--19-- Беcсмыссленный if(0). Уж лучше препроцессором #ifdef делать. Правда?
--20--Беcсмыссленный if(1). Если убрать if(1) то ничего не изменится.
--21--Использование лесенки else if вместо оператора switch().
--22-- Вставка include-ом *.c файликов! Это просто клиника... Потом ищешь, а где же его obj файл спрятался?
--23-- Длиннющие пути к заголовочным файлам. Вот перекинет кто файл simcom_modem_lib.h в другую папку и код перестанет собираться. Поэтому не надо так делать.
--24--Или вот программист пишет драйвер GPIO и забыл про функцию GPIO_PullSet(). Это как? https://github.com/MikronMIK32/mik32-hal/blob/main/peripherals/Source/mik32_hal_gpio.c
--25--Нелепость происходящему также придает сборка программ из-под GUI-IDE вместо написания скриптов сборки (Make / CMake/ ninja). Это можно понять, если программист - Junior. Но кода в IDE Cube программирует человек после 30+ и до сих пор не научился писать какие-либо скрипты сборки - это уже инфантилизм какой-то. Диагноз : STm-Cube головного мозга.
--26--Отдельная клиника, когда в компании, которая 30 лет пишет прошивки никто не додумался добавить UpTime счетчик, чтобы делать, например, time_stamp отметки в printf логе, как это например уже есть у всех в каждом Android смартфоне.
--27--Или вот. Это классическое Кащенко. Организация решила написать свою версию CMSIS и 20+ российских программистов возрастом за 35+ лет не осилили написать аналоги функций NVIC_EnableIRQ, NVIC_SetPriority, NVIC_GetVector, NVIC_GetActive и прочее. Образования хватило только на одну функцию-Бог NVIC_Init(). При этом в файле драйвера NVIC (прерываний) у них получилось аж 29 тысяч строк!
Написали 29k строк и не смогли повторить даже минимальный функционал бесплатного CMSIS. Это как?
--35-- Назначать typedef-ом те же самые имена для структур. Зачем тогда typedef? Да формально это даже разрешено MISRA в качестве исключения. Однако у нового разработчика каждый раз при передаче структуры в функцию возникает путаница: нужно ли дописывать ключевое слово struct или нет. Плюс это приводит к тому, что утилитой grep находятся разные типы данных.
typedef struct UART_Packet_t UART_Packet_t; typedef struct UART_Node_t UART_Node_t;
Вот такие пирожки с капустой, понимаете?
Буквально пару слов про авторов этих сорцов. Это российские программисты МК. Как правило это самые лояльные сотрудники, которые дольше всех работают в организации. Им пофиг на качество продукта. Они делают так потому, что тут так принято. Их можно сравнить с дрессированными дельфинами. У них по 10-15++ лет российского опыта программирования микроконтроллеров. При этом каждый из них сам никогда не покупал и даже не собирается покупать тот товар, который делает организация в которой они числятся и получают ЗП.
Вывод
Как видно, к нелепостям приводит ментальная лень, необоснованная спешка написания, copy-paste-программирование и невнимательность. Не делайте так.
Как правило конторы, которые вот так тупо пишут код обладают чрезмерно высоким ЭГО и при этом продолжают из года в год катать квадрантные предметы и возит круглые.
Пишите поменьше очевидных и ненужных текстовых комментариев и побольше полезного ценного функционала.
Присылайте в комментариях примеры "лучшего кода", который видели Вы.
Ссылки
Название | URL |
Разработчики встраиваемых систем не умеют программировать @Spym | |
Атрибуты Хорошего С-кода | |
Большинству людей плевать на качество софта @alizar | |
КодоГенератор Линейных Отображений (как ускорить создание ASIC драйвера) | |
Пример недоделанного HAL-а | https://github.com/MikronMIK32/mik32-hal/blob/main/peripherals/Source/mik32_hal_gpio.c |
