Ответы пользователя по тегу Рефакторинг
  • Как отрефакторить такой код?

    0xD34F
    @0xD34F Куратор тега JavaScript
    Общую часть похожих кусков кода превращаем в функцию, а то, чем они различаются, будем передавать в эту функцию в качестве параметров:

    [
      { del: 'region_id', search: 'regionName' },
      { del: 'district_id', search: 'districtName' },
      { del: 'area_id', search: 'areaName' },
      { del: 'city_id', search: 'cityName' },
      { del: 'place_id', search: 'cityName' },
      { del: 'name', search: 'streetName' },
    ].forEach(n => {
      if (this.tableParam[n.del]) {
        delete this.tableParam[n.del];
        this.searchForm.get(n.search).patchValue(null);
      }
    });
    Ответ написан
    5 комментариев
  • Как еще можно оптимизировать js код?

    0xD34F
    @0xD34F Куратор тега JavaScript
    Исправил синтаксические ошибки.

    Ага, на пять с плюсом.

    • Пропущен if или условие лишнее:

      } else (date.getDay() == 0) {

      } else (a < 1) {

    • Лишнее : после ?:

      this.updateTime.getMonth() > 9 ? : '0'

    • Перед методом renderItems стоит лишняя фигурная скобка, а после метода createPeriod такой скобки не хватает.


    И это только синтаксические. Есть ещё ворох иных.

    • Имена свойств, содержащих обработчики событий, состоят из букв только в нижнем регистре, тут должно было быть input.onchange:

      this.input.onChange = this.onChange;

    • Какое там первое слово в имени базового класса должно быть - date или data? Вы бы определились:

      class dateInput {

      class dateRange extends dataInput {

    • В конструкторе dateRange отсутствует вызов конструктора базового класса - не хватает super(); перед this.container = ...:

      constructor() {
       this.container = document.querySelector('.containerForLastUpdateRecordAndPeriodItems');

    • Это ещё что за на хрен:

      period = `${date.toLocaleDateString()} - {date.setHours(168).toLocaleDateString()}`;

      Во-первых - не указан индекс, и вместо собираемого массива используется объект, переданный в функцию, вместо period должно быть periods[i].
      Во-вторых- метод setHours возвращает число, а не объект даты, так вызвать toLocaleDateString не получится.
      В-третьих - отсутствует $ перед фигурной скобкой при подстановке второй даты.

    • Присваивание значений необъявленным переменным:

      b = false;

      element = document.createElement('div')

      Никогда так больше не делайте. Ну и бегом гуглить, почему это плохо.

    • Неправильно пытаетесь вызывать метод renderItems - не хватает this.:

      onChange() {
       renderItems(this.createItems(this.createPeriod(this.inputValue)));

      Кроме того, this тут будет вовсе не экземпляром класса - надо привязывать контекст при установке этого метода в качестве обработчика события (this.onChange.bind(this) - это в конструкторе dateInput). А ещё, поскольку метод переопределён, надо не забыть вызвать метод базового класса, иначе не будет установлено значение свойства inputValue:

      onChange(e) {
        super.onChange(e);
        this.renderItems(...

    • Можете хоть из кожи вон вылезти, но константа своего значения не изменит, а вот TypeError при попытке выполнить хотя бы одну итерацию подобного цикла получите обязательно:

      for (const i = 0; i < n-a; i++) {

    • Что принимает document.createElement в качестве параметров? Откройте документацию и разберитесь, чтобы такую чушь больше не сочинять:

      const element = document.createElement('div', {textContent: item});

    • Опять проблемы со значением this:

      this.container.appendChild(element);

      Поскольку данная строка находится внутри коллбека forEach, который представлен обычной функцией, то экземпляр класса оказывается недоступен. Или замените обычную функцию на стрелочную, или передайте this в forEach третьим параметром.

    • Это ещё что за на хрен №2:

      createPeriod(date) {
       var newDate = date;
        newDate.year = newDate.year + 1;

      Во-первых - в метод передаётся строка, а не дата.
      Во-вторых - будь там дата, скопировать с помощью оператора присваивания её бы не получилось, для объектов копируются ссылки, а не значения.
      В-третьих - год получается и устанавливается совсем не так, для этого есть методы:

      createPeriod(dateStr) {
        const start = new Date(dateStr);
        const end = new Date(dateStr);
        end.setFullYear(end.getFullYear() + 1);
        return { start, end };
      }

    • Это ещё что за на хрен №3:

      const range = Object.create(dateRange.prototype); 
      range.constructor = range.constructor.bind(range); 
      range.constructor();

      Во-первых, без new конструктор вызывать нельзя.
      Во-вторых - к чему такие сложности? - достаточно new dateRange;.


    А уж что касается общей логики данного кода и насколько он соответствует поставленной задаче (о которой вы почему-то решили умолчать, так что здесь можно было бы говорить только в предположительном ключе), то я даже пытаться думать в эту сторону не буду. Хотя несомненно, что и тут всё очень-очень плохо - раз уж вы при написании кода накосячили как только можно и как нельзя, то почему на этапе проектирования должно было быть что-то принципиально иное?

    Подскажите, как еще можно оптимизировать(зарефакторить) данный js код?

    А зачем? Лучшее, что тут можно сделать - выбросить всё на хрен и попробовать написать заново, предварительно подумав, на этот раз головой вместо задницы. Не стыдно подобный мусор показывать?

    Но окей, окей - метод createItems, например:

    • Странные непонятные числа, встречающиеся в коде - такого не надо, определяем константы, имена которых будут указывать, что это за значения:

      const MS_IN_HOUR = 3600000;
      const HOURS = 168;

    • Вместо двух циклов сделайте один - промежуточный массив dates не нужен:

      for (let i = +period.start; i < +period.end; i += MS_IN_HOUR * HOURS) {
        const date = new Date(i);

    • "Оптимизируйте" if-else-if-... - складываем нижние значения часов в массив (тоже бы неплохо определить его отдельно, дав осмысленное имя), извлекаем их используя день недели как индекс, верхнее значение получаем добавляя сколько там надо (я так понял, разница везде одинаковая между нижним и верхним):

      const hours = [ -120, 0, -24, -48, -48, -72, -96 ][date.getDay()];
      periods.push([ hours, hours + HOURS ]
        .map(n => (date.setHours(n), date.toLocaleDateString()))
        .join(' - ')
      );

    • Для обмена значениями дополнительная переменная не нужна:

      [ periods[i + a], periods[i] ] = [ periods[i], periods[i + a] ];



    Или, renderItems:

    Четыре обращения к свойству updateTime - как-то многовато, достаточно одного, чтобы запомнить значение под коротким именем:

    const ut = this.updateTime;

    Вместо отдельных элементов можно вставлять разметку:

    this.container.insertAdjacentHTML('beforeend',
      `<div>Последнее изменение: ${ut.getDate()}.${`${ut.getMonth()}`.padStart(2, 0)}</div>` +
      items.map(n => `<div>${n}</div>`).join('')
    );
    Ответ написан
    3 комментария