How to Conduct A Helpful Code Review

February 1, 2019 · View on GitHub

Consider following links:

Formatting, Style, Manual Testing
Typical mistakes
Video

Formatting, Style, Manual Testing

Pull-request template

PRs should have proper name (as per task name), should contain link to working demo (if applicable) and to code. Advise your peer to fix or complete missing parts.

User interface testing

Make sure to test the demo properly
Please check that all interactive elements are visually indicated (by changing cursor form, background etc.)

Code formatting

console.log statements should not be left in final version of the code, unless it's part of the functionality
Please advise your peer to fix irregular indentations and remove redundant empty lines.
Please advise your peer to put newline at the end of every file.
Unnecessary comments should be avoided. Advise your peer to find a way to express their intent through expressive variable names or by abstracting part of the code into properly named function.
Magic numbers are bad

^ Back to TOC ^

Code style

Variable name should be camelCase:

//Before:  
let my_var, myothervar;

// After:
let myVar, myOtherVar;

Variable names should not be too short or too obscure:

//Before: 
let fns, a, b, cont;

// After:
let functionsList, card, rootElement, content;

Variable names should not be too general.

//Before: 
let arr, str;

// After:
let cardsList, cardTitle;

If-statement: multiple conditions can often be combined in one condition:

//Before: 
if (a) return;
if (b) return;

// After:
if (a || b) return;

Magic numbers in code should be avoided. For details see this link.

^ Back to TOC ^

DRY, KISS, SOLID

Please read carefully about DRY, KISS, SOLID, YAGNI principles and help your peers to find possibilities to apply them.

^ Back to TOC ^

JavaScript Features

for loops, forEach, map & other iterative forms
Please pay attention to the content of loop body or callback. Frequent mistake is to make some operation in every iteration, which can be done only once outside loop.
forEach or map? Rule of thumb: if you are using the result of iteration, namely newly created array, use map. if you only need the side-effect of iteration, use forEach. forEach says "we don't use the result".
Read about the following Array methods on MDN and try to find opportunities to use them:
- Array.prototype.find
- Array.prototype.concat
- Array.prototype.includes
- Array.prototype.join

Array function body with brackets & return statement where it is not necessary

// Before
myArray.map(item => { return item.name });

// After
myArray.map(item => item.name);

Creating global variables by accident should be avoided. Variables should be always declared with let/const keywords:

// Before
handleClick = (e) => { /* doSomething */ }

// After
const handleClick = (e) => { /* doSomething */ }

Missing “use strict” directive increases risk of unexpected behavior (except for ES6 modules).
Promises & asynchronicity
- not returning promises
- using async/await where Promise is enough

Encourage your peers to use more ES6 features

let/const instead of var

Destructuring

// Before
`element.addEventListener(‘click’, (e) => { const target = e.target ;  /* do something */})`

// After
`element.addEventListener(‘click’, ({ target }) => {  /* do something */})`

Arrow functions for callbacks and other small functions
Template literals (especially for string concatenation)

^ Back to TOC ^

Typical mistakes

DOM API

event delegation

DOM manipulation in loops.

Adding elements to DOM from a loop is a bad practice. A browser will run reflow and repaint for every element in the loop. Instead, you can:

Use append method, which can add several elements in one operation
Create some wrapper, add your items to the wrapper and then add it to DOM. It will be one operation.
Clone current container. Add items to a container and then replace your old container with a new one. But be aware of event listeners.
Use innerHTML instead

Use of window.event property
window.event is not universally supported and should be avoided. Notably fails in Firefox with error message "window.event is undefined". Use event passed to event handler function:

// Before
someElement.addEventListener('click', function() {
    const value = event.target.value;
    // do something
})

//After
someElement.addEventListener('click', function(event) {
    const value = event.target.value;
    // do something
})

// Even better - with ES6 features (arrow functions and destructuring)
someElement.addEventListener('click', ({ target }) => {
    const value = target.value;
    // do something
})

Relying on DOM structure

Don't use constructions like children[0], firstElementChild, parentNode, nextSibling, etc. In such way, you rely on the order of DOM elements. So in case when you will have changed design - your code will be broken. Which is bad, obviously. Use querySelector or closest, if in event, instead.

Changing styles with JS

Don't use inline style changing - element.style. In most cases this is a bad approach for several reasons:

First of all, a browser will apply such styling for each line separately/ Which means, that every such line of code will be a reason for running of calculations of a page and for drawing it, so you can receive a performance problem. Read about reflow and repaint.
This is an imperative way, you need to write declarative and describe what your code does, not how. This will make your code shorter and easier to maintain.
Reuse of code. Saying, you will need to rotate some other stuff - you will add a similar line to another part of an application. Which is not right because of DRY.

Separation of responsibility - JS for logic, CSS - for styling.

So, replace such parts classes. You can use classList to manipulate them.

Handling changes

keyUp handles not all input types (try pasting text via context menu instead of typing)

^ Back to TOC ^

OO JS (Frogger)

Relates to Object-Oriented JavaScript task.

Minimal requirements to meet:

^ Back to TOC ^

OOP Exercise

Relates to OOP Exercise.

Minimal requirements to meet:

Implement a base class to inherit from
Employ default parameters
Each species is represented with its own class
No need to specify species at instantiation
Classes for species that do not have hands by natural design do not consequently have hands or any equivalent property
All inhabitants are stored in a container (array or object)
JS native features are intensively employed (const, let, Array.map|join|forEach|..., etc)
OOP, SOLID and DRY principles are intensively employed

Optional level up (not required to implement):

Friends list is a list of objects refs rather than names (strings)
Cat-woman class is built employing composition rather than inheritance only

Bonus:

toString magic method; when implemented print(inhabitant) does the job as .toString is called implicitly
this.constructor.name; when used no need to store species property

ES6 classes cheat-sheet

^ Back to TOC ^

Video

Watch a video below for Kottans code review basics intro

(1h13m23s, narrated in Russian)

^ Back to TOC ^