Common anti-patterns
in JavaScript apps
Volodymyr Vyshko
Volodymyr Vyshko
Lead Software Engineer at GlobalLogic
9 years in IT
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
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.
Do not initiate unnecessary layouts.
Do not use the in-line method to deal with events.
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 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()
// HeyClass 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%
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!
JS Antipatterns
By Vladimir Vyshko
