Используйте самые простые инструменты для более простого и удобного в сопровождении кода.

Когда я рецензирую код, мне нравится внимательно изучать любое использование reduce. Напоминаем, что reduce (или fold, или catamorphism, если вы очень любите) позволяет вам взять повторяемый объект, например массив, и создать из него некоторые другие данные.

Если вы не видели его раньше, вот документация JavaScript MDN для него.

Заманчиво воспользоваться самым мощным инструментом из имеющихся, а reduce - действительно мощный инструмент. За такую ​​большую мощность приходится платить. Чем сложнее ваш код, тем выше вероятность, что в нем есть ошибка. Читать сложнее, чем более простые функции. Хуже всего то, что это создает очевидную точку расширения, которой легко злоупотребить.

Не будь глупцом; используйте более простой инструмент

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

function getAllowedItems(items, points, denyList) {
    const uuidsByValues = items.reduce((ret, item, idx) => {
        const point = points[idx]
        ret[item.values] = point
            .reduce((uuids, p) => {
                uuids.push(p.uuid)
                return uuids
            }, [])
            .reverse()
        return ret
    }, {})
    return items.reduce((allowed, item) => {
        const uuids = uuidsByValues[item.values]
        if (!uuids.some((uuid) => denyList.includes(uuid))) {
            allowed.push(item)
        }
        return allowed
    }, [])
}

Мы собираемся провести рефакторинг этого кода с целью повышения читабельности без изменения какого-либо поведения.

Это означает, что любые ошибки, присутствующие в оригинале, должны присутствовать в конечном результате; мы не собираемся добавлять null проверок или проверок за выходом за границу в этом проходе. Возможно, этот код никогда не потребует их, так что давайте не будем выполнять ненужную работу.

Не делай работы, которая нам не нужна

Прежде всего, что делает этот код? Быстрое сканирование предлагает следующее:

  • имя может предполагать, что он возвращает все разрешенные элементы из некоторого ввода.
  • аргументы показывают, что он принимает предметы, список запрещенных предметов и, возможно, какой-то механизм подсчета очков? Непонятно, что такое points.
  • оператор return allowed поддерживает понятие разрешенного списка; наличие reduce говорит о том, что это сокращение входного списка (хотя reduce, безусловно, можно использовать для создания более крупной структуры, например: xs.reduce((acc, x) => { acc.push(x) ; acc.push(x) ; return acc }, []))

Это быстрое сканирование уже ввело нас в заблуждение; в этом коде нет разрешенного списка.

Именование - сложная проблема, поэтому мы оставим ее на потом в цикле рефакторинга. Надеюсь, по ходу дела нам на ум придут какие-то очевидные имена.

Тем не менее, есть несколько стандартных имен, которые мы можем применить, чтобы помочь нам понять код. У некоторых функций есть шаблоны именования, такие как вызов аккумулятора при уменьшении acc. Используя общие имена, мы оставляем подсказки другим разработчикам об использовании и объеме переменных в нашем коде.

Давайте сосредоточимся на первом операторе и применим acc переименование:

const uuidsByValues = items.reduce((outerAcc, item, idx) => {
    const point = points[idx]
    outerAcc[item.values] = point
        .reduce((innerAcc, p) => {
            innerAcc.push(p.uuid)
            return innerAcc
        }, [])
        .reverse()
    return outerAcc
}, {})

Это совсем не улучшило код, но мы проинформировали нас в нашей роли рефакторинга, что эти переменные имеют локальную область видимости. Внутренний reduce очень прост; он выполняет только одну операцию, а второй аргумент - это пустая версия повторяемого объекта. Это говорит о том, что мы могли бы заменить его более простой функцией.

Результатом этого кода является массив той же длины, что и входной, содержащий разные данные. Это map, который является частным случаем reduce. Вы можете достичь array.map(p) с array.reduce((acc, x) => { acc.push(p(x)) ; return acc; }, []). Мы можем применить обратное так:

outerAcc[item.values] = point.map((p) => p.uuid)

Теперь мы можем переименовать outerAcc:

const uuidsByValues = items.reduce((acc, item, idx) => {
    const point = points[idx]
    acc[item.values] = point.map((p) => p.uuid).reverse()
    return acc
}, {})

Мы не можем упростить оставшийся reduce здесь, поскольку он создает хеш; мы могли бы встроить const, но мы не получаем от него особой читабельности, и мы теряем очевидную точку расширения, чтобы добавить null проверку позже.

Переходя к оператору return, вот что у нас сейчас есть:

return items.reduce((allowed, item) => {
    const uuids = uuidsByValues[item.values]
    if (!uuids.some((uuid) => denyList.includes(uuid))) {
        allowed.push(item)
    }
    return allowed
}, [])

Мы можем использовать аналогичную логику для замены reduce. filter - это частный случай reduce; вы можете достичь arr.filter(p) с arr.reduce((acc, x) => { if (p(x)) { acc.push(x) } ; return acc; }, []). Мы можем применить обратное так:

