Переключатель рефакторинга или оператор if / else?

Question

Переключатель рефакторинга или оператор if / else?

Я работаю над школьным проектом и получил отзывы от моего учителя. Он сказал, что в моем коде есть некоторые плохие практики, он сказал, что случаи переключения могут быть заменены полиморфным подходом. Только я понятия не имею, как я мог это сделать.

Мой код получает сообщения от шины CAN. Эти сообщения приходят с разных устройств, я проверяю сообщения с какого устройства. Если есть новое устройство, я создаю объект, анализирую сообщение и сохраняю информацию.
Эта система практически одинакова для каждого сообщения.

Вот мой код

void Application::PollWhisperConnectBus()
{
HAL_GPIO_TogglePin(PORT_LED1, PIN_LED1);

whisper_connect_id_ = hcan2.pRxMsg->StdId;

if (whisper_connect_id_ >= 0x580 && whisper_connect_id_ <= 0x58F)
{
WIBDevice();
}
if (whisper_connect_id_ >= 0x590 && whisper_connect_id_ <= 0x59F)
{
BMSSDevice();
}
if (whisper_connect_id_ >= 0x5B0 && whisper_connect_id_ <= 0x5BF)
{
DCPowerCubeDevice();
}
if (whisper_connect_id_ >= 0x5C0 && whisper_connect_id_ <= 0x5CF)
{
ACPowerCubeDevice();
}
if (whisper_connect_id_ >= 0x700 && whisper_connect_id_ <= 0x70F)
{
WIBHeartBeatDevice();
}
}

Это одна из функций, которая проверяет наличие объекта класса, если это так, анализирует сообщение.

void Application::DCPowerCubeDevice()
{
bool found_device = false;
int device = (hcan2.pRxMsg->StdId & 0x0F) + device_instance_offset_;

WhisperConnectDevice* whisper_connect_device;
for(unsigned int i = 0; i < whisper_connect_device_list_.size(); ++i)
{
if ((whisper_connect_device = whisper_connect_device_list_.at(i)) != NULL &&
whisper_connect_device->GetClassName() == "DCPowerCube")
{
DCPowerCube* dc_powercube = dynamic_cast<DCPowerCube*>(whisper_connect_device);
if (dc_powercube != NULL)
{
if (dc_powercube->GetDevice() == device)
{
dc_powercube->ParseCanMessage(&hcan2);
found_device = true;
break;
}
}
}
}
if (!found_device)
{
WhisperConnectDevice* dc_powercube;
if ((dc_powercube = new DCPowerCube) != NULL)
{
dc_powercube->SetDevice(device);

int n2k_address = nmea2000_.FindFirstFreeCanId(n2k_address_, device_list_);

if (n2k_address != 0xFFFF)
{
dc_powercube->SetSrcCanId(n2k_address);
dc_powercube->SetDeviceInstanceOffset(device_instance_offset_);
dc_powercube->SetDeviceInstance(0x30 + device);
dc_powercube->AddressClaim(nmea2000_);
dc_powercube->SendPGN126996(nmea2000_);
dc_powercube->SendPGN126998(nmea2000_, "DCPowerCube", "", "");
device_list_.at(n2k_address) = 0x01;
}

DCPowerCube* dc_powercube2 = dynamic_cast<DCPowerCube*>(dc_powercube);
if (dc_powercube2 != NULL)
{
dc_powercube2->SetCurrentLimit(16);
}
AddToWPCDeviceList(dc_powercube);
}
}
}

void DCPowerCube::ParseCanMessage(CAN_HandleTypeDef *can_handle)
{
if (can_handle != NULL)
{
uint16_t message_index = (can_handle->pRxMsg->Data[1] << 8) + can_handle->pRxMsg->Data[2];

switch (message_index)
{
case 0x1008:
device_name_[0] = can_handle->pRxMsg->Data[4];
device_name_[1] = can_handle->pRxMsg->Data[5];
device_name_[2] = can_handle->pRxMsg->Data[6];
device_name_[3] = can_handle->pRxMsg->Data[7];
device_name_[4] = '\0';
break;
case 0x100A:
software_version_[0] = can_handle->pRxMsg->Data[4];
software_version_[1] = can_handle->pRxMsg->Data[5];
software_version_[2] = can_handle->pRxMsg->Data[6];
software_version_[3] = can_handle->pRxMsg->Data[7];
software_version_[4] = '\0';
break;
case 0x1018:
serial_number_ = can_handle->pRxMsg->Data[4] << 24 | can_handle->pRxMsg->Data[5] << 16 |
can_handle->pRxMsg->Data[6] << 8 | can_handle->pRxMsg->Data[7];
break;
case 0x2100:  // DC PowerCube status
power_cube_status_ = can_handle->pRxMsg->Data[4];
io_status_bit_ = can_handle->pRxMsg->Data[5];
dip_switch_status_bit_ = can_handle->pRxMsg->Data[6];
break;
case 0x2111:  // Grid voltage, current, current limit
grid_voltage_ = (can_handle->pRxMsg->Data[4] << 8) + can_handle->pRxMsg->Data[5];
grid_current_ = can_handle->pRxMsg->Data[6];
grid_current_limit_ = can_handle->pRxMsg->Data[7];
break;
case 0x2112:  // Generator frequency, RPM
generator_freq_ = (can_handle->pRxMsg->Data[4] << 8) + can_handle->pRxMsg->Data[5];
rpm_ = (can_handle->pRxMsg->Data[6] << 8) + can_handle->pRxMsg->Data[7];
break;
case 0x2113:  // Generator current
gen_current_phase1_ = can_handle->pRxMsg->Data[4];
gen_current_phase2_ = can_handle->pRxMsg->Data[5];
gen_current_phase3_ = can_handle->pRxMsg->Data[6];
gen_current_limit_ = can_handle->pRxMsg->Data[7];
break;
case 0x2114:  // Load percentage
grid_load_ = can_handle->pRxMsg->Data[4];
generator_load_ = can_handle->pRxMsg->Data[5];
dc_output_load_ = can_handle->pRxMsg->Data[6];
break;
case 0x2151:  // Battery type & charger state
battery_type_ = can_handle->pRxMsg->Data[4];
charger_state_ = can_handle->pRxMsg->Data[5];
break;
case 0x2152:  // DC output voltage & DC slave voltage
dc_output_voltage_ = (can_handle->pRxMsg->Data[4] << 8) + can_handle->pRxMsg->Data[5];
dc_slave_voltage_ = (can_handle->pRxMsg->Data[6] << 8) + can_handle->pRxMsg->Data[7];
break;
case 0x2153:  // DC output current & DC output current limit
dc_output_current_ = (can_handle->pRxMsg->Data[4] << 8) + can_handle->pRxMsg->Data[5];
dc_output_current_limit_ = (can_handle->pRxMsg->Data[6] << 8) + can_handle->pRxMsg->Data[7];
break;
case 0x21A0:  // Temperature sensor
temp_sens_BTS_ = can_handle->pRxMsg->Data[4];
temp_sens_intern1_ = can_handle->pRxMsg->Data[5];
temp_sens_intern2_ = can_handle->pRxMsg->Data[6];
temp_sens_intern3_ = can_handle->pRxMsg->Data[7];
break;
case 0x21A1:
break;
}
}
}

