Common anti-patterns

in JavaScript apps

Volodymyr Vyshko

Volodymyr Vyshko

Lead Software Engineer at GlobalLogic

9 years in IT

GitHub, LinkedIn, FB group

Frontend, node.js backend,  java backend, scrum master, lead

when not to use node.js, database choice, MERN stack 

Agenda

React

React antipatterns,

Redux

Common language-specific antipatterns

Backend

JavaScript

Frontend

Do not use global variables.

Always declare import statements at the top.

Do not use variables without declaration.

Do not use constructors when creating arrays and objects.

Do not use the == to check for equality.

Do not omit the curly braces ({…}).

Do not omit the second parameter, base, of parseInt.

Do not omit the break clauses in switch statements.

Do not use for-in to iterate through an array.

Do not use delete to delete an element from an array.

Do not process any task unrelated to the iteration inside the loop.

Do not use continue within the loop.

Do not use try-catch in a loop.

Do not search for the same DOM element more than once.

Minimize DOM Manipulation.

Do not initiate unnecessary layouts.

Do not use the in-line method to deal with events.

Do not use eval().

Do not use with().

When using setTimeout or setInterval, do not use strings as callbacks.

Do not use new Function() constructor.

Do not override or extend native objects.

Do not use increment/decrement operators.

Do not use this assign.

Do not omit the semicolon(;) at the end of each statement.

Do not use variables and functions before declaration. (ES5)

Do not reference the array.length property for each turn in the iteration. (Legacy)

An antipattern is just like a pattern, except that instead of a solution it gives something that looks superficially like a solution but isn't one.


Andrew Koenig (C Traps and Pitfalls)

Microservices

With Node.js you can quickly start building scalable microservice-based applications

But just because you can, doesn't mean you should do it always

Jason Statham

Let’s admit it. Not all applications are large enough to be broken down into smaller services.

 Always start with microservices

Anti-pattern #1

The distributed monolith

Anti-pattern #2

Monolithic application

Microservices

We are agile!

And we do not have to plan things anymore!

Everything Micro (Except for the Data)

Anti-pattern #3

The agile Frankenstein

Anti-pattern #4

If you can't define boundaries in your monolith, then this problem will naturally flow into your microservices and that is even a bigger disaster.

Nest.js modules

import { Module } from '@nestjs/common';
import { CatsController } from './cats.controller';
import { CatsService } from './cats.service';

@Module({
  controllers: [CatsController],
  providers: [CatsService],
})
export class CatsModule {
  constructor(private catsService: CatsService) {}
}
@Controller()
export class CatsController {
  @Get('cats')
  findAll(@Req() request: Request): any {
    const dragonId = message.dragonId;
    const items = [
      { id: 1, name: 'Mythical Sword' },
      { id: 2, name: 'Key to Dungeon' },
    ];
    return items;
  }
}

@Controller()
export class HeroesController {
  @MessagePattern('cats')
  killDragon(@Payload() request: KillDragonMessage): any {
    const dragonId = message.dragonId;
    const items = [
      { id: 1, name: 'Mythical Sword' },
      { id: 2, name: 'Key to Dungeon' },
    ];
    return items;
  }
}

Analyze the project requirements, lifespan and in-house expertise

Apply SOLID principles

Plan! Use "Clean/layered/etc" architecture and DDD

Event-loop

const express = require("express")

const app = express();

app.get("/getfibonacci", (req, res) => {
  const startTime = new Date()
  const result = fibonacci(parseInt(req.query.number))
  const endTime = new Date()
  res.json({
    number: parseInt(req.query.number),
    fibonacci: result,
    time: endTime.getTime() - startTime.getTime() + "ms",
  })
});

const fibonacci = n => {
  if (n <= 1) {
    return 1
  }
  return fibonacci(n - 1) + fibonacci(n - 2)
}

app.listen(3000, () => console.log("listening on port 3000"))

What's wrong with this code?

JavaScript is “single-threaded” because JavaScript works on “Event Loop”

NodeJS is NOT “single-threaded” as it has a libuv threadpool in its runtime.

The chefs are the libuv threadpool and OS Async Helpers.

The waiter is the V8 engine and the Event Loop

The JavaScript code you want to be executed is the food

Node.js restorant

setImmediate()

setImmediate(callback[, ...args]) takes a callback and add it to the event queue( specifically the immediate queue).

function fib(n, cb) {
  if (n <= 1) {
    return cb(n);
  }

  Promise.all([
    new Promise((res) => {
      setImmediate(() => {
        fib(n - 1, res);
      });
    }),

    new Promise((res) => {
      setImmediate(() => {
        fib(n - 2, res);
      });
    }),
  ]).then((results) => cb(results[0] + results[1]));
}
const express = require("express");
const fs = require("fs");

