company_banner

Качество кода

    Качество кода — тема, которая родилась вместе с программированием. Для оценки и контроля качества менеджмента предприятий применяется 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 марта
    Конференции Олега Бунина (Онтико)
    438,03
    Конференции Олега Бунина
    Поделиться публикацией

    Комментарии 40

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

      И так далее.
        –1

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


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

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

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

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

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

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

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

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

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


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


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


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

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

              result = user;
              if (isAdmin()) {
                  result = admin;
              }
              
              return result;
                0
                Скажем так, ранний return нормален, если у вас несбалансированные ветки ифа — одна длинная, а другая короткая. Тогда да, выкинуть короткую ветку из else и поставить в начало с return будет вполне себе красиво и более читабельно. Но вот если что-то в этой ситуации не так, то будет классическое «но есть нюанс».

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

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


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

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


                  if (isAdmin()) 
                      return admin;
                  
                  if (isManager()) 
                      return manager;
                  
                  if (isOwner()) 
                      return owner;
                  
                  if (isAuthorizedUser())
                     return user;
                  
                  return anonimUser;
                    0
                    Один return одна точка выхода из функции. Если кода мало то ваш вариант думаю допустим. Но если кода много в методе я обычно стараюсь избегать варианты где несколько return.
                      +2

                      Попытка оставить один return выльется в большую вложенность. Код внутри if/else, внутри которых ещё if/else, а там ещё ветка else, и ещё одна… только для того, чтобы завершить выполнение функции одним return. А ещё это может быть вложено в циклы. Кошмар.


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

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


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

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

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

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

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

                        — Дедка за репку, бабка за дедку… через семь калбэков позвали мышку — а она давно от багов умерла.
                          0
                          Ну, да, это проблема сильной связанности кода. С ней тоже надо бороться.
              0
              conferences
                  .filter(someItem => someItem === 'Frontend Conf')
                  .forEach(someItem => console.log('Hello' + someItem));


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


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

              Может это не сильно принципиальная разница, но её стоит отметить.
                +1
                Вообще-то эти фрагменты кода совершенно не близки по исполнению и результату :)

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

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

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

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

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

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

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

                  После строки


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

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

                    +8
                    Кажись автор местами путает декларации и функциональщину. И ту и другую можно противопоставлять императиву, но в коде, приведенном в статье идет голимая функциональщина со словами — смотри как декларативненько. Декларативность это немногое другое. Можно с пеной у рта доказывать что функциональщина более декларативна чем императивщина (и я с этим согласен), но делая 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 нельзя называть декларацией — это императив, хоть и в функциональном стиле.

                    Мое мнение, возможно никем не разделенное и однозначно никому не навязываемое.
                      +4
                      Чаю этому господину. Так называемую «декларативность» суют сейчас к месту и не к месту.
                      0
                      Радостно, что инструменты и практики взрослой разработки продолжают приходить и в js/ на фронт.

                      Особенно радует порт сонара.
                        +2
                        Применить паттерн раннего 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 обязанности — создание элемента, изменение его свойств и добавление элемента на страницу.
                          +3
                          Есть мнение, что это антипаттерн и у функции должен быть единственный return. Но это обсуждаемо. Так или иначе, ранний return также ухудшает читаемость кода, заставляя прыгать взглядом по функции вверх — вниз.

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

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


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

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

                              0
                              if(!isItSomethingICanWorkWith()){
                              return | throw | reject
                              }
                            +3
                            В декларативном стиле мы скажем: «Пожарь яичницу»

                            «Пожарь» — в императиве.
                              +1
                              «Нам нужна яичница с беконом и помидорчиками, в меру прожаренная»?
                                0
                                Другое дело)
                              0

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

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

                                Автор очевидно имел в виду «отсутсвие» или «неправильные» отступы. Чем грешат некоторые из примеров, кстати.
                                Отдельно удивляют тулзы вроде Решарпера, которые любят перенести кусок длинной строки на следующую, но с изначальным отступом. Получаем «стихи Маяковского»:
                                  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
                                  Ветки видно сразу, но сама конструкция все равно тяжеловесная и стоит разделить.
                                    0
                                    Разделить на что? Предложите Ваш менее тяжеловесный вариант, пожалуйста.
                                    И да, я предпочитаю использовать тернарные операторы для выбора значения, а не выполнения дествий по веткам. То есть давайте модифицируем пример:
                                    var data = arr.length > 0
                                      ? arr[1] === 1
                                          ? arr[1]
                                          : arr[1] + 5
                                      : -8;
                                    

                                    А если из реальной жизни, то напишите код перевода градусов по цельсию в строку с вариантами «ноль»/«ХХ тепла»/«ХХ мороза».
                                  +2
                                  По поводу создания временной переменной ваши коллеги правы. Иногда для значения полученного с помощью интерфейса имеется какая-либо смысловая нагрузка в теле функции. Для того, что бы увеличить читаемость логики, есть несколько вариантов:
                                  • Написать комментарий с описанием логики работы функции с полученным значением;
                                  • Сделать функцию-обертку для интерфейсной функции с другим именем;
                                  • Сделать временную переменную с именем, которое отражает смысловую нагрузку полученного значения применительно к алгоритму функции;

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

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

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

                                        0
                                        То же самое, у нас такие же «правила безопасности». Вот только они далеко не везде совпадают с декларируемыми в данной статье.

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

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

                                      Самое читаемое