Code quality

Code quality is a theme that was born with programming. ISO 9000 is used to assess and control the quality of enterprise management, GOST and the same ISO are used for products, but there is no GOST code for quality assessment. There is no exact definition and standard for code quality either.



Each developer understands quality in his own way, based on experience. The views of the joons and the lead are different, and this leads to disagreement. Each team for individual projects evaluates the code in its own way. The team is being updated, developers are leaving, team leaders are changing - the definition of quality is changing. Ivan Botanov will try to help solve this problem ( StressoID) from Tinkoff.ru - Frontend-developer, teacher of the Angular online course, speaker at meetings and conferences, tutoring on YouTube, and sometimes team coach in companies.

In deciphering Ivan’s report on  Frontend Conf, we ’ll talk about readability, naming, declarativeness, Code style, and indirectly touch on the relations of joons and leads: mistakes, rakes and “burning” of timlids.

Disclaimer: Prepare yourself mentally, there will be a lot of bad code in the text , taken from a  "special" site .

A bit of history

We all write in different ways. You may have noticed this when you changed your place of work, project or team - unusual things are immediately evident. This has also happened to me many times, which is why this report was born. I focused on novice developers, but the article will be useful to those who already more manage than write, or customize development processes. What are we going to talk about:

• About the problems of unreadable code.
• Let's discuss naming.
• Let's see what are the differences between declarative and imperative style, and what are the problems.
• About code modularization and typing.
• About Code style and technical debt, about commits and git flow.
• About tools that you can use, and fixing errors in the prod.

Before you begin, ask the question: "How many programmers have to knock down the bus stop to the project to develop?" . The correct answer: everyone.

What is a Bus Factor?

Conditionally, Petya or Vasya is working in the team, who knows everything about the project: they go to him, ask about this and that, how it works here, and how it works there. Everyone depends on Petya, the bus number of the project is one. The smaller the number, the more difficult it is to develop the project, because everyone distracts Petya, and he is cool, and must do the tasks, and not answer questions.

You will say how cool it is to be Pete! Everyone loves him, appreciates him, needs him.

This is not true. Usually, Petya is a team leader, and he must deal with other tasks: discuss the development of the project, build architecture, manage the team, but do not go to the dunes and explain why it is written here and not otherwise.

If the code is clean and good, then it’s nice to read it and there are fewer questions from the jones.Clean code increases the viability of the project and lowers the threshold for entry . When new people appear on the team, they will ask fewer questions. In such a project, it is easier to attract developers due to the low entry threshold.

Quality code boosts bus numbers.

Readability

Readability is affected by indentation, crooked naming and strong nesting - many projects suffer from this. In addition to indentation, multi-line ternary operators, the lack of a single Code style, a combination of development approaches and non-obvious redefinition of variables reduce readability. All of these are the most common causes of poor code readability.

For myself, I have identified the term linear code  - this is code that can be read like a book.

Linear code is read from left to right, from top to bottom, without having to return to the previously written code.

An example of such a code:

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);
                            }
                        })
                    }
                })
            }
        })
    }
})

This code is linear, but it has another problem - it is heavily nested. Therefore, we must also monitor nesting. Nonlinear code

example :

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 we discard all that is superfluous and figure out what breaks linearity, then we will see something like this:

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

When there is else, first you have to look at what is written in one place, then in another. If it is a large or heavily nested if, then the attention is scattered, and the code is hard to read.

How to reduce nesting and achieve linear code?

Combine conditions . This is the simplest thing we can do - nested if I can combine in condition and slightly reduce nesting.

It was:

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

It became:

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

Apply an early return pattern . It allows you to completely get rid of else. I can replace the method or a piece of code with if-else with an early return, and then either one block of code or another will be executed. This is very convenient - you do not have to scroll and return to some parts of the code.

It was:

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

It became:

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

Apply promise chain . This is a typical Protractor code. I wrote E2E tests not so long ago, and such code hurt my eyes:

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');
        });
    });
})

With promise chain, the code turns:

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'); 
    });

If we use the knowledge of the arrow function, we can do this:

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'));

It is readable, declarative, beautiful - everything is clearly visible.

Higher order observable. Since I am an angular and use RxJS, I am faced with the problem of strong nesting of spaghetti code - nested Observable , i.e. nested subscriptions. There is a stream, and inside the stream you need to get the value and then something to do with another stream. Some write like this:

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

And this really affects adult projects. You can do this:

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

Applying knowledge of the RxJS API, we moved away from strong nesting, thanks to higher order observable , and came to  declarative . This thing, which throws the value of the internal flow into the external, is all. But it’s clean, linear, beautiful and not invested.

