1

Хочу попробовать написать данный алгоритм более кратко, но не знаю как. Буду рад любой помощи

Данный алгоритм добавляет условия в секцию 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

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;
}

Опять же, фильтры стоит называть разумными именами

2
  • Попробовал исправить в соответствии с Вашим комментарием. Не уверен, что повысилась читаемость. Например, я не совсем понял как мне вынести большие блоки в отдельные функции, если данный блок у меня постоянно меняется. Сейчас добавлю новый код в пост 2 мая 2020 в 7:47
  • @Dont_kill_me_pls добавил коммент в ответ. Не понял, что Вы имеете ввиду под "постоянно меняется блок" 2 мая 2020 в 8:00

Ваш ответ

By clicking “Отправить ответ”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Всё ещё ищете ответ? Посмотрите другие вопросы с метками или задайте свой вопрос.