const app = express();

app.get("/file", (req, res) => {
    let file = fs.readFileSync("package.json", "utf8");

    res.setHeader('Content-Length', file.length);
    res.setHeader('Content-disposition', 'attachment; filename=test.txt');
    res.setHeader('Content-type', 'text/html');

    res.write(file, 'binary');
  
    res.end();
})

Let's find an issue here

readFileSync also blocks any other code from execution

Heavy calculations in main thread

Anti-pattern #5

Syncronous I/O

Anti-pattern #6

const express = require("express");
const fs = require("fs");

const app = express();

app.get("/file", (req, res) => {
    fs.readFile("package.json", "utf8", (error, data) => {
        res.setHeader('Content-Length', data.length);
        res.setHeader('Content-disposition', 'attachment; filename=test.txt');
        res.setHeader('Content-type', 'text/html');
    
        res.write(data, 'binary');
        res.end();
    });
})

Is that solved an issue?

Well partially...

fs.createReadStream, on the other hand, reads the entire file in chunks of sizes that you specified

fs.readfile loads the whole file into the memory you pointed out

const {
  Worker,
  isMainThread,
  parentPort,
  workerData,
} = require("worker_threads");

function fib(n) {
  if (n <= 1) return n;
  return fib(n - 1) + fib(n - 2);
}

if (isMainThread) {
  function calc(n) {
    return new Promise((resolve, reject) => {
      const worker = new Worker(__filename, {
        workerData: n,
      });
      worker.on("message", resolve);
      worker.on("error", reject);
      worker.on("exit", (code) => {
        if (code !== 0)
          reject(new Error(`Worker stopped with exit code ${code}`));
      });
    });
  }

  calc(43).then((data) => console.log(data));
} else {
  const n = workerData;
  parentPort.postMessage(fib(n));
}

Thread worker

All or nothing! Reading the whole file at once.

Anti-pattern #7

Overusing threads/processes

Anti-pattern #8

Apply macro tasks to split calculations, use threads and processes

Always use async I/O

Prefer streams over atomic files operations

Use thread pool or cluster

MERN

Slack

Strong Consistency offers up-to-date data but at the cost of high latency

WAIT FOR THE INTERNET TO START WORKING AGAIN

Eventual consistency offers low latency but may reply to read requests with stale data since all nodes of the database may not have the updated data.

Anser based on available data

Available

Not available

Mongo general-purpose database as a default choice

Anti-pattern #9

'use strict';

const express = require('express');
const bodyParser = require('body-parser');
const pg = require('pg');
const hash = require('./hash.js');

const PORT = 8000;

const app = express();

const pool = new pg.Pool({
  host: '127.0.0.1',
  port: 5432,
  database: 'example',
  user: 'marcus',
  password: 'marcus',
});

app.use(bodyParser.json());

app.use(bodyParser.urlencoded({ extended: true }));

app.get('/user', (req, res) => {
  console.log(`${req.socket.remoteAddress} GET /user`);
  pool.query('SELECT * FROM users', (err, data) => {
    if (err) throw err;
    res.status(200).json(data.rows);
  });
});

app.post('/user', async (req, res) => {
  const { login, password } = req.body;
  const user = JSON.stringify({ login, password });
  console.log(`${req.socket.remoteAddress} POST /user ${user}`);
  const sql = 'INSERT INTO users (login, password) VALUES ($1, $2)';
  const passwordHash = await hash(password);
  pool.query(sql, [login, passwordHash], (err, data) => {
    if (err) throw err;
    res.status(201).json({ created: data.insertId });
  });
});

app.get('/user/:id', (req, res) => {
  const id = parseInt(req.params.id);
  console.log(`${req.socket.remoteAddress} GET /user/${id}`);
  pool.query('SELECT * FROM users WHERE id = $1', [id], (err, data) => {
    if (err) throw err;
    res.status(200).json(data.rows);
  });
});

app.put('/user/:id', async (req, res) => {
  const id = parseInt(req.params.id);
  const { login, password } = req.body;
  const user = JSON.stringify({ login, password });
  console.log(`${req.socket.remoteAddress} PUT /user/${id} ${user}`);
  const sql = 'UPDATE users SET login = $1, password = $2 WHERE id = $3';
  const passwordHash = await hash(password);
  pool.query(sql, [login, passwordHash, id], (err, data) => {
    if (err) throw err;
    res.status(201).json({ modified: data.insertId });
  });
});

app.delete('/user/:id', (req, res) => {
  const id = parseInt(req.params.id);
  console.log(`${req.socket.remoteAddress} DELETE /user/${id}`);
  pool.query('DELETE FROM users WHERE id = $1', [id], (err, data) => {
    if (err) throw err;
    res.status(200).json({ deleted: data.insertId });
  });
});