Nested ternary operators

In my opinion, the worst that can be found in code is nested ternary operators . Rewrite them on block conditional statements, try not to use them at all. We will not talk about implicit conditions at all - this is a failure.

An example of nested ternary operators and implicit conditions:

arr.length > 0 ? arr[1] == 1 ? arr[1] = 2 : arr[1] = 1 : console.log('empty arr');
!a && b && func()

I wrote this simple ternary in 5 minutes. It has the length of the array and some operations, but it is hard to read, because somewhere one question, somewhere else - everything is not clear. This code can be rewritten using multi-line nested ternary operators:

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

You will say:

-  OK, fine!
-  Seen?
- It is  visible!

And what do you say to this:

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;
        })();
    })()
);

Someone wrote and supported this ternarnik. By accessing such a section of code, it will be difficult to figure out where the question mark begins and where it ends. If you use ternaries, please do not invest in one another - this is bad.

Naming

Another bad code:

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;
         }
     };

We can say that this code is obfuscated , but even so: we see that there is an understandable function, a clear 'data', setCookie do something, and then it’s just blanket and nothing is clear - something is concatenated , somewhere spaces. Everything is very bad.

What you need to consider in naming

We use CamelCase notation : camelCaseNotation. No transliteration, all the names of the methods are only in English : ssylka, vikup, tovar, yslygaor checkTovaraNaNalichieTseni - this is a failure. The latter, by the way, I wrote when I was just starting to program.

No item, data, el, html, arr , especially when iterating through arrays. For example, for an array of goods or offerov choose clear names product, offer, etc. The difference between item and product is not so great, but readability is higher. Even if you have a one-line function that pluses something, a business-friendly name will increase readability.

Bottom underline for private properties :private_property. I added this rule because I have been writing TypeScript for the second year, but there are no access modifiers in JS, and in the naming convention we agreed that the underscore defines private properties for other developers.

The constants in capital letters : const BLOCK_WIDTH = 300;and the  names of the classes in a case capitalized:class SomeClass . I write in TypeScript and everything is clear there, everything is clear in  ES6 , but there are also legacy projects in which all function classes with an operator are newwritten in a capitalized case.

No one-letter variables : u = user. This is a reference to  i  - do not. Write clearly, that is, business functionally. There is no need to make the Check method, which checks something, but what is not clear. Writespeaking the names of methods : addProductToCard(); sendFeedback().

Imperativeness

A small digression. Imperativeness appeared simultaneously with programming. At that time, they coded in Assembler, and wrote imperatively: each command, each step was described in detail, and a memory cell was assigned to the value. We live in 2019 and so no longer write on JS.



This is simple but imperative code that has a for loop, variables. It is unclear why they are added here.

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

Imperative code problems : a lot of variables, a lot of servicing constructions of these variables and a lot of comments, because these variables need to be described somehow - you cannot create a variable and forget about it. All this affects the readability of the code.

Declarativeness

The declarative style has replaced. We write in JavaScript and it is available to us. The declarative style looks like this:

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

This is the same as in the imperative, but much simpler and more understandable.

Benefits of a declarative code

Such code is easier to read, maintain, and test, and complex code constructs can be hidden behind methods and abstractions. You can understand the differences between imperative and declarative style by the example of frying eggs. To fry fried eggs in an imperative style, we take a frying pan, put it on a fire, pour oil, take an egg, break it, pour it out. In a declarative style, we say: “Fried eggs”, and the process will be hidden behind abstractions. We want to fry scrambled eggs, not to figure out how it works.

Problems begin when not very experienced developers come from the university where they studied Pascal and write like this:

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

This is a combination of declarative and imperative styles. There is neither readability, nor full imperativeness, some variables and if. This ifperson added because he simply did not know about the filter. If you are a lead and see such a code, come up, poke a link with a stick and bring the code to declarative.

Creating Variables

Do not create variables for the sake of variables - this is a bad idea. When I found out from the developers why they were doing this, I heard:

- Well, it increases readability!

What increases readability here const username = user.name? If you want to create a variable, give the name significance. For example, we have a regular expression:

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

Here I would create a variable so that a person does not waste time on proceedings, but read that this regular checks the phone, and goes further. If you have mathematical operations, also write to a variable, because, for sure, the mathematical operation has a business entity, a certain business landmark, for example, to calculate the basket or make a discount. In this case, you can create a variable.

Creating a variable to create variables is not worth it.

Unobvious redefinition of variables

Suppose we created a variable elementfrom the name of which it is not clear what it is. We wrote a DOM element, wrote its redefinition into an array for some reason, and left:

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

