Качество кода — тема, которая родилась вместе с программированием. Для оценки и контроля качества менеджмента предприятий применяется ISO 9000, для продуктов — ГОСТ и тот же ISO, а вот для оценки качества кода ГОСТа нет. Точного определения и стандарта для качества кода тоже нет.



Каждый разработчик понимает качество по-своему, исходя из опыта. Представления джунов и лидов различаются, и это приводит к разногласиям. Каждая команда для отдельных проектов оценивает код по-своему. Команда обновляется, разработчики уходят, тимлиды сменяются — определение качества меняется. Эту проблему попробует помочь решить Иван Ботанов (StressoID) из Tinkoff.ru — Frontend-developer, преподаватель онлайн-курса по Angular, спикер на митапах и конференциях, ведущий уроков на YouTube и, иногда, тренер команд в компаниях.

В расшифровке доклада Ивана на Frontend Conf поговорим о читаемости, нейминге, декларативности, Code style, и косвенно коснемся отношений джунов и лидов: ошибки, грабли и «сгорание» тимлидов.

Disclaimer: Подготовьтесь морально, в тексте будет много плохого кода, взятого с «особенного» сайта.


Немного истории


Все мы пишем по-разному. Вы могли заметить это, когда меняли место работы, проект или команду — непривычные вещи сразу бросаются в глаза. Со мной такое тоже было много раз, поэтому и родился этот доклад. Я ориентировался на начинающих разработчиков, но статья будет полезна и тем, кто уже больше управляет, чем пишет, или настраивает процессы разработки. Про что будем говорить:

  • Про проблемы нечитаемого кода.
  • Обсудим нейминги.
  • Посмотрим, в чем отличия декларативного и императивного стиля, и в чем проблемы.
  • Про модульность кода и типизацию.
  • Про Code style и техдолг, про коммиты и git flow.
  • Про инструменты, которые можно использовать, и фиксацию ошибок в проде.

Прежде, чем начнем, задам вопрос: «Сколько программистов должен сбить автобус, чтобы проект перестал развиваться?». Правильный ответ: всех.

Что такое Bus-Factor?


Условно, в команде работает Петя или Вася, который знает всё про проект: к нему ходят, спрашивают про то и это, как здесь работает, а как там. От Пети все зависят, автобусное число проекта — единица. Чем меньше число, тем сложнее развивать проект, потому что все отвлекают Петю, а он — крутой, и должен делать задачки, а не на вопросы отвечать.

Вы скажете, как круто быть Петей! Его все любят, ценят, в нем нуждаются.

Это не так. Обычно Петя — это тимлид, и должен заниматься другими задачами: обсуждать развитие проекта, выстраивать архитектуру, управлять командой, но никак не ходить к джунам и объяснять, почему здесь написано так, а не иначе.

Если код чистый и хороший, то его приятно читать и к нему возникает меньше вопросов от джунов. Чистый код повышает жизнеспособность проекта и снижает порог вхождения. Когда в команде появятся новые люди, они будут задавать меньше вопросов. В такой проект проще привлекать разработчиков за счет низкого порога вхождения.

Качественный код повышает автобусное число.

Читаемость


Читаемость ухудшают отступы, кривые нейминги и сильная вложенность — этим страдают многие проекты. Кроме отступов читаемость снижают многострочные тернарные операторы, отсутствие единого Code style, комбинация подходов разработки и неочевидное переопределение переменных. Все это — самые частые причины плохой читаемости кода.

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

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

Пример такого кода:

list.forEach((element1) => {
    if (element1.parent_id == null) {
        output.push(element1);
        list.forEach((element2) => {
            if (element2.parent_id == element1.id) {
                output.push(element2);
                list.forEach((element3) => {
                    if (element3.parent_id == element2.id) {
                        output.push(element3);
                        list.forEach((element4) => {
                            if (element4.parent_id == element3.id) {
                                output.push(element4);
                            }
                        })
                    }
                })
            }
        })
    }
})

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

Пример нелинейного кода:

if(!brk && html.childNodes[0].value && html.childNodes[0].max) {
    if(clear)
        html.childNodes[0].value = 1;
    else
    if(html.childNodes[0].value <= html.childNodes[0].max) {
        ++ html.childNodes[0].value;
        if(brk) {
            for(id = 1; id < html.childNodes.length; ++ id)
                findActive(html.childNodes[id], true);
            html.parentNode.className = "";
        }
        return null;
    } else {
        html.parentNode.className = "Ready";
        html.className = "";
        return html;
    }
}

Если отбросить все лишнее и разобраться, что ломает линейность, то увидим что-то подобное:

if (condition) {
    return;
} else {
    return;
}

Когда есть else, сначала приходится смотреть, что написано в одном месте, потом в другом. Если это большой или сильно вложенный if, то внимание рассеивается, и код тяжело читать.

Как снизить вложенность и добиться линейности кода?


Объединить условия. Это самое простое, что мы можем сделать — вложенные if я могу объединить в condition и немного снизить вложенность.

Было:

if (isUser()) {
    if (isAdmin()) {
        console.log('admin');
    }
}

Стало:

if(isUser() && isAdmin()) {
    console.log('admin');
}

Применить паттерн раннего return. Он позволяет насовсем избавиться от else. Метод или кусок кода с if-else я могу заменить на ранний return, и тогда выполнится либо один блок кода, либо другой. Это очень удобно — не приходится скроллить и возвращаться к каким-то участкам кода.