app.listen(PORT, () => {
  console.log(`Listen on port ${PORT}`);
});

Find an issue here

Mixing up logical layers

Anti-pattern #10

Clean architecture

Express motivates devs to use mixins

var express = require('express');
var app = express();

var requestTime = function (req, res, next) {
  req.requestTime = Date.now();
  next();
};

app.use(requestTime);

app.get('/', function (req, res) {
  var responseText = 'Hello World!';
  responseText += 'Requested at: ' + req.requestTime + '';
  res.send(responseText);
});

app.listen(3000);

Official guide

Mixins in middlewares

Anti-pattern #11

Polluting the global namespace

Anti-pattern #12

const EventEmitter = require('events');

function fib(n) {
    if (n <= 1) { return 1 };
    return fib(n - 1) + fib(n - 2);
}

const myEmitter = new EventEmitter();

myEmitter.on('calculate', (number) => {
  const result = fib(number);
  myEmitter.emit('results', result)
});

// other file

myEmitter.on('results', (number) => {
  console.log('The result is', number);
  myEmitter.removeListener('calculate');
});

myEmitter.emit('calculate', 42);

EventEmitter

Emit from the event-listener

Anti-pattern #13

Removing event listeners by other code

Anti-pattern #14

Async handling with an EventEmitter

Anti-pattern #15

Use CAP when choosing a DB

Do not mix up event handlers, apply SOLID

Do not add mixins in middlewares

JavaScript

const data = [1, 3, 8, 11];

const results = [];

data.map(item => {
  if(item % 2 === 0) { 
  	results.push(item);
  }
});


results.forEach((data, i, self) => {
  self[i] = data * 10;
})

return results;

map & forEach

class User {
    generateHash() { ... }
    getNameFormatted() { ... }
    banUser() { ... }
    getWifeName() { ... }
    getChildrenInfo() { ... }
    getLogin() { ... }
    getPassword() { ... }
    getBirthdayInfo() { ... }
    isUserLikesStarWars() { ... }
    sendEmailWithCats() { ... }
    rmRf() { ... }
    orderThePizza() { ... }
    callThePolice() { ... }
    installCyberpunk() { ... }
    makeWorkout() { ... }
    buySomeBeer() { ... }
    isUserDead() { ... }
    isPunk() { ... }
    extractBrain() { ... }
    checkHasPants() { ... }
    findBankAccount() { ... }
    getInfoHowMuchBitcoins() { ... }
    ...
}

This is a bit of a silly example, but it's a God object nonetheless.

Swiss knife, god object

Debugging

Testing

Extending

Reasons why God objects are bad

 nightmare to walkthrough heaps of code

If you’ve ever written unit tests, you will understand the pain

Tightly coupled classes!

Misuse of .map and .forEach

Anti-pattern #16

God object

Anti-pattern #17

function getUser() {
    //...
  
    if (something) {
       return null
    }
    return user;
}

function getInfo(user) {
  if (user !== null) {
    //...
  }
  return null;
}

function getDetails() {
  const info = getInfo(getUser());
  if (info === null) {
    throw new Exception()
  }
  //...
}

Sometimes, the easiest way to manage null is to not use it

Return null

  • What if this API returns null?
  • If so, what does null mean?
  • Does this API accept null?
  • Does null represent an error or normal behaviour?
  • Eventually, you will see null checks everywhere, especially when dealing with external code.
const Foo = (function () {
    "use strict";
    let instance; //prevent modification of "instance" variable
    function Singleton() {
        if (instance) {
            return instance;
        }
        instance = this;
        //Singleton initialization code
    }
    // Instance accessor
    Singleton.getInstance = function () {
        return instance || new Singleton();
    }
    return Singleton;
}());

Foo.getInstance();

Lazy singleton

In short, the singleton pattern makes code more complex, less useful, and a real pain to re-use or test. Eliminating singletons can be tricky, but it’s a worthwhile endeavour.

Return null

Anti-pattern #18

Lazy singleton

Anti-pattern #19

class Rabbit { 
  constructor(name) {
    this.name = name;
  }

  sayHello() {
    console.log(`Hello, i am ${this.name}`);
  }
}

const r = new Rabbit('Oleg');

r.sayHello()
// Hello, i am Oleg

Rabbit.prototype.sayHello = () => console.log('Hey');

r.sayHello()
// Hey

Class defines an instantiable type

Prototype is a living instance

Similar to polluting the global namespace

Do not override methods, Do not add new methods, Do no remove existing methods

Possibility of naming collisions and incompatible implementations

Inheritance

Using inheritance is not the only way to extend a class behavior. But definitely is the most dangerous and harmful one.

Lack of features

Context binding

Problems behind JS classes

Optimization 

JavaScript is not a class-based object-oriented language

Overuse of inheritance and classes