Everything is OK, gone, forgotten. Following Petya, who works in our team, came in and added a block if. So what? Just redefined the variable again, and left. And the scope is already different. When the next developer tries to understand this code, especially if the method is large, it will wait someIdor someItem, and there it is not at all. This is a place where you can lose a lot of time looking for what the problem is. We will write debugger, deliver brake point, watch what is there - in general, do not write like that.

Division into methods

We briefly consider the division into methods and smoothly move on to abstractions.

Methods should carry atomic functionality : one method - one action . If you have a one-line action, don’t mix it yet, simply because the method is so small. The method should be no more than 10 lines. This statement provokes a holivar and now also "shoots", so write to me or in the comments, and I will explain why I wrote this rule.

Code modularity

Modularity improves code readability by splitting into abstractions, helps to “hide” difficult to read code, it is easier to test, and it is easier to fix errors . I will explain in more detail.

Hiding behind abstractions

For example, there is code that creates a button, assigns an id to it, a class and clicks on it - everything is simple.

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

You can add a function to the button code, wrap it, and use the function when creating the button 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;
}

By the "talking" name it is clear what the function does and that id is passed. If we want to make a button and not understand how it is created and why, we write code using this function.

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

Next we write helper , which is later used by other developers. If they want to understand how scrambled eggs are fried or want to change the recipe - add or remove salt, they will drop by and revere.

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

Typing

I will not talk about typing for a long time - there are a bunch of reports. I write in TypeScript, I like it, but there are still flow and other tools. If you do not have typing in your project, then it's time to implement it. This helps to debug a bunch of errors.

Code smells

Code smells are very much intertwined with my theme, because writing poor-quality code generates the very smells. Look at the cool report by Alexei Okhrimenko , he touches on this topic in detail.

Code style

This is the set of team, project, or company rules that developers adhere to. A good Code style contains examples of good and bad code. It can be written in any convenient tool and place. We have this Wiki, and for a small company, a file in Word is enough. You can also take the ready-made Code style, which is already used by other companies: JQuery , Google or Airbnb  - the most popular Code style.

If you use a specific technology or framework, they usually also have their own Code style, which is worth a look. For example, in Angular, this is the Angular Style Guide , or the React / JSX Style Guide from Airbnb.

This is an example of our code style.



Here is a section for creating variables, and describes how not to do and how to.

Technical debt

This is a kind of payment for the fact that somewhere we once mowed. Often a technical debt is born when we do not have time to complete a task and write a reminder to return to it later. Of cases that are not related to business functionalities, this is, for example, updating the framework.

Tech debt spawns crutches and a low-quality code.

Because of the technical debt, I write bad code and crutches. The next developer will look at this, that’s all, see the jambs and add another crutch: “If there’s still some support, there’s nothing wrong.” Tech debt spawns crutches, quality is lost, which again spawns crutches, and they grow tech debt even more.

There is a theory of “broken windows”. If a broken window appears in the building and it is not changed, then after a while a second broken window will appear, a third, graffiti. People see that no one is following the building and punishment for broken windows should not be. So is the code. Often in legacy projects, the code is surrounded by crutches, because the conditional Petit and Vasya see crutches and think: “It's okay, I’ll be the first one.” Therefore, in normal companies, technical debt is given enough time - they either take a quota or a technical sprint that will solve the problem. If you are a leader, or somehow influence the processes of building sprints and the list of tasks that are taken into work, pay attention to the technical debt - this is important.

Repository

Let's discuss the commits messages. The picture shows examples of real messages that I saw in different projects. Which of them do you think are informative?

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

I write in WebStorm and love it. In it, you can configure the highlighting of task numbers, the transition when you click in Task Tracker is cool. If someone doesn’t use WebStorm, then it’s time, because with him, the commit messages are obtained in high quality. What is a quality commit message? This is a commit, in which there is a task number and a short but succinct statement of the essence of the changes : “made a new module”, “added a button”, “added a feature where the component is created”, and not a faceless “added feature”. When viewing commits, it will be clear where the component was added and where the bug was fixed. Even in commit messages, it is necessary to indicate the type of changes : feature, bugfix, so that it is clear within which the changes occurred.

Gitflow

I’ll talk about Gitflow briefly. Detailed description in the translation of the Vincent Driessen article . Gitflow is one of the most popular and very successful repository management models, which has the main branches  - develop, master, preprod, prod, and  temporary branches : feature, bug, release. When we start the task, we divert the feature branch from the develop branch. After passing the code review on the feature branch, we pour it back into develop. In the end, we collect releases from develop and release in master.

