Читабельность кода

В процессе одного код ревью глаз зацепился за весьма своеобразный участок кода

findItem(id) { const items = [this.cars, this.yachts, this.flats]; return items.reduce((result, subItems) => { const found = subItems.find((item) => item.id === id); return found || result; }, null); }

В нем нет ничего особенного, но он сразу бросается в глаза и первое, что приходит в голову при его виде - этот код мне не нравится. Можно ли избавиться от вложенности. Зачем мы каждый раз делаем subItems.find, даже если элемент найден? Это не очень оптимально, надо исправлять:

findItem(id) { const items = [this.cars, this.yachts, this.flats]; return items.reduce((result, subItems) => { return result || subItems.find((item) => item.id === id); }, null); }

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

Плоское лучше, чем вложенное

дзен python

Раз уж от find избавиться не получается, то нужно пробовать избавиться от reduce. Линейный код ведь читать всегда проще.

findItem(id) { return this.getItems().find((item) => item.id === id); } getItems() { return [...this.cars, ...this.yachts, ...this.flats]; }

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

p.s.
А что по производительности? Spread-операция ведь не бесплатная.
Да, и бенчмарк это показал. Версия со spread оказалась на 80% медленнее.
И тут уже нужно смотреть на объем данных и частоту использования функции.

p.p.s.
В комментариях предлагайте ваши варианты улучшения

0
16 комментариев
Написать комментарий...
Igor Matyas

Почему бы не сделать:

return this.cars.find(i => i.id === id) ?? this.yachts.find(i => i.id === id) ?? this.flats.find(i => i.id === id) ?? null;

Никаких дополнительных массивов и функций не добавляется, код остается читаемым

И в последнем примере потерян null по дефолту если ничего не найдено, который был в самом начале

Ответить
Развернуть ветку
Game Topia

Вот как у вас и как у автора вообще не принято писать. Это факт. Не в одной нормальной компании так не пишут. Почему - потому что это не правильно. Те, кто говорит что не важно как выглядит код, главное чтобы он работал - работают в говностудиях в которых это действительно не важно. В нормальную компании такой код бы не прошел.

Хотя бывают исключения и для говнокода. Например сложные мат расчеты.

Ответить
Развернуть ветку
Igor Matyas

Не всегда стоит усложнять :) Я хотел сказать, что можно компактно написать такую же функцию без создания новых объектов в памяти

И смотрите:
1. У вас функция стала статической, хотя в примере такой не была
2. Вы ввели дженерики для этой самой функции, которые делают ее более общей, но и при этом усложняют чтение кода
3. Ну, и конверсия из списка аргументов в массив и потом еще вызов у него flat тоже не даются бесплатно

Ответить
Развернуть ветку
Game Topia

Не понятно, что в данном контексте означает слово "статическая", поскольку в отличие от оригинала она агрегирует массив (он не вживлен в нее статически), а кроме этого, поскольку она не является членом класса, то ее нельзя обозначить как статическая из-за невозможности применения к ней модификатора static. Поэтому я просто не понимаю, как интерпретировать ваше "стала статическая".

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

И дженерики вообще ничего не усложняют, к тому же там вообще два варианта кода на js и на ts. Единственное, что я только что заметил, это нормализатор строки форму сожрал параметры функции (сделал код неправильным). Получилось что тип Т нигде не определен.

Ответить
Развернуть ветку
Igor Matyas

В примере в функции использовалось ключевое слово this, поэтому логично было предположить, что это метод класса. "Статическая" - функция может быть вызвана без создания экземпляра этого класса.

Если сделать вашу функцию нестатической - не станет ли это тем же самым, что и автора в последнем примере?

По поводу производительности flat - не все так просто, особенно при больших массивах: https://stackoverflow.com/questions/61411776/is-js-native-array-flat-slow-for-depth-1

Ответить
Развернуть ветку
Game Topia

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

