Clean code

Adapted in JavaScript

Agenda

  • Variables
  • Functions
  • Classes
  • Miscellaneous

Variables

There are only two hard things in Computer Science: cache invalidation and naming things.

There are only two hard things in Computer Science: cache invalidation and naming things.

Phil Karlton

Use meaningful and pronounceable variable names

const yyyymmdstr = moment().format('YYYY/MM/DD');
const yearMonthDay = moment().format('YYYY/MM/DD');

Use the same vocabulary for the same type of variable

getUserInfo();
getClientData();
getCustomerRecord();
getUser();

Use searchable names

// What the heck is 525600 for?
for (let i = 0; i < 525600; i++) {
  runCronJob();
}
// Declare them as capitalized `const` globals.
const MINUTES_IN_A_YEAR = 525600;
for (let i = 0; i < MINUTES_IN_A_YEAR; i++) {
  runCronJob();
}

Keep convention in mind

class AnimalSoSweet {}                 // UpperCamelCase

let anAnimal = new AnimalSoSweet();    // LowerCamlCase
function sendARequest(requestToSend){};// LowerCamlCase

const NB_MS_IN_HOUR = 3600;            // SCREAMING_SNAKE_CASE

Use explanatory variables

const cityStateRegex = /^(.+)[,\\s]+(.+?)\s*(\d{5})?$/;
saveCityState(
cityStateRegex.match(cityStateRegex)[1], 
cityStateRegex.match(cityStateRegex)[2]
);
const cityStateRegex = /^(.+)[,\\s]+(.+?)\s*(\d{5})?$/;
const match = cityStateRegex.match(cityStateRegex)
const city = match[1];
const state = match[2];
saveCityState(city, state);

Avoid Mental Mapping

const locations = ['Austin', 'New York', 'San Francisco'];
locations.forEach((l) => {
  doStuff();
  doSomeOtherStuff();
  // ...
  // ...
  // ...
  // Wait, what is `l` for again?
  dispatch(l);
});
const locations = ['Austin', 'New York', 'San Francisco'];
locations.forEach((location) => {
  doStuff();
  doSomeOtherStuff();
  // ...
  // ...
  // ...
  dispatch(location);
});

Don't add unneeded context

const Car = {
  carMake: 'Honda',
  carModel: 'Accord',
  carColor: 'Blue'
};

function paintCar(car) {
  car.carColor = 'Red';
}
const Car = {
  make: 'Honda',
  model: 'Accord',
  color: 'Blue'
};

function paintCar(car) {
  car.color = 'Red';
}

Short-circuiting is cleaner than conditionals

function createMicrobrewery(name) {
  let breweryName;
  if (name) {
    breweryName = name;
  } else {
    breweryName = 'Hipster Brew Co.';
  }
}
function createMicrobrewery(name) {
  const breweryName = name || 'Hipster Brew Co.'
}

Functions

Function arguments (2 or fewer ideally)

function createMenu(title, body, buttonText, cancellable) {
  // ...
}
const menuConfig = {
  title: 'Foo',
  body: 'Bar',
  buttonText: 'Baz',
  cancellable: true
}

function createMenu(menuConfig) {
  // ...
}

Functions should do one thing (Small !)

function emailClients(clients) {
  clients.forEach(client => {
    const clientRecord = database.lookup(client);
    if (clientRecord.isActive()) {
      email(client);
    }
  });
}
function emailClients(clients) {
  clients
    .filter(isClientActive)
    .forEach(email);
}

function isClientActive(client) {
  const clientRecord = database.lookup(client);
  return clientRecord.isActive();
}

Don't use flags as function parameters

function createFile(name, temp) {
  if (temp) {
    fs.create('./temp/' + name);
  } else {
    fs.create(name);
  }
}
function createFile(name) {
  fs.create(name);
}

function createTempFile(name) {
  createFile('./temp/' + name);
}

Function names should say what they do

function dateAdd(date, month) {
  // ...
}

const date = new Date();

/* It's hard to to tell from the 
   function name what is added
*/
dateAdd(date, 1);
function dateAddMonth(date, month) {
  // ...
}

const date = new Date();
dateAddMonth(date, 1);

Function should not have side effect

let name = 'Ryan McDermott';

function splitIntoFirstAndLastName() {
  name = name.split(' ');
}

splitIntoFirstAndLastName();
function splitIntoFirstAndLastName(name) {
  return name.split(' ');
}

const name = 'Ryan McDermott'
const newName = splitIntoFirstAndLastName(name);

Use default arguments instead of short circuiting

function writeForumComment(subject, body) {
  subject = subject || 'No Subject';
  body = body || 'No text';
}
function writeForumComment(
    subject = 'No subject', 
    body = 'No text') {
  // ...
}

Encapsulate conditionals