WhisperConnectDevice является базовым классом DCPowerCube.

Я хотел бы получить некоторые отзывы о том, как подойти к этой проблеме.

0

c++polymorphism stm switch-statement

Решение

Другие решения

Других решений пока нет …

Источник

Accepted Answer

Независимо от того, вводите ли вы полиморфизм или нет, кажется, что вы должны сопоставить внешне предоставленный номер типа (ID) с кодом, так что вам всегда понадобится некоторая промежуточная структура.

Ваши кандидаты:

Блок из if заявления наверное if—else—if…
switch заявление (если значения являются допустимыми)
Какая-то справочная таблица (массив, ассоциативная карта, другое …)

Вы уже получили if но может улучшиться с if—else—if,
Как правило, это считается самым уродливым подходом к горячим точкам кодирования с высоким потенциалом обслуживания. Горячая точка кодирования, потому что все новые идентификаторы возвращаются в этот блок кода.

Я также заметил, что в этом случае все ваши диапазоны 0xnn0 в 0xnnF включительно для некоторых nn, так что вы можете по крайней мере упростить, уменьшив младшие 4 бита:

auto whisper_connect_type = whisper_connect_id_ >> 4;

Ваш switch Опция упрощается до:

switch(whisper_connect_type) {
case 0x58: WIBDevice(); break;
case 0x59: BMSSDevice(); break;
case 0x5B: DCPowerCubeDevice(); break;
case 0x5C: ACPowerCubeDevice(); break;
case 0x70: WIBHeartBeatDevice(); break;
default: HandleUnknownDeviceIDError(whisper_connect_id_); break;
}

NB: Я очень рекомендую некоторый код для обработки неподдерживаемого идентификатора. Мой совет — бросить исключение или что-то, ведущее к прекращению. break; для полноты Я не думаю, что вы возвращаетесь с неизвестного удостоверения личности.

Альтернативой является определение ассоциативной карты:

#include <iostream>
#include <unordered_map>
#include <memory>

class WhisperHandler {
public:
virtual void HandleWhisper() const = 0 ;
virtual ~WhisperHandler() {}
};

class WhisperHandlerWIBDevice : public WhisperHandler {
public:
void HandleWhisper() const override {
std::cout << "Handler WIBDevice...\n";
}
} ;

int main() {
std::unordered_map<unsigned,std::unique_ptr<const WhisperHandler>> handlers;

//...

std::unique_ptr<const WhisperHandler> handler(std::make_unique<const WhisperHandlerWIBDevice>());
std::pair<const unsigned , std::unique_ptr<const WhisperHandler> > pair({0x5B,std::move(handler)});
handlers.insert(std::move(pair));

//...

{
const auto &chandlers=handlers;
auto handlerit(chandlers.find(0x5B1));
if(handlerit!=chandlers.end()){
handlerit->second->HandleWhisper();
}else{
//ERROR - UNKNOWN HANDLER.
}
}
return 0;
}

Однако я бы посоветовал получить возврат инвестиций только для всего этого полиморфного механизма, если вы разрешите динамическую регистрацию обработчиков либо из разных модулей приложения, либо путем динамической загрузки библиотек, которые регистрируются при загрузке.

Если это одно проектное приложение (как оно выглядит), то switch Таблица отправки будет работать нормально.

Поскольку приложения, как правило, обмениваются данными с использованием идентификаторов определенного типа, ОО может начать выглядеть громоздким, когда на практике требуется взять идентификатор, сопоставить его с полиморфным обработчиком и затем вызвать обработчик. По логике вы дважды сделали ID для логического отображения!

Сноска. Хитрость в отбрасывании младших 4 битов несколько отличается от этих методов и (конечно) слегка хрупкая, если младшие 4 бита становятся релевантными для определения обработчика по линии.

2