Чистый код: Switch

#любой_язык #switch #рефакторинг

Корректно ли выносить цепочку Switch в отдельный метод, если цепочка довольно большая?
Или можно оставить ее в том же методе? Какой вариант выглядит лучше/красивее/чаще используется?
    

Ответы

Ответ 1

Вы подходите не совсем правильно. Разбивать функцию на части нужно не по формальным
признакам («длинный switch»), а по логическим. Вы должны задать себе вопрос: имеет
ли ваш кусок кода самостоятельный смысл? (Например: можно ли сказать несколькими словами,
что именно этот кусок делает?)

Если ответ на этот вопрос положительный, вынесите этот кусок в функцию, и назовите
её этими самыми словами. Если отрицательный — оставляйте всё как было.



Небольшое дополнение. Кодировать switch можно по-разному. Можно оставить его как
switch. Если это отображение одного объекта на другой, можно закодировать его как выборку
из std::unordered_map. А возможно, ваш switch лучше представить в виде вызова виртуальной
функции.

Когда именно и как правильно — снова-таки зависит от смысла. Пример:

string text;
switch (code)
{
case 100:
    text = "Continue";
    break;
case 101:
    text = "Switching protocols";
    break;
case 200:
    text = "OK";
    break;
case 301:
    text = "Moved permanently";
    break;
case 404:
    text = "Not found";
    break;
}


по идее лучше закодировать так:

unordered_map message_text
{
    { 100, "Continue" },
    { 101, "Switching protocols" },
    { 200, "OK" },
    { 301, "Moved permanently" },
    { 404, "Not found" }
};


и в коде

string s = message_text[200];


или там

string s;
auto it = message_text.find(200);
if (it != message_text.end())
    s = it->second;


(и выделить в отдельную функцию GetHttpMessageByCode).



Пример того, когда switch разумно заменить на иерархию классов:

switch (employee_kind)
{
case EmployeeKind::regular:
    salary = get_base_salary();
    bonus = total_profit * bonus_ratio / number_of_employees;
    break;
case EmployeeKind::external:
    salary = get_work_hours * get_hourly_rate(employee_id);
    bonus = 0;
    break;
case EmployeeKind::manager:
    salary = get_base_salary();
    bonus = bonus_fund / number_of_managers;
    if (salary < 100)
        bonus += 100 - salary;
    break;
}


Такой код можно заменить на иерархию классов:

class employee
{
protected:
    int get_base_salary() { return 0; }
public:
    virtual void compute_salary_and_bonus() = 0;
};

class regular_employee : public employee
{
public:
    virtual void compute_salary_and_bonus()
    {
        salary = get_base_salary();
        bonus = total_profit * bonus_ratio / number_of_employees;
    }
};

class expernal_employee : public employee
{
public:
    virtual void compute_salary_and_bonus()
    {
        salary = get_work_hours() * get_hourly_rate();
        bonus = 0;
    }
};

class manager : public employee
{
public:
    virtual void compute_salary_and_bonus()
    {
        salary = get_base_salary();
        bonus = bonus_fund / number_of_managers;
        if (salary < 100)
            bonus += 100 - salary;
    }
};


Ответ 2

Зависит от того, когда читабельность будет выше. Если у Вас какой-то простой перекодировщик,
то логично вынести в отдельный метод

int charToInt(char a) {
  switch (a) {
    case '0': return 0;
    case '1': return 1;
    case '2': return 2;
    ...........
    default: throw "Error char"
  }
}


(да, я знаю, что можно кастануть к int и вычесть 0x30)

Если же у Вас какой-то парсер

switch (a) {
  case '0':
    ........
    break;
  case '1':
    ........
    break;
  default: throw "Error char"
}


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

Наверное, общий совет будет такой - если Вы можете вынести switch в метод, который
принимает ровно один аргумент, а все break можете заменить на return, то выносите


Ответ 3

Зависит от предпочтений. Я например - выношу и даю название CheckSomething  - something
это то, что проверяется :) И не забудте в любом случае default case :)