Больше одного знания и одной сущности в методе показа выбранного месяца


Исходный код

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), потому что правильнее будет решить проблему с лишним месяцем в другом месте — там, где он собственно появляется (то есть сделать так, чтобы он вообще не появлялся, раз он не нужен).