Anti-pattern #20

Modifying prototype (of external classes)

Anti-pattern #21

React

class App extends Component {
  render() {
    const styles = { 
      width: "400px",
      height: "400px",
      position:  "relative" 
    };
  }
}

Constant placement in components

 Many developers tend to define constant object literals and arrays inside the render methods in react

class MyComponent extends Component {
  constructor(props) {
    super(props);
    this.state = {
      counter: 350
    };
  }
  updateCounter() {
    // this line will not work
    this.state.counter = this.state.counter + this.props.increment;
 
    // this will not work as intended
    this.setState({
      counter: this.state.counter + this.props.increment; 
    });
    
  }
  //...
}

Using setState

The setState is pretty simple & straight forward concept in React. But developers might over look some of the nuisances when using setState

function useCounter() {
  const [count, setCount] = useState(0)

  const increment = useCallback(() => setCount(count + 1), [count])
  const decrement = useCallback(() => setCount(count - 1), [count])

  return { count, increment, decrement }
}

// good

function useCounter() {
  const [count, setCount] = useState(0)

  const increment = useCallback(() => setCount(x => x + 1), [])
  const decrement = useCallback(() => setCount(x => x - 1), [])

  return { count, increment, decrement }
}
{elements.map((element, index) =>
    <Display
       {...element}
       key={index}
       />
   )
}

// then
elements.splice(3, 1);

Indexes as keys

Using certain set of values such as array indexes may break your application or render wrong data.

useEffect(() => setCurrentCount(`count is ${count}`), [count, currentCount]);

// instead
// 
 const increment = () => {
    const newCount = count + 1;
    setCount(newCount);
    currentCount(`count is ${newCount}`);
  }
  const decrement = () => {
    const newCount = count - 1;
    setCount(newCount);
    currentCount(`computed count is ${newCount}`);
  }

Using useEffect to detect state changes

Why add extra work like running an effect when you already know the next value? Instead, the recommended solution is to either use one variable instead of two (since one can be calculated from the other one, it seems), or to calculate next value first and update them both using it together.

Dan Abramov

Defining constans in render

Anti-pattern #22

Treating setState as sync

Anti-pattern #23

Indexes as keys

Anti-pattern #24

useEffect for state updates

Anti-pattern #25

Making things components that don’t render anything 

PureComponent and React.memo() everywhere because it's faster

Calling setState() in a way that causes an infinite loop of rerenders, for example in

Putting things in state which don't affect rendering,

Refs everywhere

Excessive prop, state and everything destructuring because we want to show off our es6 knowledge

Component methods that return JSX scattered all over the file

{someArray.length && someArray.map(...)}

Passing props down many levels

Too many levels of components

Not using propTypes and defaultProps

Redux

function mapStateToProps(state) {
 return {
    aRandomProp: Math.random()
 }
}

function mapStateToProps(state) {
 return {
    aFilteredArray: state.anArray.filter(value => value.isTrue === true)
 }
}

Creating New Objects In mapStateToProps

Use selectors, memoization and global state instead

React state

Redux

New objects in mapState

Anti-pattern #26

Keeping everything in the Redux

Anti-pattern #27

(link)

Redux doesn't solve any problem. It just creates a new problem by giving you an abstract box. And what to store is that box - its your problem.

Dan Abramov

Redux Google trend worldwide for the last year

October 2019

October 2020

25%

50%

75%

100%

link

25%

Why the popularity has decreased?

Learning curve

Boilerplate code

Performance

Central store

No built-in way to handle side-effects

State of JS 2020

Should we stop using Redux?

Is it dead ?

Use react features/hooks instead ?

TL;DR

Is Redux dead, dying, deprecated, or about to be replaced?

No.

Use Redux when

You need a single source of truth

You want to maintain an undo history

You need a serializable state/actions

Travel between the state history in development

Provide alternative UI without disturbing too much of the business logic

Redux overuse

Anti-pattern #27

 Always start with microservices

The distributed monolith

Everything Micro (Except for the Data)

The agile Frankenstein

Heavy calculations in main thread

Syncronous I/O

All or nothing! Reading the whole file at once.

Overusing threads/processes

Mongo general-purpose database as a default choice

Mixing up logical layers

Mixins in middlewares

Polluting the global namespace

Emit from the event-listener

Removing event listeners by other code

Async handling with an EventEmitter

Defining constans in render

Treating setState as sync

Indexes as keys

useEffect for state updates

New objects in mapState

Keeping everything in the Redux

Redux overuse

Overuse of inheritance and classes

Modifying prototype (of external classes)

Return null

Lazy singleton

Misuse of .map and .forEach

God object

Refs evrywhere

Many react levels

vars in state

We are on the top of the Iceberg

Thank

You!

Made with Slides.com