if (fsm.state === 'fetching' && isEmpty(listNode)) {
  // ...
}
function shouldShowSpinner(fsm, listNode) {
  return fsm.state === 'fetching' && isEmpty(listNode);
}

if (shouldShowSpinner(fsmInstance, listNodeInstance)) {
  // ...
}

Sort correctly callers and callee

function doVerySpecificThing(){
    //...
}

function doSpecificThing(){
    //...
    doVerySpecificThing();
}

function doThing(){
    //...
    doSpecificThing();
}
function doThing(){
    //...
    doSpecificThing();
}

function doSpecificThing(){
    //...
    doVerySpecificThing();
}

function doVerySpecificThing(){
    //...
}

Prefer functional programming over imperative

const programmerOutput = [
  { name: 'Uncle Bobby', linesOfCode: 500 },
  { name: 'Suzie Q', linesOfCode: 1500 }
  //....
];

let totalOutput = 0;

for (let i = 0; i < programmerOutput.length; i++) {
  totalOutput += programmerOutput[i].linesOfCode;
}
const programmerOutput = [
  { name: 'Uncle Bobby', linesOfCode: 500 },
  { name: 'Suzie Q', linesOfCode: 1500 }
  //....
];

const totalOutput = programmerOutput
  .map((programmer) => programmer.linesOfCode)
  .reduce((acc, linesOfCode) => acc + linesOfCode, 0);

Prefer polymorphism over lot of conditionals

class Airplane {
  // ...
  getCruisingAltitude() {
    switch (this.type) {
      case '777':
        return this.getMaxAltitude() - this.getPassengerCount();
      case 'Air Force One':
        return this.getMaxAltitude();
      case 'Cessna':
        return this.getMaxAltitude() - this.getFuelExpenditure();
    }
  }
}
class Airplane {}

class Boeing777 extends Airplane {
  getCruisingAltitude() {
    return this.getMaxAltitude() - this.getPassengerCount();
  }
}

class AirForceOne extends Airplane {
  getCruisingAltitude() {
    return this.getMaxAltitude();
  }
}

Never ever lets dead or commented code !!!

// function oldRequestModule(url) {
  // ...
// }

function newRequestModule(url) {
  // ...
}

const req = newRequestModule;
inventoryTracker('apples', req, 'www.inventory-awesome.io');
function newRequestModule(url) {
  // ...
}

const req = newRequestModule;
inventoryTracker('apples', req, 'www.inventory-awesome.io');

Classes

Single responsibility

Open-closed

Liskov substitution

Interface segregation

Dependency injection

SOLID principles

en.wikipedia.org/wiki/SOLID_(object-oriented_design)

Single Responsibility Principle (SRP)

class UserService {
    saveUser(user){
        eventEmmitter.dispatch(ADD_USER, user);
        return db.exec('INSERT VALUES ...', user);
    }
}
class UserResource {
    //...
    save(user){
        return db.exec('INSERT VALUES ...', user);
    }
}
class UserService {
    //...
    saveUser(user){
        eventEmmitter.dispatch(ADD_USER, user);
        return userRessource.save(user)
    }
}

"There should never be more than one reason for a class to change"

Open/Closed Principle (OCP)

class ExpenseController {
  constructor() {}

  addNewExpense(expense) {
   if (this.validateExpense(expense)){
      expenseService.save(expense);
   }
  }
  validateExpense (expense){
    return !!expense.description;
  }
}
class UserAuth {
  constructor(user) {
    this.user = user;
  }

  verifyCredentials() {
    // ...
  }
}
class ExpenseController {
  constructor(validators) {this.validators=validators;}

  addNewExpense(expense) {
   if (this.validateExpense(expense)){
      expenseService.save(expense);
   }
  }
  validateExpense (expense){
    return this.validators.every(validator => {
        return validator.isValid(expense));
    });
  }
}

"open for extension, but closed for modification"

Liskov substitution principle (LSP)

class Rectangle {
  //...
  setWidth(width) {
    this.width = width;
  }

  setHeight(height) {
    this.height = height;
  }

  getArea() {
    return this.width * this.height;
  }
}
class Square extends Rectangle {
  // ...
  setWidth(width) {
    this.width = width;
    this.height = width;
  }

  setHeight(height) {
    this.width = height;
    this.height = height;
  }
}
function renderLargeRectangles(rectangles) {
  rectangles.forEach((rectangle) => {
    rectangle.setWidth(4);
    rectangle.setHeight(5);
    const area = rectangle.getArea();//Will return 25 for Square. Should be 20.
    rectangle.render(area);
  });
}

const rectangles = [new Rectangle(), new Rectangle(), new Square()];
renderLargeRectangles(rectangles);

Liskov substitution principle (LSP)

class Shape {
  setColor(color) {
    // ...
  }

