Минутка рефакторинга №1

Я тут подумал, что неплохо было бы раз в неделю-две делать публичное ревью кода, дабы делиться знаниями и практиковаться в улучшении кода. Я закинул удочку в Форвеб и первым прислать код на ревью не побоялся Сергей Мелодин, за что ему спасибо. Перейдём к ревью.

Задача:

Раз в t секунд последовательно выводить в консоль порции по n значений из массива, пока не будут выведены все элементы.

Исходное решение:

var myArr = ["Lorem", "ipsum", "dolor", "sit", "amet", "consectetur", "adipiscing", "elit", "sed", "do", "eiusmod", "tempor", "incididunt", "ut", "labore", "et", "dolore", "magna", "aliqua", "Ut", "enim", "ad", "minim", "veniam", "quis", "nostrud", "exercitation", "ullamco", "laboris", "nisi", "ut", "aliquip", "ex", "ea", "commodo", "consequat"];
var i = myArr.length;
var x = 0;

var step = 10; // шаг смещения

function offset(x){
var y = x;
  while(x<y+step && x<i){
    console.log(myArr[x]);
    x++;
  };
};

function start(){
  offset(x);
  console.log('pause');
  x = x+step;
  if(x>i){ clearInterval(timer) };
};

var timer = setInterval(start, 1000);

Начнём с того, что тестовые данные можно сгенерировать динамически, благо значения элементов массива нам не важны:

// было
var myArr = ["Lorem", "ipsum", "dolor", "sit", "amet", "consectetur", "adipiscing", "elit", "sed", "do", "eiusmod", "tempor", "incididunt", "ut", "labore", "et", "dolore", "magna", "aliqua", "Ut", "enim", "ad", "minim", "veniam", "quis", "nostrud", "exercitation", "ullamco", "laboris", "nisi", "ut", "aliquip", "ex", "ea", "commodo", "consequat"];

// стало
var myArr = Array.from({ length: 36 }, (value, index) => index);

Причёсываем код для лучшей читаемости (исправляем отступы и добавляем недостающие пробелы). Выносим интервал в отдельную переменную для гибкости. Интервал и количество выводимых значений за одну итерацию обозначаем прописными буквами, как константы. Количество выводимых за одну итерацию значений при этом переименовываем из step в ITEMS_PER_ITERATION. Заменяем var на const или let в зависимости от того, переприсваивается ли переменная в дальнейшем. Убираем точки с запятой после объявления функций, циклов и условий, они там не нужны. Переименовываем timer в intervalId, что точнее отражает суть значения. Избавляемся от микрооптимизации в виде сохранения длины myArr в переменную i — во-первых, название переменной никак не отражает её суть, во-вторых, преждевременная оптимизация лишь портит читаемость и усложняет код:

const ITEMS_PER_ITERATION = 10;
const INTERVAL = 1000;

const intervalId = setInterval(start, INTERVAL);
const myArr = Array.from({ length: 36 }, (value, index) => index);
let x = 0;

function offset(x) {
  const y = x;

  while(x < y + ITEMS_PER_ITERATION && x < myArr.length){
    console.log(myArr[x]);
    x++;
  }
}

function start() {
  offset(x);
  console.log('pause');

  x = x + ITEMS_PER_ITERATION;

  if (x > myArr.length) {
    clearInterval(intervalId);
  }
}

Сейчас есть две функции start() и offset(), с первого взгляда непонятно, что они делают. Самое ужасное — они обе изменяют переменную x. Но не всё так просто! На первый взгляд может показаться, что они изменяют одну и ту же переменную. Однако в функции offset() x — это аргумент, и он создаёт локальную переменную x. Такие переменные называются связанными — они непосредственно связаны с функцией, в которой они используются. Если бы у offset() не было аргументов, мы бы работали с внешней переменной x. Если переменная не связана, она называется свободной.