return items.filter((item) => {
    const uuids = uuidsByValues[item.values]
    const p = uuids.some((uuid) => denyList.includes(uuid))
    return !p
})

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

function getAllowedItems(items, points, denyList) {
    const uuidsByValues = items.reduce((acc, item, idx) => {
        const point = points[idx]
        acc[item.values] = point.map((p) => p.uuid).reverse()
        return acc
    }, {})
    return items.filter((item) => {
        const uuids = uuidsByValues[item.values]
        const p = uuids.some((uuid) => denyList.includes(uuid))
        return !p
    })
}

Это гораздо более читабельно, чем оригинал. Мы видим, что мы создаем хэш значений, а затем удаляем элементы на основе списка запрещенных идентификаторов, используя этот хеш. Мы дважды перебираем массив items; возможно, мы сможем объединить эти операции во что-нибудь попроще.

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

function getAllowedItems(items, points, denyList) {
    return items.filter((item, idx) => {
        const point = points[idx]
        const uuids = point.map((p) => p.uuid).reverse()
        const p = uuids.some((uuid) => denyList.includes(uuid))
        return !p
    })
}

Теперь мы полностью удалили хэш. Нам больше не нужны reduce; немного встраивания и переименования сделают этот код кристально понятным:

function removeDeniedItems(items, points, denyList) {
    return items.filter((item, idx) => {
        const uuids = points[idx].map((p) => p.uuid)
        const isDenied = uuids.some((uuid) => denyList.includes(uuid))
        return !isDenied
    })
}

Этот код легче читать; его меньше, и он использует более простые функции. Он также работает лучше, только один раз перебирает входные данные.

Однако я сказал, что мы собираемся провести чистый рефакторинг, и мы потерпели неудачу на этом фронте.

Интересно, что отредактированный код вообще не полагается на значение item. Раньше мы использовали item.values в качестве ключа; когда мы объединили два оператора вместе, мы неявно предполагали, что values уникальны для каждого элемента. Если это правда, то это изменение - хорошо; давайте посмотрим, что было бы в противном случае.

const uuidsByValues = items.reduce((acc, item, idx) => {
    acc[item.values] = points[idx].map((p) => p.uuid).reverse()
    return acc
}, {})

Старое поведение заключалось в переопределении предыдущего значения в хэше, что означало выбор неправильных uuid из хеша в фильтре. В свою очередь, это привело бы к неправильному включению или исключению элементов из результата.

Мы можем проверить это с помощью теста:

const items = [{ values: 'some-value' }, { values: 'some-value' }]
const points = [[{ uuid: 'id01' }], [{ uuid: 'id02' }]]
describe('with first item denied', () => {
    const denyList = ['id01']
    test('original has two items', () => {
        const output = getAllowedItems(items, points, denyList)
        assert(output.length === 2)
    })
    test('refactored has one item', () => {
        const output = removeDeniedItems(items, points, denyList)
        assert(output.length === 1)
    })
})
describe('with second item denied', () => {
    const denyList = ['id02']
    test('original has no items', () => {
        const output = getAllowedItems(items, points, denyList)
        assert(output.length === 0)
    })
    test('refactored has one item', () => {
        const output = removeDeniedItems(items, points, denyList)
        assert(output.length === 1)
    })
})

Я считаю, что отредактированное поведение правильное, но оно, безусловно, будет зависеть от вашего варианта использования. Итак, я могу сказать, что, устранив зависимость от данных на values, мы устранили ошибку.

В более коротком и понятном коде меньше мест, где можно спрятать ошибки.

Результатом я вполне доволен, но осталась одна мелочь. Использование индекса массива idx является опасным моментом для возможного сбоя. Если массив будет отсортирован или будет добавлен элемент, что весьма вероятно, тогда наш код завершится ошибкой.

В исходной версии этого кода этой проблемы не существовало; points был получен напрямую и асинхронно из items, поэтому порядок индекса всегда соблюдался. Когда я извлек и запутал этот код, я обнаружил проблему. Это выходит за рамки того, что я рассматриваю здесь, поэтому я проигнорирую это.

Но стоит отметить.

Еще раз для сравнения:

До:

function getAllowedItems(items, points, denyList) {
    const uuidsByValues = items.reduce((ret, item, idx) => {
        const point = points[idx]
        ret[item.values] = point
            .reduce((uuids, p) => {
                uuids.push(p.uuid)
                return uuids
            }, [])
            .reverse()
        return ret
    }, {})
    return items.reduce((allowed, item) => {
        const uuids = uuidsByValues[item.values]
        if (!uuids.some((uuid) => denyList.includes(uuid))) {
            allowed.push(item)
        }
        return allowed
    }, [])
}

После:

function removeDeniedItems(items, points, denyList) {
    return items.filter((item, idx) => {
        const uuids = points[idx].map((p) => p.uuid)
        const isDenied = uuids.some((uuid) => denyList.includes(uuid))
        return !isDenied
    })
}

Намного лучше!