"Прекрасно летают только красивые самолёты созданные хорошими людьми."
(Андрей Николаевич Туполев)
Я долгое время негодовал по поводу того, что часто приходится копаться в плохо оформленном Си коде. Но теперь я решил подойти к проблеме с философской точки зрения. Отныне я коллекционирую такие куски странного кода. Буду сохранять отдельно все проявления несуразного кода и пополнять эту копилку. Теперь каждая нелепая функция приносит мне радость так, как пополняет ценную коллекцию того, как не надо делать. Понимаете?
Итак, код к параду построен!
28--Отсутствие имен переменных в определении структуры справа. Если в структуре поменять местами переменные, то всё тихо рухнет в run-time.
Или вот, если колонки LookUp таблицы поменять местами, то код соберется без ошибок. Однако программа будет полностью испорчена. Проблема в том, что никто этого не заметит.
/* Data bit table */
static const CAN_BaudRegType s_aFlexCan_DataBaudDividerTable[] =
{
/* clock source hz baudrate presdiv propseg pseg1 pseg2 rjw */
{ FLEXCAN_BAUDCLK_HZ_16M, FLEXCAN_BAUD_2M, 1, 3, 2, 2, 1 },
{ FLEXCAN_BAUDCLK_HZ_24M, FLEXCAN_BAUD_1M, 2, 5, 3, 3, 1 },
{ FLEXCAN_BAUDCLK_HZ_24M, FLEXCAN_BAUD_2M, 1, 5, 3, 3, 1 },
{ FLEXCAN_BAUDCLK_HZ_24M, FLEXCAN_BAUD_3M, 1, 3, 2, 2, 1 },
{ FLEXCAN_BAUDCLK_HZ_48M, FLEXCAN_BAUD_1M, 3, 7, 4, 4, 1 },
{ FLEXCAN_BAUDCLK_HZ_48M, FLEXCAN_BAUD_2M, 2, 5, 3, 3, 1 },
{ FLEXCAN_BAUDCLK_HZ_48M, FLEXCAN_BAUD_3M, 1, 7, 4, 4, 1 },
{ FLEXCAN_BAUDCLK_HZ_48M, FLEXCAN_BAUD_4M, 1, 5, 3, 3, 1 },
{ FLEXCAN_BAUDCLK_HZ_48M, FLEXCAN_BAUD_6M, 1, 3, 2, 2, 1 },
{ FLEXCAN_BAUDCLK_HZ_120M, FLEXCAN_BAUD_1M, 8, 8, 3, 3, 1 },
{ FLEXCAN_BAUDCLK_HZ_120M, FLEXCAN_BAUD_2M, 4, 8, 3, 3, 1 },
{ FLEXCAN_BAUDCLK_HZ_120M, FLEXCAN_BAUD_3M, 4, 5, 2, 2, 1 },
{ FLEXCAN_BAUDCLK_HZ_120M, FLEXCAN_BAUD_4M, 3, 5, 2, 2, 1 },
{ FLEXCAN_BAUDCLK_HZ_120M, FLEXCAN_BAUD_5M, 2, 5, 3, 3, 1 },
{ FLEXCAN_BAUDCLK_HZ_120M, FLEXCAN_BAUD_6M, 2, 5, 2, 2, 1 },
{ FLEXCAN_BAUDCLK_HZ_120M, FLEXCAN_BAUD_8M, 1, 8, 3, 3, 1 },
{ FLEXCAN_BAUDCLK_HZ_150M, FLEXCAN_BAUD_1M, 10, 8, 3, 3, 1 },
{ FLEXCAN_BAUDCLK_HZ_150M, FLEXCAN_BAUD_2M, 5, 8, 3, 3, 1 },
{ FLEXCAN_BAUDCLK_HZ_150M, FLEXCAN_BAUD_3M, 5, 5, 2, 2, 1 },
{ FLEXCAN_BAUDCLK_HZ_150M, FLEXCAN_BAUD_5M, 2, 8, 3, 3, 1 }
};
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 */
}
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
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.
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--Очевидные комментарии
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 файликов! Это просто клиника...
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). Это можно понять, если программист - Junior. Но кода в IDE программирует человек после 30+ и до сих пор не научился писать скрипты сборки - это уже инфантилизм какой-то.
26--Отдельная клиника, когда в компании, которая 30 лет пишет прошивки никто не додумался добавить UpTime счетчик, чтобы делать, например, time_stamp отметки в логе как это например уже есть в каждом Android смартфоне.
27--Или вот. Это классическое Кащенко. Организация решила написать свою версию CMSIS и 20+ программистов за 35 лет не осилили написать аналоги функций NVIC_EnableIRQ, NVIC_SetPriority, NVIC_GetVector, NVIC_GetActive и прочее. Образования хватило только на одну функцию-Бог NVIC_Init(). При этом в файле драйвера NVIC (прерываний) у них получилось аж 29 тысяч строк!
Написали 29k строк и не смогли повторить даже минимальный функционал бесплатного CMSIS. Это как?
Вот такие пирожки с капустой, понимаете?
Буквально пару слов про авторов этих сорцов. Это лояльные сотрудники. Они делают так потому, что тут так принято. Их можно сравнить с дрессированными дельфинами. У них по 10-15 лет опыта. При этом каждый из них сам никогда не покупал и не собирается покупать тот товар, который делает организация с которой они числятся.
Вывод
Как видно, к нелепостям приводит необоснованная спешка написания, copy-paste-программирование и невнимательность. Не делайте так.
Присылайте в комментариях примеры "лучшего кода", который видели Вы.
Ссылки
Название | URL |
Разработчики встраиваемых систем не умеют программировать @Spym | |
Атрибуты Хорошего С-кода | |
Большинству людей плевать на качество софта @alizar | |
КодоГенератор Линейных Отображений (как ускорить создание ASIC драйвера) | |
Пример недоделанного HAL-а | https://github.com/MikronMIK32/mik32-hal/blob/main/peripherals/Source/mik32_hal_gpio.c |
