Ревью кода: стилистика

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

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

Автор решения: hedgehogues

У Вас, очевидно, проблема со стиллистикой кода. К сожалению, править его сейчас сложно, потому что неизвестна логика работы. Для проведения рефакторинга часто помогают тесты. Они играют роль тестовых данных: поменяли код и тесты не сломались, значит, скорее всего, всё идёт по плану.

Есть несколько простых правил.

Не писать больше 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;
}

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

→ Ссылка