Итак, избавимся от излишней сложности со свободными и связанными переменными. Функция offset() по сути должна выводить в консоль ITEMS_PER_ITERATION элементов массива. Изменим её сигнатуру — аргумент x заменим на array, а саму функцию переименуем в logArrayItems. Ну и исправим функцию start(), чтобы она правильно работала с logArrayItems:

const ITEMS_PER_ITERATION = 10;
const INTERVAL = 1000;

const intervalId = setInterval(start, INTERVAL);
const myArr = Array.from({ length: 36 }, (value, index) => index);
let x = 0;

function logArrayItems(array) {
  array.forEach((item) => console.log(item));
}

function start() {
  logArrayItems(myArr.slice(x, x + ITEMS_PER_ITERATION));
  console.log('pause');

  x = x + ITEMS_PER_ITERATION;

  if (x > myArr.length) {
    clearInterval(intervalId);
  }
}

Теперь выглядит попроще, но всё далеко от идеала. Наше решение не выглядит целостным. Чтобы это исправить, обернём его в функцию logArrayItemsSequentially, которая будет принимать массив, элементы которого надо вывести в консоль, и настройки — количество выводимых за итерацию элементов и интервал в миллисекундах. Хорошей практикой считается обязательные параметры передавать в функцию как есть, а все опциональные параметры передавать в виде объекта (при этом опциональным параметрам нужно задать значения по умолчанию). Это позволяет сократить код при обычном использовании функции, но не потерять гибкость и оставить возможность переопределения настроек:

function logArrayItemsSequentially(array, options = {}) {
  const {
    itemsPerIteration = 10,
    interval = 1000,
  } = options;
  const intervalId = setInterval(start, interval);
  let x = 0;

  function logArrayItems(array) {
    array.forEach((item) => console.log(item));
  }

  function start() {
    logArrayItems(array.slice(x, x + itemsPerIteration));
    console.log('pause');

    x = x + itemsPerIteration;

    if (x > array.length) {
      clearInterval(intervalId);
    }
  }
}

const sampleArray = Array.from({ length: 36 }, (value, index) => index);

logArrayItemsSequentially(sampleArray/*, здесь могли быть переопределённые настройки */);

Отлично, теперь у нас есть целостное решение — функция, которую можно даже опубликовать на NPM (но лучше не надо). Снаружи её использование выглядит просто и понятно. Наведём порядок изнутри.

Для начала избавимся от функции logArrayItems — она используется лишь в одном месте:

// было
function logArrayItems(array) {
  array.forEach((item) => console.log(item));
}

function start() {
  logArrayItems(array.slice(x, x + itemsPerIteration));
  // ...
}

// стало
function start() {
  array.slice(x, x + itemsPerIteration).forEach((item) => console.log(item));
  // ...
}

У нас всё ещё остаётся изменяемая переменная-счётчик, что не очень хорошо. Изменение значений переменных всегда усложняет чтение и понимание кода, а в некоторых случаях усложняет тестирование.

Давайте рассмотрим суть нашего алгоритма. Наш алгоритм работает итеративно — каждый раз он выводит в консоль порцию массива и переходит к остальным элементам, повторяя те же действия, пока все элементы массива не будут выведены на экран. Сейчас такое поведение реализовано с помощью setInterval и счётчика x, указывающего на индекс первого не выведенного в консоль элемента массива.

Такое же поведение мы можем реализовать намного проще. Нам не нужны вложенные функции, счётчики и setInterval — нам нужны рекурсия и setTimeout! Тело функции logArrayItemsSequentially будет обрабатывать только одну итерацию, а переход к следующей итерации будет делаться рекурсивным вызовом logArrayItemsSequentially. Разберём решение:

function logArrayItemsSequentially(array, options = {}) {
  const {
    itemsPerIteration = 10,
    interval = 1000,
  } = options;
  // элементы, которые нужно вывести в консоль на текущей итерации
  const currentPortion = array.slice(0, itemsPerIteration);
  // оставшиеся элементы
  const tail = array.slice(itemsPerIteration);

  // сразу делаем вывод в консоль
  currentPortion.forEach((item) => console.log(item));
  console.log('pause');

  // если остаток не пустой, передаём его в следующую итерацию,
  // выполнение которой откладываем с помощью setTimeout
  if (tail.length) {
    setTimeout(() => logArrayItemsSequentially(tail, options), interval);
  }
}

