Ревью кода: стилистика
Хочу попробовать написать данный алгоритм более кратко, но не знаю как. Буду рад любой помощи
Данный алгоритм добавляет условия в секцию WHERE в запросе
if (filterValue && (filterTo || filterFrom)) {
if (filterTo && filterFrom) {
queryParams.where = {
[filter]: {
[Op.or]: {
[Op.eq]: new Date(filterValue),
[Op.gte]: new Date(filterFrom),
[Op.lte]: new Date(filterTo),
},
},
};
} else if (filterFrom) {
queryParams.where = {
[filter]: {
[Op.or]: {
[Op.eq]: new Date(filterValue),
[Op.gte]: new Date(filterFrom),
},
},
};
} else {
queryParams.where = {
[filter]: {
[Op.or]: {
[Op.eq]: new Date(filterValue),
[Op.lte]: new Date(filterTo),
},
},
};
}
} else if (filterValue) {
queryParams.where = { [filter]: { [Op.eq]: new Date(filterValue) } };
} else if (filterTo || filterFrom) {
if (filterTo && filterFrom) {
queryParams.where = { [filter]: { [Op.between]: [new Date(filterFrom), new Date(filterTo)] } };
} else if (filterFrom) {
queryParams.where = { [filter]: { [Op.gte]: new Date(filterFrom) } };
} else {
queryParams.where = { [filter]: { [Op.lte]: new Date(filterTo) } };
}
}
Код, получившийся после прочтения комментария.
if (filterValue && filterTo && filterFrom) {
return {
[filter]: {
[Op.or]: {
[Op.eq]: new Date(filterValue),
[Op.gte]: new Date(filterFrom),
[Op.lte]: new Date(filterTo),
},
},
};
}
if (filterValue && filterFrom) {
return {
[filter]: {
[Op.or]: {
[Op.eq]: new Date(filterValue),
[Op.gte]: new Date(filterFrom),
},
},
};
}
if (filterValue && filterTo) {
return {
[filter]: {
[Op.or]: {
[Op.eq]: new Date(filterValue),
[Op.lte]: new Date(filterTo),
},
},
};
}
if (filterValue) {
return { [filter]: { [Op.eq]: new Date(filterValue) } };
}
if (filterTo && filterFrom) {
return { [filter]: { [Op.between]: [new Date(filterFrom), new Date(filterTo)] } };
}
if (filterFrom) {
return { [filter]: { [Op.gte]: new Date(filterFrom) } };
}
if (filterTo) {
return { [filter]: { [Op.lte]: new Date(filterTo) } };
}
Ответы (1 шт):
У Вас, очевидно, проблема со стиллистикой кода. К сожалению, править его сейчас сложно, потому что неизвестна логика работы. Для проведения рефакторинга часто помогают тесты. Они играют роль тестовых данных: поменяли код и тесты не сломались, значит, скорее всего, всё идёт по плану.
Есть несколько простых правил.
Не писать больше 1 вложенного if. Их в 95% случаев можно развернуть в 2 независимых if. Для этого потребуется знание булевой алгебры для правки условий, а также умение писать функции. Пример с самой верхней вложенностью:
if (filterValue && (filterTo || filterFrom)) {
first_if(...)
} else if (filterValue) {
second_if(...)
} else if (filterTo || filterFrom) {
third_if(...)
}
Такой код намного проще воспринимать. Теперь, давайте разберёмся с получившейся структурой.
Следующее правило. Не писать else. Любое else порождает дополнительные блоки ветвления. А значит, дополнительные отступы и нелинейность. Это, как правило, усложняет логику и стиллистику. Любой отступ усложняет код. В данном случае, я не смогу изменить if так, чтобы не сломать логку, потому что я не знаю точно значений выражений. Но выглядит это могло бы, примерно так:
if (filterValue && (filterTo || filterFrom)) {
first_if(...)
return
}
if (filterValue) {
second_if(...)
return
}
if (filterTo || filterFrom) {
third_if(...)
return
}
Если после выполнения условий не нужен выход из функции, Вы можете вынести общую логику в отдельную функцию и вызывать её во всяком условии. Ещё одним вариантом будет написать взаимноисключающие условия. Здесь, разумеется, есть баланс сложности логики и его читаемости.
Аналогичным образом Вы можете поступить со всеми остальными условиями. Также, следует выносить большие блоки в отдельные функции. Например, этот:
{
[filter]: {
[Op.or]: {
[Op.eq]: new Date(filterValue),
[Op.gte]: new Date(filterFrom),
[Op.lte]: new Date(filterTo),
},
}
}
Не писать слишком большие функции (20-30 строк -- это максимум). Если Вы будете писать слишком большие функции, то число аргументов у такой функции будет расти. Это также приведёт к ухудшению читаемости. Кроме того, вряд ли Вы сможете точно сказать, чем занимается конкретная функция, что также не добавляет качества коду. Таким образом, следует контролировать число аргументов у функции. Если число аргументов большое (больше 2-3), то одним из возможных решений, может являться создание классов/структур, в которые инкапсулируют (скрывают) в себе часть дефолтных параметров. В таком случае, Вам не придётся их передавать в методы при их вызове.
Я бы вынес в отдельные функция следующие части:
(filterValue, filterTo) => {
[filter]: {
[Op.or]: {
[Op.eq]: new Date(filterValue),
[Op.lte]: new Date(filterTo),
},
},
},
Каждую из подобных функций можно вынести в отдельную функцию:
if (filterValue && filterTo && filterFrom) {
return C(filterValue, filterFrom, filterTo);
}
if (filterValue && filterFrom) {
return B(filterValue, filterFrom);
}
if (filterValue && filterTo) {
return A(filterValue, filterTo);
}
if (filterValue) {
return { [filter]: { [Op.eq]: new Date(filterValue) } };
}
if (filterTo && filterFrom) {
return { [filter]: { [Op.between]: [new Date(filterFrom), new Date(filterTo)] } };
}
if (filterFrom) {
return { [filter]: { [Op.gte]: new Date(filterFrom) } };
}
if (filterTo) {
return { [filter]: { [Op.lte]: new Date(filterTo) } };
}
Аналогично, можно вынести оставшиеся функциии. При этом, обязательно назовите их разумными именами. Далее, Вы можеет попробовать Ваши фильтры объединить в группы по смыслу и вынести в отдельные функции. Тогда, у Вас может получиться следующее:
result = f_1();
if (result.ok) {
return result.value;
}
result = f_2();
if (result.ok) {
return result.value;
}
Опять же, фильтры стоит называть разумными именами