  render(area) {
    // ...
  }
}
class Rectangle extends Shape {
  constructor() {
    super();
    this.width = 0;
    this.height = 0;
  }

  setWidth(width) {
    this.width = width;
  }

  setHeight(height) {
    this.height = height;
  }

  getArea() {
    return this.width * this.height;
  }
}
class Square extends Shape {
  constructor() {
    super();
    this.length = 0;
  }

  setLength(length) {
    this.length = length;
  }

  getArea() {
    return this.length * this.length;
  }
}
function renderLargeShapes(shapes) {
  shapes.forEach((shape) => {
    switch (shape.constructor.name) {
      case 'Square':
        shape.setLength(5);
        break;
      case 'Rectangle':
        shape.setWidth(4);
        shape.setHeight(5);
    }

    const area = shape.getArea();
    shape.render(area);
  });
}

Interface segregation principle (ISP)

class ACar {
    run(){
        throw new Error('SubType of car should implement run');
    }
    fillFuel(){
        throw new Error('SubType of car should implement fillFuel');
    }
}
class MyOldCar extends ACar{
    run(){
        // run ...
    }
    fillFuel(){
        // handle fillFuel
    }
}
class MyElectricCar extends ACar{
    run(){
        // run ...
    }
    fillFuel(){
        // Not necessary 
    }
}

"Clients should not be forced to depend upon interfaces that they do not use."

Interface segregation principle (ISP)

class ACar {
    run(){
        throw new Error('SubType of car should implement run');
    }
}

const FuelCar = (Superclass) => class extends Superclass {
    fillFuel() {
        throw new Error('SubType of FuelCar should implement fillFuel');
    }
}
class MyOldCar extends FuelCar(ACar){
    run(){
        // run ...
    }
    fillFuel(){
        // handle fillFuel
    }
}

const ElectricCar = (Superclass) => class extends Superclass {
    reload() {
        throw new Error('SubType of ElectricCar should implement reload');
    }
}
class MyElectricCar extends ElectricCar(ACar){
    run(){
        // run ...
    }
    reload(){
        // handle reload
    }
}

"Clients should not be forced to depend upon interfaces that they do not use."

Dependency Inversion Principle (DIP)

"1. High-level modules should not depend on low-level modules. Both should depend on abstractions.

2. Abstractions should not depend upon details. Details should depend on abstractions."

class UserController {
  constructor(){
    this.userService = new UserService();
  }
  save(user){
    this.userService.save(user);
  }
}
class UserController {
  constructor(userService){
    this.userService = userService;
  }
  save(user){
    this.userService.save(user);
  }
}

Use method chaining

"1. High-level modules should not depend on low-level modules. Both should depend on abstractions.

2. Abstractions should not depend upon details. Details should depend on abstractions."

class User {
  constructor(name, age){
    this.name = name;
    this.age = age;
  }
  setName(name){
    this.name = name;
  }
  setAge(age){
    this.age = age;
  }
}

const user = new User();
user.setName('Mathieu');
user.setAge('28');
class User {
  constructor(name, age){
    this.name = name;
    this.age = age;
  }
  setName(name){
    this.name = name;
    return this;
  }
  setAge(age){
    this.age = age;
    return this;
  }
}

const user = new User()
              .setName('Mathieu')
              .setAge('28');

Miscellaneous

 

Only comment things that have business logic complexity.

function hashIt(data) {
  // The hash
  let hash = 0;

  // Length of string
  const length = data.length;

  // Loop through every character in data
  for (let i = 0; i < length; i++) {
    // Get character code.
    const char = data.charCodeAt(i);
    // Make the hash
    hash = ((hash << 5) - hash) + char;
    // Convert to 32-bit integer
    hash &= hash;
  }
}

"Comments are an apology, not a requirement. Good code mostly documents itself."

function hashIt(data) {
  let hash = 0;
  const length = data.length;

  for (let i = 0; i < length; i++) {
    const char = data.charCodeAt(i);
    hash = ((hash << 5) - hash) + char;
    // Convert to 32-bit integer
    hash &= hash;
  }
}

Bad comments

/**
 * 2016-12-20: Removed monads, didn't understand them (RM)
 * 2016-10-01: Improved using special monads (JP)
 * 2016-02-03: Removed type-checking (LI)
 * 2015-03-14: Added combine with type-checking (JR)
 */
////////////////////////////////////////////////////////////////////////////////
// Scope Model Instantiation
////////////////////////////////////////////////////////////////////////////////
$scope.model = {
  menu: 'foo',
  nav: 'bar'
};
while (toto < MAX_VALUES){
    if (test > MIN_VALUES){
    } // if
} // while

Follow
the clean
code rules

slides.com/mbreton/clean-code-javascript

github.com/ryanmcdermott/clean-code-javascript