Да пофигу до производительности! Существуют узкие места программы и вот только на них стоит обращать внимание. К примеру не важно какая глубина и длина массивов, если операция выравнивания осуществляется только один раз перед циклом в котором будут обрабатываться его элементы. Замарачиватся на таком, это экономия на спичках о которой думают разработчики не дотягивающие даже до джуна.

Ответить
Развернуть ветку
Game Topia

function findItemById(id, ...collections) {
return collections
.flat()
.find( item => item.id === id ) ?? null;
}

findItemById(id, this.cars, this.yachts);

Ну или ts

interface IIdable {
id: string;
}

function findItemById(id: string, ...collections: T[][]): T | null {
return collections
.flat()
.find( item => item.id === id ) ?? null;
}

findItemById( `id`, this.cars, this.yachts );

Ответить
Развернуть ветку
Ovik Hachikyan

Если id разные, то обычно указывают uuid, чтобы было сразу понятно. Вместо костыля getItems можно спокойно брать готовое решение из lodash, потому что если проект большой, то точно где-то он уже используется, Тем более что готовое решение из lodash будет побыстрее обычного спреда и позволит засклейлится проще.

Ответить
Развернуть ветку
Сергей Галанин

Поддерживаю лодаш. В яваскрипте убогая стандартная библиотека, а лодаш — он такой семантичный и лаконичный, ммм, конфетка, а не код.

Ответить
Развернуть ветку
Ovik Hachikyan

Тут наверное вопрос в том, насколько большие эти сущности (cars, yachts…), ибо если они очень большие, то неплохо было бы добавить некого рода кеширование, чтобы лишний раз не складывать их, ибо такие операции на больший кусаках данных лучше на беке или даже в самой бд совершить. Если же объемы данных малы, то парится из-за скорости спреда не стоит

Ответить
Развернуть ветку
Сергей Галанин

Искать в массиве элемент с помощью перебора — не очень решение. Если один раз его вызываем или на массиве с маленьким количеством элементов — то ок, а если в цикле — будут тормоза. Я обычно, когда в цикле нужно находить элементы по айдишнику, сначала перекладываю их в объект с ключами-айдишниками, и тогда сложность резко падает с O(N) до O(1). Это не часто бывает нужно, но случается. Несколько раз удавалось таким образом ускорять алгоритм в 1000-10000 раз.

А так у Игоря нормальное решение в одну строку — лаконично и читаемо. Хотя многое зависит от контекста. Почему три массива разнородных объектов лежат в одном экземпляре? Может, правильнее их держать в трёх разных? Почему у нас есть айдишник объекта неизвестного нам типа? — то есть почему мы ищем в трёх массивах, а не в одном конкретном, по типу? Иногда вот так странный код одного метода может указать на слабые места в архитектуре.

Ответить
Развернуть ветку
Александр Ковалёв
Можно ли избавиться от вложенности.

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

Ответить
Развернуть ветку
Game Topia
В нем нет ничего особенного

Вы вообще о чем? Это просто срань во плоти, а не код. Я конечно понимаю, что при ревьювьер должен думать о чувствах того, чей код он проверяет, но... Это срань. Мало того, что он написан дебилом, так он ещё и не рабочий. Если вы найдете совпадение в первом массиве, то reduce все равно продолжит выполнение. Из этого выходит, что название не отражает сути. К тому же функция не чистая из-за мутабильности массивов. И кроме этого, агрегация предпочтительнее композиции. И в итоге у вас один говнокод превратился в другой говнокод. Да и БЕНЧМАРКИ для лошар. Вы из какого века сбежали?

Ответить
Развернуть ветку
Игорь Помилуйко
Автор

Код рабочий, просто я не стал приводить код всего класса, дабы акцентировать внимание только на конкретном методе.

Ответить
Развернуть ветку
Game Topia

Где же он рабочий? Если в cars и yachts будут элементы с одинаковым id, то что вернет функция названная findItem? Как вообще функция может называться findItem если она не работает с коллекцией?

Ответить
Развернуть ветку
Игорь Помилуйко
Автор

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

Ответить
Развернуть ветку
Читать все 16 комментариев
null