Было:

if (isAdmin()) {
    return admin;
} else {
    return user;
}

Стало:

if (isAdmin()) {
    return admin;
}

return user;


Применить promise chain. Это типичный Protractor-код. Я не так давно писал E2E-тесты, и такой код резал глаза:

ptor.findElement(protractor.By.id('q01_D')).click().then(() => {
    ptor.findElement(protractor.By.id('q02_C')).click().then(() => {
        ptor.findElement(protractor.By.id('q03_D')).click().then(() => {
            console.log('done');
        });
    });
})

С promise chain код превращается:

ptor.findElement(protractor.By.id('q01_D')).click()
    .then(() => {
        return ptor.findElement(protractor.By.id('q02_C')).click();
    })
    .then(() => {
        return ptor.findElement(protractor.By.id('q03_D')).click();
     })
    .then(() => {
        console.log('done'); 
    });


Если воспользуемся знанием стрелочной функции, то можем сделать так:

ptor.findElement(protractor.By.id('q01_D')).click()
    .then(() => ptor.findElement(protractor.By.id('q02_C')).click())
    .then(() => ptor.findElement(protractor.By.id('q03_D')).click())
    .then(() => console.log('done'));

Это читаемо, декларативно, красиво — все хорошо видно.

Higher order observable. Так как я ангулярщик и использую RxJS, то сталкиваюсь с проблемой сильной вложенности спагетти-кода — вложенные Observable, то есть вложенные подписки. Есть поток, и внутри потока надо получить значение и дальше что-то совершить с другим потоком. Некоторые пишут так:

Observable.of(1,2,3)
    .subscribe(item => {
        item += 2;
        Observable.of(item)
            .subscribe(element => {
                element += 1;
            })
    })

Причем этим страдают действительно взрослые проекты. Можно сделать так:

Observable.of(1,2,3)
    .mergeMap(item => Observable.of(item + 2))
    .mergeMap(element => Observable.of(element + 1))
    .subscribe()

Применив знание API RxJS, мы ушли от сильной вложенности, благодаря higher order observable, и пришли к декларативности. Эта штука, которая прокидывает значение внутреннего потока во внешний — вот и все. Зато чисто, линейно, красиво и не вложено.

Вложенные тернарные операторы


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

Пример вложенных тернарных операторов и неявных условий:

arr.length > 0 ? arr[1] == 1 ? arr[1] = 2 : arr[1] = 1 : console.log('empty arr');

!a && b && func()

Этот простой тернарник я написал за 5 минут. В нем есть длина массива и некоторые операции, но его тяжело прочитать, потому что где-то один вопрос, где-то другой — все непонятно. Этот код можно переписать, используя многострочные вложенные тернарные операторы:

arr.length > 0 ?
    arr[1] === 1 ?
        arr[1] = 2
    : arr[1] = 1
: console.log('empty arr');

Вы скажете:

— ОК, нормально!
— Видно?
— Видно!

А что вы скажете на это:

return query instanceof RegExp ?
    (function () {
fn.each(function (id) {
    if (id.match(query)) {
seatSet.push(id, this);
    }
});
return seatSet;
    })() :
    (query.length == 1 ?
(function (character) {
    //user searches just for a particual character
    fn.each(function () {
if (this.char() == character) {
    seatSet.push(this.settings.id, this);
}
    });
        return seatSet;
})(query) :
        (function () {
            //user runs a more sophisticated query, so let's see if there's a dot
    return query.indexOf('.') > -1 ?
        (function () {
            //there's a dot which separates character and the status
                var parts = query.split('.');

                fn.each(function (seatId) {
                    if (this.char() == parts[0] && this.status() == parts[1]) {
                        seatSet.push(this.settings.id, this);
                    }
                });

                return seatSet;
        })() :
            (function () {
                fn.each(function () {
                    if (this.status() == query) {
                        seatSet.push(this.settings.id, this);
                    }
                });
            return seatSet;
        })();
    })()
);

Кто-то написал и поддерживает этот тернарник. Зайдя на такой участок кода, будет тяжело разобраться, где знак вопроса начинается, а где заканчивается. Если вы используете тернарники, пожалуйста, не вкладывайте один в другой — это плохо.

Нейминги


Еще один плохой код:

var _0x30119c = function() {
    var _0x3af68e = {
        'data': {
            'key': 'cookie',
            'value': 'timeout'
         },
         'setCookie': function(_0x3543f3, _0x13e5c1, _0x586dac, _0x1c9d63) {
             _0x1c9d63 = _0x1c9d63 || {};
             var _0x47b83f = _0x13e5c1 + '=' + _0x586dac;
             var _0xae3be = 0x0;
             for (var _0xae3be = 0x0, _0x5d2845 = _0x3543f3['length'];
                 _0xae3be < _0x5d2845; _0xae3be++) {
                 var _0x440369 = _0x3543f3[_0xae3be];
                 _0x47b83f += ';\x20' + _0x440369;
                 var _0x411875 = _0x3543f3[_0x440369];
                 _0x3543f3['push'](_0x411875);
                 _0x5d2845 = _0x3543f3['length'];
                 if (_0x411875 !== !![]) {
                     _0x47b83f += '=' + _0x411875;
                 }
             }
             _0x1c9d63['cookie'] = _0x47b83f;
         }
     };


Можно сказать, что этот код обфусцированный, но даже если так: видим, что есть понятная функция, ясное ‘data’, setCookie что-то делают, а дальше просто пелена и ничего непонятно — что-то конкатенируется, где-то пробелы. Все очень плохо.

Что необходимо учитывать в неймингах


Используем CamelCase нотацию: camelCaseNotation. Никакого транслита, все названия методов только на английском: ssylka, vikup, tovar, yslyga или checkTovaraNaNalichieTseni — это провал. Последнее, кстати, я сам написал, когда только начинал программировать.

Никаких item, data, el, html, arr, особенно при итерации массивов. Например, для массива товаров или офферов выбирайте понятные имена: product, offer, etc. Отличие item от product не так уж велико, но читаемость — выше. Даже если у вас однострочная функция, которая что-то плюсует — бизнес-понятное название повысит читаемость.

Нижнее подчёркивание для приватных свойств: private_property. Я добавил это правило, потому что я сам пишу на TypeScript уже второй год, но в JS нет модификаторов доступа, и в соглашении об именовании мы договорились, что нижнее подчеркивание определяет для других разработчиков private свойства.

Константы заглавными буквами: const BLOCK_WIDTH = 300;, а наименования классов в capitalized case: class SomeClass. Я пишу на TypeScript и там все понятно, в ES6 тоже все понятно, но есть еще legacy проекты, в которых все классы функции с оператором new пишите в capitalized case.

Никаких однобуквенных переменных: u = user. Это отсылка к i — не надо так. Пишите понятно, то есть бизнес-функционально. Не надо делать метод Check, который что-то чекает, а что — не ясно. Пишите говорящие названия методов: addProductToCard(); sendFeedback().

Императивность


Небольшое отступление. Императивность появилась одновременно с программированием. В то время кодили на Ассемблере, и писали императивно: каждую команду, каждый шаг описывали подробно, присваивали значению ячейку памяти. Мы живем в 2019 году и так на JS уже не пишем.



Это простой, но императивный код, в котором есть цикл for, переменные. Неясно, зачем они здесь добавлены.

for (let i = 0; i >= 10; i++) {
    const someItem = conferences[i];
    const prefixString = 'Hello ';
    if (someItem === 'Frontend Conf') {
        console.log(prefixString + someItem);
    }
}

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

Декларативность


На смену пришел декларативный стиль. Мы пишем на JavaScript и он нам доступен. Декларативный стиль выглядит так:

conferences
    .filter(someItem => someItem === 'Frontend Conf')
    .forEach(someItem => console.log('Hello' + someItem));

Это то же самое, что в императивном, но гораздо проще и понятнее.

Преимущества декларативного кода


Такой код проще читается, поддерживается и тестируется, а сложные конструкции кода можно спрятать за методами и абстракциями. Понять различия между императивным и декларативным стилем можно на примере жарки яичницы. Чтобы пожарить яичницу в императивном стиле, мы берем сковороду, ставим на огонь, наливаем масло, берем яйцо, разбиваем, выливаем. В декларативном стиле мы скажем: «Пожарь яичницу», и процесс будет скрыт за абстракциями. Мы хотим пожарить яичницу, а не разбираться, как это работает.

Проблемы начинаются, когда не очень опытные разработчики приходят из университета, где они учили Pascal, и пишут так:

const prefix = 'Hello ';
conferences
    .forEach(someItem => {
        if (someItem === 'Frontend Conf') {
            const result = prefix + someItem;
            console.log(result)
        }
    });


Это комбинация декларативного и императивного стилей. Здесь нет ни читаемости, ни полной императивности, какие-то переменные и if. Этот if человек добавил потому, что просто не знал про фильтр. Если вы лид, и видите такой код, подойдите, ткните палкойссылкой и приведите код к декларативности.

Создание переменных


Не создавайте переменные ради переменных — это плохая идея. Когда я выяснял у разработчиков, зачем они так делают, то слышал:

— Ну, это же увеличивает читаемость!

Что тут увеличивает читаемость — const username = user.name? Если хотите создать переменную — придайте названию значимость. Например, у нас есть регулярное выражение:

const somePattern = /[0-9]+/;
str.split(somePattern);

const someResult = a + b - c;

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

Создавать переменную ради создания переменных не стоит.

Неочевидное переопределение переменных


Допустим, мы создали переменную element, из названия которой непонятно, что это такое. Написали DOM element, записали его переопределение в массив по каким-то причинам, и ушли:

let element = document.getElementById('someId');
arr.forEach(item => {
    // ...
    // несколько строк кода мы пропустили
    // ...
    element = document.getElementById('someItem')
    if (typeof item  === 'string') {
        let element = document.getElementById('some' + item);
        element.appendChild();
    }
});

Все ОК, ушли, забыли. Следом Петя, который работает в нашей команде, зашел и добавил блок if. А что такого? Просто переопределил переменную еще раз, и ушел. А область видимости уже другая. Когда следующий разработчик попытается разобраться в этом коде, особенно если метод большой, то будет ждать someId или someItem, а там совсем не то. Это место, где можно потерять много времени, отыскивая, в чем же проблема. Мы будем писать debugger, поставим brake point, смотреть, что там — в общем, не пишите так.

Разделение на методы


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

Методы должны нести атомарную функциональность: один метод — одно действие. Если у вас действие из одной строки, не подмешивайте еще, просто потому что метод такой маленький. Метод должен быть не больше 10 строк. Это утверждение вызывает холивар и сейчас тоже «выстрелит», поэтому пишите мне или в комментариях, и я объясню, почему написал это правило.

Модульность кода


Модульность улучшает читаемость кода за счет разделения на абстракции, помогает «спрятать» сложно читаемый код, его проще тестировать, и легче исправлять ошибки. Объясню подробнее.

Прячемся за абстракциями


Например, есть код, который создает кнопку, присваивает ей id, класс и кликает на нее — все просто.

const element = document.createElement(‘button’);
element.id = 'id_button';
element.classList = ‘red’;
document.body.appendChild(element);
element.click();

Можно добавить в код кнопки функцию, обернуть, и использовать при создании кнопки функцию createButton:

const.button = createButton('id_button');
button.click();

function createButton((id') {
    element = document.createElement(‘button’);
    element.id = id;
    element.classList = ‘red’;
    document.body.appendChild(element);

    return element;
}

По «говорящему» названию понятно, что функция делает и что передается id. Если мы хотим сделать кнопку и не разбираться, как она создается и зачем, — мы пишем код, используя эту функцию.

button.component.js

let button = createButton('id_button');
button.click();

Дальше пишем helper, который позже используют другие разработчики. Если они пожелают понять, как жарится яичница или захотят изменить рецепт — добавить или убрать соль, то зайдут и почитают.

button.helpers.js
function createButton(id) {
    let button = document.createElement('button');
    button.id = id;
    button.classList = 'red’;
    document.body.appendChild(element);

    return button;
}

Типизация


Не буду долго рассказывать про типизацию — есть куча докладов. Я пишу на TypeScript, мне нравится, но есть еще flow и другие инструменты. Если у вас нет в проекте типизации, то самое время внедрить. Это помогает отладить кучу ошибок.

Code smells


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

Code style


Это набор правил команды, проекта или компании, которых придерживаются разработчики. Хороший Code style содержит примеры хорошего и плохого кода. Его можно писать в любом удобном инструменте и месте. У нас это Wiki, а для маленькой компании достаточно файла в Word. Также можно взять готовый Code style, который уже используют другие компании: JQuery, Google или Airbnb — cамый популярный Code style.

Если вы используете определенную технологию или фреймворк, то они, как правило, тоже имеют свои Code style, которые стоит посмотреть. Например, в Angular это Angular Style Guide, или React/JSX Style Guide от Airbnb.

Это пример нашего Сode style.



Здесь есть раздел создания переменных, и описано, как не надо делать и как надо.

Техдолг


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

Техдолг порождает костыли и низкокачественный код.

Из-за техдолга я пишу плохой код и костыли. Следующий разработчик посмотрит на вот это вот всё, увидит косяки и допишет еще костыль: «Если сейчас еще подопру, ничего страшного не будет». Техдолг порождает костыли, теряется качество, что снова порождает костыли, а они растят техдолг еще больше.

Существует теория «разбитых окон». Если в здании появляется разбитое окно, и его не меняют, то через некоторое время появится второе разбитое окно, третье, граффити. Люди видят, что за зданием никто не следит и наказания за разбитые окна не следует. Так же и с кодом. Часто в legacy проектах код обрастает костылями, потому что условные Пети и Васи видят костыли и думают: «Ничего страшного, подопру, не я первый». Поэтому в нормальных компаниях техдолгу уделяют достаточно времени — либо берут квоту, либо технический спринт, который решит вопрос. Если вы лид, или как-то влияете на процессы построения спринтов и список задач, которые берутся в работу, уделите внимание техдолгу — это важно.

Репозиторий


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



Правильный ответ
Информативные сообщения — в блоках, а «добавил фичу», «поправил баг» — неинформативные.

Сообщения коммитов


Я пишу в WebStorm и люблю его. В нем можно настроить подсветку номеров задач, переход при клике в Task Tracker — это круто. Если кто-то не использует WebStorm — самое время, так как с ним сообщения коммитов получаются качественными. Что такое качественный месседж коммита? Это коммит, в котором есть номер задачи и короткое, но ёмкое изложение сути изменений: «сделал новый модуль», «добавил кнопку», «добавил фичу, где создается компонент», а не безликое «добавил фичу». При просмотре коммитов будет понятно, где добавили компонент, а где исправили баг. Еще в сообщениях коммитов обязательно указание типа изменений: feature, bugfix, чтобы было понятно, в рамках чего происходили изменения.

Gitflow


Про Gitflow расскажу коротко. Подробное описание в переводе статьи Vincent Driessen. Gitflow одна из самых популярных и очень удачных моделей ведения репозитория, в которой есть основные ветки — develop, master, preprod, prod, и временные ветки: feature, bug, release. Когда мы начинаем задачу — отводим feature-ветку от develop-ветки. После прохождения код-ревью на feature-ветке вливаем ее обратно в develop. Под конец собираем релизы из develop и релизимся в master.



Инструменты


Первый — Commitizen. Это программная утилита, про которую я узнал не так давно — посмотрел, пощупал, понравилось. Она позволяет стандартизировать сообщения коммитов, имеет приятный консольный интерфейс, в котором можно выбрать фичи. Если у вас есть коммиты в духе: «поправил фичу» или «поправил баг», — самое время показать Commitizen своим ребятам, чтобы они хотя бы для начала ее использовали, а потом уже можно писать «из головы».

Linters — обязательный инструмент в каждом проекте. В линтерах много готовых конфигов, но можно писать свои правила. Если у вас есть свои правила, то линтер должен линтить эти правила — вам нужно будет написать правила для своего Code style.
Полезные ссылки по линтерам:


Отдельным пунктом выделил sonarJS. Это инструмент, который позволяет интегрировать проверку кода в CI. Например, мы делаем pull request, и дальше sonarJS пишет в pull request про наши косяки, и, либо апрувит, либо нет. Это круто — мне понравилось. Даже если условный Вася думает, что его косяк никто не заметит — заметит sonarJS.

Инструмент легко встраивается в Jenkins. Наши ребята встроили достаточно быстро. Скорее всего, интегрируется и в другие системы, но мы не пробовали. SonarJS еще проверяет код на code smells. Если честно, я не знаю, делают ли так обычные линтеры.

Форматеры или стилизаторы — это инструмент, который форматирует код по конфигу, например, Prettier. Можно настроить на pre-push hook, и получить единый стиль кода в репозитории. Петя из нашей команды может поставить 500 пробелов, или вообще не писать точку с запятой — в репозитории все будет лежать чистенько и красиво.

Форматер позволяет держать код в едином стиле.

Хотел рассказать историю, которая произошла с нами. Мы внедрили Prettier в проект, в котором написано много задач, и решили не прогонять через него весь проект, а только кусочки кода с фичами. Нам казалось, что постепенно так мы выплывем и не испортим историю коммитов: в аннотации будет видно, кто правил последний. Это было плохое решение. Когда мы делали задачку и pull request, и меняли пару строк, то Prettier форматировал весь файл, а когда смотрели pull request — вот же, всего две строки! Это забирало кучу времени на код-ревью. Поэтому если хотите внедрить Prettier — прогоняйте весь проект.

Есть еще инструмент — error tracking в runtime. Например, вы выложили приложение в прод, по нему ходят пользователи. Если где-то сломалась консоль или упал сетевой запрос — мы можем это отследить. Error tracking показывает шаги пользователей по DOM-дереву, поддерживает фильтры, имеет гибкую настройку классификации ошибок и много всего. Я сам пользовался Sentry, могу рекомендовать TrackJS и точно знаю, что есть еще ресурсы.

Но от платных решений вроде Sentry, можно отказаться. Почему? Давайте сначала посмотрим, что такое Sentry. Это список ошибок.


Это детализация ошибки.


Здесь можно посмотреть устройство пользователя, ОС, браузер и сделать выводы: «Ага, сделали фильтр по iOS, и видим, что там есть такой баг, а в Android нет — наверное, проблема связана с iOS».

В Sentry есть StackTrace — тоже можно посмотреть, если нужно.


Мы отказались от Sentry. В скрипте сбора ошибок мало строк, а все остальное — это визуализация: графики и чарты. Сами ошибки собираются достаточно просто, скрипт небольшой, поэтому мы подумали: «Зачем нам платить деньги, когда мы можем написать свое?». Написали — оказалось возможно. У нас нет крутых графиков и красоты, но зато решение рабочее. По моему опыту, если не хочется платить деньги — можно «покурить» скрипт сбора ошибок и написать свою обертку.

Чек-лист


Чек-лист на основе того, о чем мы говорили.

  • Добивайтесь линейности кода и уменьшайте его вложенность.
  • Правильно называйте переменные — со смыслом.
  • Стремитесь к декларативности и модульности.
  • Особенно не комбинируйте. Когда вы комбинируете, в мире грустит один frontend-developer из Tinkoff.ru.
  • Заведите себе Code style, если его еще нет. Если лень — берите готовый.
  • Не копите техдолг — это опасная штука. Есть понятие энтропии, как меры хаоса, и у техдолга она высока.
  • Используйте Git Flow — это наше все, я его люблю. Но если решите использовать другой способ ведения репозитория — ваше право.
  • Инструменты везде — линтеры, форматеры. Мы живем в современном мире, и грех этим не пользоваться.

Свой доклад я бы хотел закончить правилом бойскаута. В книге «Чистый код» написано, что если я прихожу на парковку и вижу окурок — я подниму и донесу до урны, и сделаю мир чище и лучше. То же можно сказать про код. Если я прихожу в класс и вижу, что там накосячено, а правка косяка займет немного времени, то исправлю его. Так энтропия уменьшится, проект будет жить долго, а техдолг копиться медленнее.

Контакты докладчика Ивана Ботанова:Twitter и Facebook.

Доклад Ивана — один из лучших на Frontend Conf. Понравилось? Приходите на Frontend Conf РИТ++ в мае и подпишитесь на рассылку: новые материалы, анонсы, доступы к видео и больше крутых статей.

Если доклад вызвал желание поделиться своим опытом — подавайте доклады на FrontendConf РИТ++. Даже если доклад не готов — присылайте тезисы, а Программный комитет поможет подготовиться к выступлению. Спешите, прием заявок заканчивается 27 марта

Комментарии (32)


  1. JustDont
    26.02.2019 18:30

    Доклад может и один из лучших, но по прочтении невооруженным взглядом видятся взаимоисключающие параграфы. Как собственно и всегда, когда речь заходит за сферическую читаемость кода в вакууме.
    «Не вкладывайте тернарные операторы»-«не создавайте лишних переменных»-«пишите линейно» — в итоге тут таки будут либо трусы, либо крестик. Вложенные тернарные операторы как раз таки и пишутся, чтоб избавиться от двух последующих пунктов, уберем тернарники — вернёмся к if then else и временным переменным.

    И так далее.


    1. Danik-ik
      26.02.2019 19:49

      Кажется, без конкретных примеров это всего лишь брюзжание. А если предложить конкретные примеры, вдруг окажется, что вариантов намного больше двух, а Вы сравниваете холодное с твёрдым в мире, где в тренде тёплоге, мягкое и пушистое.


      И если речь идёт о выводе из вакуума: Вы готовы предложить примеры, которые (на Ваш взгляд) ну никак нельзя сделать удобочитаемыми, разумно употребляя предложенные правила?


      1. JustDont
        26.02.2019 20:11

        разумно употребляя предложенные правила?

        Заметьте, как вам пришлось употребить слово «разумно», чтоб не загнать себя в угол. Потому что когда «разумно» — это как раз переход от вакуума к реальным случаям, где читаемость читаемостью, но есть еще и многие другие факторы.

        Кажется, без конкретных примеров это всего лишь брюзжание.

        Вам серьезно нужны «конкретные примеры кода», чтоб понять, что заменяет тернарник? Да вот это он заменяет:
        let a;
        if (condition) {
          a = 1;
        } else {
          a = 2;
        }

        А два вложенных — соответственно, иф с ифами внутри, и так далее. Как такой кусок не переписывай, а читаемость у него будет приблизительно одинаково плохая — ну, разве что исключая вырожденный случай типа приведенного в статье, где скобочки блоков будут куда более заметны (и для IDE в том числе), чем где-то заныканые среди кода "?" и ":".

        Любые правила красоты кода — они всегда имеют ограниченные показания к применению, про которые обычно в статьях, полных категорического императива «всегда делай вот так» не пишут. А потом люди читают эти статьи, и пишут
        if (!condition) {
          // ...много кода...
          return 2;
        }
        // ...еще много кода...
        return 1;

        И ты такой спрашиваешь — а чё у тебя отрицание условия когда можно было и не отрицать, и блоки кода на ровном месте местами переставлены? И тебе в ответ «ну вот паттерн раннего return, туда-сюда...», и ты сидишь, и думаешь, что что-то в мире пошло не так.


        1. Danik-ik
          27.02.2019 12:14

          Вы верно заметили цель употребления слова "Разумно". Я в любом случае не хочу обсуждать неразумное применение любых правил, независимо от того, призывал ли автор делать "только так" или нет. И я действительно не хочу в этот угол. Честно говоря, явно выраженные призывы постоять в углу категорических императивов вызывают у меня стойкое отторжение.


          Однако, я не нашёл в тексте категорического призыва «всегда делать вот так». Я вижу цель (с которой я согласен) и предложения по решению в виде тезисов (которые никто не запрещает обсудить). Получается, что с поправкой на моё личное восприятие (в тексте этого нет, но для меня это непереопределяемое умолчание) — я получаю призыв к разумному использованию ряда практик на пути к определённой цели. Да, ограничения (типа "всегода имей в виду цель и сверяй с ней результат") не рассмотрены, но это выступление на конференции, а не учебник и не энциклопедическая статья. Выступление на конференции, как правило, не даёт ответов на все вопросы (вопросы часто задаются потом). И делать его в виде сжатого набора тезисов "что я предлагаю" (в контексте "для чего я это предлагаю") — нормально.


          Чем заменить тернарник, я знаю, честно. И даже чем вложенный тернарник заменить. Вопрос мой был не о том, но возможно, я Вас изначально не понял: похоже, Вы в своём комментарии исходили из того, что всё, что может быть исполнено "неразумно", будет сделано именно так. Имея опыт оперативной техподдержки в энтерпрайзе не могу не согласиться, будет. Но тут даже явное указание на ограничения не спасает, так как всё, что может быть не прочитано… может быть не прочитано.


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


        1. Satim
          27.02.2019 14:37

          Тоже про ранний return подумал
          ведь логичнее было бы так сделать

          result = user;
          if (isAdmin()) {
              result = admin;
          }
          
          return result;


          1. JustDont
            27.02.2019 14:44

            Скажем так, ранний return нормален, если у вас несбалансированные ветки ифа — одна длинная, а другая короткая. Тогда да, выкинуть короткую ветку из else и поставить в начало с return будет вполне себе красиво и более читабельно. Но вот если что-то в этой ситуации не так, то будет классическое «но есть нюанс».

            Ну да и вообще, если меня коллеги спрашивают, что вот они сидят думают час над порядком следования блоков в коде (или еще чем-то примерно настолько же очень важным) и им нужно еще мнений — мое мнение обычно выражается в «займитесь лучше чем-нибудь реально полезным» ^_^


          1. CJay
            27.02.2019 17:15

            А почему тогда не так?


            if (isAdmin()) 
                return admin;
            
            return user;

            А если ветвей больше, то ещё нагляднее, как мне кажется:


            if (isAdmin()) 
                return admin;
            
            if (isManager()) 
                return manager;
            
            if (isOwner()) 
                return owner;
            
            if (isAuthorizedUser())
               return user;
            
            return anonimUser;


            1. Satim
              28.02.2019 10:36

              Один return одна точка выхода из функции. Если кода мало то ваш вариант думаю допустим. Но если кода много в методе я обычно стараюсь избегать варианты где несколько return.


    1. shm-vadim
      26.02.2019 20:28

      уберем тернарники — вернёмся к if then else и временным переменным


      Сходу не нашел, где именно, но в свое время читал, что функция, где идет широкое ветвление, берет на себя слишком много, и ее желательно разбить на несколько, выполняющих что-то одно. И в этом смысле вложенные if/else и тернарные операторы действительно нужно стараться избегать, т.е. чем их меньше, тем лучше.


      1. JustDont
        26.02.2019 21:03

        Стремление разбивать код тоже крайне легко довести до абсурда, и получить 1000 разных функций в 100 разных файлах на пару строк каждая, и без всякой вложенности. Будет ли это читать легче одного монстро-метода на 1000 строк? Сомневаюсь, скорее всего будет примерно одинаково плохо.

        Соль тут как раз в слове «разумно», прозвучавшем в комментариях выше — когда подходы применяются разумно, то есть, по совокупности доводов, а не просто «так будет читаемее и точка» — тогда всё нормально. Например, когда из процессов внутри функции можно без труда выделить подпроцессы (это обычно тогда, когда вы без труда и фантазий можете поименовать выделенную функцию, и у ревьюверов от её имени не случается WTF) — тогда их выделить может быть даже и нужно. По крайней мере это более сильный аргумент для выделения, чем «всё широкое ветвление надо разбить!!!1!».


        1. shm-vadim
          27.02.2019 06:41

          По-моему метод-монстр на 1000 строк — это совершенная патология, а вот 1000 методов в различных классах или просто функций — это логичный процесс развития программы. Т.е. при ее росте у вас, так или иначе, все равно эти 1000 методов появятся.

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


          1. oracle_and_delphi
            27.02.2019 09:23

            1000 методов в различных классах или просто функций — это логичный процесс развития программы

            Если они реально необходимы, а не являются разбиением ради разбиения.
            Чем больше методов — тем выше вероятность в них запутаться, и тем больше мата, когда через несколько лет все эти методы станут… ЛЕГАСИ!!!


            1. shm-vadim
              27.02.2019 11:32

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

              По мне, так этот процесс куда более безболезненный чем копание в полотне кода, где переменная может использоваться через 200, 300 и 700 строк после ее инициализации. Вот как это рефакторить и дебажить — совершенно непонятно.


              1. oracle_and_delphi
                28.02.2019 20:29

                Тысячи методов и функций в сотнях различных классах, могут привести к таком результату:

                — MVP это просто. Смотри: Из вью мы вызываем метод из презентера, а презентер дергает метод из модели, модель после обработки возвращает в презентер, а презентер возвращает во вью, и все это делается коллбеками.

                — Дедка за репку, бабка за дедку… через семь калбэков позвали мышку — а она давно от багов умерла.


                1. shm-vadim
                  28.02.2019 20:38

                  Ну, да, это проблема сильной связанности кода. С ней тоже надо бороться.


  1. unel
    26.02.2019 20:23

    conferences
        .filter(someItem => someItem === 'Frontend Conf')
        .forEach(someItem => console.log('Hello' + someItem));


    Это то же самое, что в императивном, но гораздо проще и понятнее.


    Не совсем то же самое: вместо одной итерации будет две: первая для фильтрации, вторая — для вывода.

    Может это не сильно принципиальная разница, но её стоит отметить.


    1. Nimtar
      27.02.2019 01:05

      Вообще-то эти фрагменты кода совершенно не близки по исполнению и результату :)

      Это я про символичную опечатку человека, отвыкшего от сишного for.


  1. unel
    26.02.2019 20:28

    Не создавайте переменные ради переменных — это плохая идея.

    Это плохая идея — почему?

    Что тут увеличивает читаемость — const username = user.name?

    Читаемость не уверен, что увеличивает, но вот доступ например ускорить может (если поиск идёт по цепочке прототипов, или если это вычисляемое свойство, например).

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


    1. alatushkin
      26.02.2019 21:01

      Рассуждения про скорость доступа тут не причем (формально -возможно, но это на 'похвастаться"/«помериться», когда больше не чем)

      Читаемость увеличит, если (some.foo()+someOther.prop)+2 присвоить переменной с осмысленным названием -то дальше по коду это будет легче пониматься.

      Про «плохость» доступа черезмногие "." — все верно, у этого есть ь название (закон Деметры), Но это скорее относится к дизайну системы. Codestyle не исправит ошибки в архитектуре


  1. HeaTTheatR
    26.02.2019 20:43
    -2

    После строки


    Читаемость ухудшают отступы

    ставлю минус статье и в карму!
    Дальнейшее чтение статьи прекращается, ибо писал человек понятия не имеющий о ЧИСТОТЕ и ЧИТАЕМОСТИ кода!


  1. gearbox
    26.02.2019 21:03
    +4

    Кажись автор местами путает декларации и функциональщину. И ту и другую можно противопоставлять императиву, но в коде, приведенном в статье идет голимая функциональщина со словами — смотри как декларативненько. Декларативность это немногое другое. Можно с пеной у рта доказывать что функциональщина более декларативна чем императивщина (и я с этим согласен), но делая something.map или forEach вы все равно указываете ЧТО ДЕЛАТЬ что бы добиться результата, а не то КАКОЙ вы результат хотите. Конкретно вот тут:

    ptor.findElement(protractor.By.id('q01_D')).click()
        .then(() => ptor.findElement(protractor.By.id('q02_C')).click())
        .then(() => ptor.findElement(protractor.By.id('q03_D')).click())
        .then(() => console.log('done'));
    


    декларативно будет что то вроде:

    Must be clicked elements with id [bla bla bla]

    ptor.findElement нельзя называть декларацией — это императив, хоть и в функциональном стиле.

    Мое мнение, возможно никем не разделенное и однозначно никому не навязываемое.


    1. VIkrom
      27.02.2019 06:36

      Чаю этому господину. Так называемую «декларативность» суют сейчас к месту и не к месту.


  1. alatushkin
    26.02.2019 21:04

    Радостно, что инструменты и практики взрослой разработки продолжают приходить и в js/ на фронт.

    Особенно радует порт сонара.


  1. sith
    26.02.2019 21:43

    Применить паттерн раннего return

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

    const.button = createButton('id_button');
    button.click();
    
    function createButton((id') {
        element = document.createElement(‘button’);
        element.id = id;
        element.classList = ‘red’;
        document.body.appendChild(element);
    
        return element;
    }


    Очень плохо. Должно быть одно из двух. Либо переименовать функцию в createRedButtonInBody либо переписать, на что-нибудь, вроде этого (я не пишу на JS и не помню синтаксис — отсюда возможные ошибки и TODO вместо кода проверок, но смысл, думаю, понятен):

    const.button = createButton('id_button', 'red', document.body);
    // TODO: Check if button was created.
    button.click();
    
    function createButton(id, classList, parent) {
    // TODO: Check paremeters.
        element = document.createElement(‘button’);
        element.id = id;
        element.classList = classList;
        parent.appendChild(element);
    
        return element;
    }


    Но я такую функцию вообще не стал бы писать с самого начала. Она перегружена функционалом и берёт на себя сразу 3 обязанности — создание элемента, изменение его свойств и добавление элемента на страницу.


    1. ZurgInq
      26.02.2019 22:38
      +2

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

      Правило одного return пришло из Си подобных языков, где нужно было освобождать память и ресурсы при выходе из функции. Ранний return отсекает граничные случаи (например валидация), и не несёт особой смысловой нагрузки, т.е. не должен заставлять прыгать взглядом.


    1. CJay
      27.02.2019 17:26

      Если у функции имеется какой-то контракт на входные данные, то разве не логично ли использовать ранний return?


      function makeSomeAction(items, action){
          if (items.isEmpty())
              return;
          if (action == null)
              return;
      
          ... какой-то код
         return;
      }

      Как это сделать с одним return?


      1. gearbox
        27.02.2019 17:42

        if(!isItSomethingICanWorkWith()){
        return | throw | reject
        }


  1. Nimtar
    27.02.2019 01:09

    В декларативном стиле мы скажем: «Пожарь яичницу»

    «Пожарь» — в императиве.


  1. tbl
    27.02.2019 09:04

    Если часто в проекте вылезают декларативные конструкции типа: someArr.map().filter().map…
    То лучше смотреть в сторону библиотек вроде underscore, они позволяют собирать ленивую цепочку и на последнем шаге ее применять за один проход итерирования, а не создают на каждом шаге промежуточные массивы.


  1. Lissov
    27.02.2019 09:36

    Читаемость ухудшают отступы

    Автор очевидно имел в виду «отсутсвие» или «неправильные» отступы. Чем грешат некоторые из примеров, кстати.
    Отдельно удивляют тулзы вроде Решарпера, которые любят перенести кусок длинной строки на следующую, но с изначальным отступом. Получаем «стихи Маяковского»:
      var subsubitem = list.SelectMany(
                                       element => element.Items.Where(
                                                                      i => i.id = 5)).                                                                                                            
                                                                             First()
    

    Спасибо, что это хоть отключаемо.
    не худшее, что можно встретить в коде, — вложенные тернарные операторы

    Не соглашусь. Они нормально читаемы, если их правильно оформить, а не как в статье. Пример с кодом из статьи, как бы оформил я:
    arr.length > 0
      ? arr[1] === 1
          ? arr[1] = 2
          : arr[1] = 1
      : console.log('empty arr');
    

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


  1. cosmrc
    27.02.2019 15:57

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

    • Написать комментарий с описанием логики работы функции с полученным значением;
    • Сделать функцию-обертку для интерфейсной функции с другим именем;
    • Сделать временную переменную с именем, которое отражает смысловую нагрузку полученного значения применительно к алгоритму функции;

    Из всех предложенных вариантов, именно последний ведет к самому качественному коду.


  1. MacIn
    27.02.2019 21:48

    Сначала написал длинный комментарий с критикой нескольких высказываний, но потом стер.
    В целом — вкусовщина и просто провокация «делайте так, потому что мне кажется, что это более читаемо». При том, что ты смотришь и думаешь: «ну как, каааак человек может считать „после“ более читаемым, чем было „до“?»

    Нет объекта спора, просто вкусовщина и любые общие споры по такой теме — пустая трата времени.