const sampleArray = Array.from({ length: 36 }, (value, index) => index);

logArrayItemsSequentially(sampleArray/*, здесь могли быть переопределённые настройки */);

На мой взгляд, на этом можно остановиться. Мы получили универсальное и простое решение. Для наглядности приведу исходное и отрефакторенное решения:

/**
 * Исходное решение
 */

var myArr = ["Lorem", "ipsum", "dolor", "sit", "amet", "consectetur", "adipiscing", "elit", "sed", "do", "eiusmod", "tempor", "incididunt", "ut", "labore", "et", "dolore", "magna", "aliqua", "Ut", "enim", "ad", "minim", "veniam", "quis", "nostrud", "exercitation", "ullamco", "laboris", "nisi", "ut", "aliquip", "ex", "ea", "commodo", "consequat"];
var i = myArr.length;
var x = 0;

var step = 10; // шаг смещения

function offset(x){
var y = x;
 while(x<y+step && x<i){
   console.log(myArr[x]);
   x++;
 };
};

function start(){
	offset(x);
 console.log('pause');
 x = x+step;
 if(x>i){ clearInterval(timer) };
};

var timer = setInterval(start, 1000);

/**
 * Отрефакторенное решение
 */

function logArrayItemsSequentially(array, options = {}) {
  const {
    itemsPerIteration = 10,
    interval = 1000,
  } = options;
  const currentPortion = array.slice(0, itemsPerIteration);
  const tail = array.slice(itemsPerIteration);

  currentPortion.forEach((item) => console.log(item));
  console.log('pause');

  if (tail.length) {
    setTimeout(() => logArrayItemsSequentially(tail, options), interval);
  }
}

const sampleArray = Array.from({ length: 36 }, (value, index) => index);

logArrayItemsSequentially(sampleArray/*, здесь могли быть переопределённые настройки */);
Share
Send
7 comments
Сергей Мелодин

Спасибо за столь подробную статью! Рефакторинг показал не только интересное решение поставленной задачи, но и помог сформулировать для себя новые. Отмечу прекрасный стиль изложения – всё доходчиво и по делу. Думаю, это получится весьма полезная рубрика и потому желаю автору дальнейших успехов!

Huckleberry Ogre

“Убираем точки с запятой после объявления функций, циклов и условий, они там не нужны.” Разве нельзя придумать (пусть даже искуственный) пример того, где это может вызвать ошибку?

Андрей Романов

Huckleberry Ogre,

«Убираем точки с запятой после объявления функций, циклов и условий, они там не нужны.» Разве нельзя придумать (пусть даже искуственный) пример того, где это может вызвать ошибку?

А зачем придумывать для этого пример? Там просто не нужны точки с запятой, они обычно ставятся после выражений, а не после блоков кода.

Даниил

Отличный формат. С удовольствием буду читать такое ищо. =)

P.S. В отрефакченном варианте `console.log(‘pause’)` потеряли по ходу чистки. =)

Андрей Романов

Даниил, вернул потерянный console.log, спасибо!

Максим

Спасибо, замечательная рубрика!

Азат

Можно ещё здесь опустить круглые скобки в списке аргументов:

currentPortion.forEach=> console.log(item;

currentPortion.forEach(i => console.log(i))

И точки запятой тут не нужны нигде

Андрей Романов

Азат, а ещё можно все переменные сделать однобуквенными, ах да, и отступы с переносами строк тут не нужны нигде, можно в одну строчку всё написать :–)

Азат

Это уже задача минификатора, не придирайся )

Ivan Zimin

Хорошо было бы в конце каждой такой статьи проводить бенчмарк.
Скорость выполнения при 10 000 повторениях, к примеру.

Your comment
won’t be published

HTML will not work

Ctrl + Enter
Popular