Исходный код
showMonth(e) {
const month = this.visualState || e.target.parentNode.dataset.month || e.target.dataset.month;
this.visualState = month;
if (month) {
const monthList = document.querySelectorAll('.ta-calendar-aviasales__month-list-month');
monthList.forEach((item, index, arr) => {
item.style.display = 'none';
<em>// index !== arr.length - 1 - что бы не показать 2 раза первый месяц за текущий и следующий год</em>if (item.dataset.month === month && (monthList.length <= 12 || index !== arr.length - 1))
item.style.display = 'block';
});
this.$monthContainer.style.display = 'block';
this.$yearList.style.display = 'none';
}
}
Что не так в этом коде
- нарушен принцип единственного знания: в одном методе сосредочена и обработка события, и запоминание состояния, и визуализация разных объектов
- нарушен принцип единственной сущности (здесь две сущности: календарь месяцев в целом и отдельный месяц)
- присваивание дефолтного значения с последующим перезатиранием
- неоптимальное распределение операций между частями связанного кода
- неудачное название поля
visualState
: непонятно, что оно хранит текущий выбранный месяц - неудачное название параметра
arr
: непонятно, массив чего в нём находится
Как можно отрефакторить исходный код
Код для работы с отдельными месяцами убираю в отдельный класс. Так как месяцы здесь — это просто узлы в DOM, а заводить для каждого узла по отдельному экземпляру нового класса достаточно хлопотно, то оптимальнее всего будет в этом классе создать статические методы для работы с месяцем, передаваемым через параметры.
Здесь я фактически вернулся к стилю процедурного программирования, а класс использовал просто для группировки методов.
classMonthNode {
staticshowByName(month_name) {
this.monthList().forEach(month_node =>this.showNode(month_node, this.nodeName(month_node) === month_name));
}
staticmonthList() {
returndocument.querySelectorAll('.ta-calendar-aviasales__month-list-month');
}
staticnodeName(month_node) {
return month_node.dataset.month;
}
staticshowNode(month_node, to_show) {
month_node.style.display = to_show ? 'block' : 'none';
}
}
Обрати внимание, как знания поделены между крохотными методами: первый знает, как показать только один месяц в целом наборе месяцев, второй — как хранится набор месяцев, третий — как организовано хранение названия метода в узле DOM (в каком именно data-атрибуте), четвёртый — как организовано скрытие и показ блока-месяца (через свойство display).
Теперь нужно разделить обработку события выбора месяца, собственно выбор месяца и переключение на блок с месяцем — это знания о разных вещах, поэтому должны быть заключены в разные методы:
onMonthClick(event) {
const chosen_month_name = MonthNode.nodeName(event.target.parentNode) || MonthNode.nodeName(event.target);
this.setCurrentMonth(chosen_month_name);
this.showMonth();
}
setCurrentMonth(month_name) {
if (this.currentMonthName !== month_name) {
this.currentMonthName = month_name;
MonthNode.showByName(month_name);
}
}
showMonth() {
this.$monthContainer.style.display = 'block';
this.$yearList.style.display = 'none';
}
Код, который проверяет наличие названия месяца, я выпилил, потому что сложившийся набор методов не предусматривает отсутствия названия.
Также я убрал проверку (monthList.length <= 12 || index !== arr.length - 1)
, потому что правильнее будет решить проблему с лишним месяцем в другом месте — там, где он собственно появляется (то есть сделать так, чтобы он вообще не появлялся, раз он не нужен).