Instruments

The first is Commitizen . This is a software utility that I learned about not so long ago - I looked, felt, liked it. It allows you to standardize message commits, has a nice console interface in which you can select features. If you have commits in the spirit of “correcting a feature” or “fixing a bug”, then it's time to show Commitizen to your guys so that they can use it at least for a start, and then you can write it from the head. Linters  is a must-have tool in every project. There are many ready-made configs in linters, but you can write your own rules . If you have your own rules, then the linter should lint these rules - you will need to write the rules for your Code style. Useful links on linter:

• github.com/dustinspecker/awesome-eslint
• eslint.org
• github.com/reid/node-jslint
• palantir.github.io/tslint/
  

A separate paragraph allocated sonarJS . This is a tool that allows you to integrate code validation into CI. For example, we make a pull request, and then sonarJS writes to pull request about our schools, and either upsets or not. This is cool - I liked it. Even if the conditional Vasya thinks that no one will notice his cant - he will notice sonarJS.

The tool integrates easily into Jenkins. Our guys built in fast enough. Most likely, it integrates into other systems, but we have not tried it. SonarJS is still checking code for code smells. To be honest, I don’t know if ordinary linter do this.

Formatters or stylists  is a tool that formats code according to a config, for example, Prettier . Can be configured with  pre-push hook, and get a single code style in the repository. Petya from our team can put 500 spaces, or not write a semicolon at all - everything will be clean and beautiful in the repository.

The formatter allows you to keep the code in a single style.

I wanted to tell a story that happened to us. We implemented Prettier in a project that wrote a lot of tasks, and decided not to run the entire project through it, but only pieces of code with features. It seemed to us that gradually we would come out and not spoil the history of commits: in the annotation we will see who last rules. That was a bad decision. When we did the task and pull request, and changed a couple of lines, then Prettier formatted the whole file, and when we watched the pull request, there were only two lines! This took up a ton of code review time. Therefore, if you want to implement Prettier, run the entire project .

There is another tool - error trackingin runtime. For example, you posted the application in prod, users go on it. If the console broke down somewhere or the network request fell, we can track this. Error tracking shows user steps along the DOM tree, supports filters, has flexible error classification settings and much more. I myself used Sentry , I can recommend TrackJS and I know for sure that there are more resources.

But paid solutions like Sentry can be discarded. Why? Let's first see what Sentry is. This is a list of errors.


This is a drill down detail.


Here you can see the user's device, OS, browser and draw conclusions: “Yeah, we made a filter on iOS, and we see that there is such a bug, but not on Android - probably the problem is with iOS.

Sentry has StackTrace  - you can also see it if necessary.


We abandoned the Sentry. There are few lines in the error collection script, and the rest is visualization: graphs and charts. The errors themselves are collected quite simply, the script is small, so we thought: “Why should we pay money when we can write our own?” Wrote - it turned out to be possible. We do not have cool schedules and beauty, but the solution is working. In my experience, if you don’t want to pay money, you can smoke a script for collecting errors and write your own wrapper.

Check list

A checklist based on what we talked about.

• Achieve linear code and reduce its nesting.
• Correctly name variables - with meaning.
• Strive for declarative and modularity.
• Особенно не комбинируйте. Когда вы комбинируете, в мире грустит один frontend-developer из Tinkoff.ru.
• Заведите себе Code style, если его еще нет. Если лень — берите готовый.
• Не копите техдолг — это опасная штука. Есть понятие энтропии, как меры хаоса, и у техдолга она высока.
• Используйте Git Flow — это наше все, я его люблю. Но если решите использовать другой способ ведения репозитория — ваше право.
• Инструменты везде — линтеры, форматеры. Мы живем в современном мире, и грех этим не пользоваться.

I would like to end my report with the scout rule . The book “Clean Code” says that if I come to the parking lot and see a cigarette butt, I will raise it and bring it to the urn, and make the world cleaner and better. The same can be said about the code. If I come to the class and see what is messed up there, and editing the jamb will take a little time, then I will fix it. So entropy will decrease, the project will live a long time, and technical debt will accumulate more slowly.

Contact speaker Ivan Botanov: Twitter and Facebook .

Ivan's report is one of the best on  Frontend Conf . Liked? Come to  Frontend Conf RIT ++ in May and  subscribe to the newsletter : new materials, announcements, video accesses and more cool articles.

If the report caused a desire to share your experience - submit reports to FrontendConf RIT ++. Even if the report is not ready, send abstracts , and the Program Committee will help prepare for the presentation. Hurry, application deadline is March 27