#любой_язык #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 :)
