Clean Code JavaScript

Question 1

Are function callers and callees located close to each other?

Look at this code. Can you spot the problems with its organization?

class PerformanceReview {
constructor(employee) {
this.employee = employee;
}

lookupPeers() {
return db.lookup(this.employee, 'peers');
}

lookupManager() {
return db.lookup(this.employee, 'manager');
}

getPeerReviews() {
const peers = this.lookupPeers();
// ...
}

perfReview() {
this.getPeerReviews();
this.getManagerReview();
this.getSelfReview();
}

getManagerReview() {
const manager = this.lookupManager();
}

getSelfReview() {
// ...
}
}

const review = new PerformanceReview(employee);
review.perfReview();

🔍 Hints

Think about:

How are related functions positioned relative to each other?

Which functions call other functions?

How many times do you need to scroll up and down to understand the flow?

What would make the code easier to read?

Accepted Answer

The Principle:

Code organization impacts readability and maintainability. Functions that work together should be located close to each other in the source file. This principle follows natural reading patterns and makes it easier to understand the flow of the program.

Solution:

Here's how to better organize the code:

class PerformanceReview {
constructor(employee) {
this.employee = employee;
}

// Main workflow function at the top
perfReview() {
this.getPeerReviews();
this.getManagerReview();
this.getSelfReview();
}

// Grouped related functions together
getPeerReviews() {
const peers = this.lookupPeers();
// ...
}

lookupPeers() {
return db.lookup(this.employee, 'peers');
}

getManagerReview() {
const manager = this.lookupManager();
}

lookupManager() {
return db.lookup(this.employee, 'manager');
}

getSelfReview() {
// ...
}
}

const review = new PerformanceReview(employee);
review.perfReview();

Why is this better?

The main workflow function (perfReview) is at the top

Helper functions are grouped with the functions that call them

Related functions are kept together (e.g., getManagerReview next to lookupManager)

The code flows more naturally, reducing the need to jump around while reading

Question 2

Are you maintaining consistent capitalization in your code?

Look at this code. What consistency issues do you spot?

const DAYS_IN_WEEK = 7;
const daysInMonth = 30;

const songs = ['Back In Black', 'Stairway to Heaven', 'Hey Jude'];
const Artists = ['ACDC', 'Led Zeppelin', 'The Beatles'];

function eraseDatabase() {}
function restore_database() {}

class animal {}
class Alpaca {}

🔍 Hints

Think about:

How are constants named?

What's the pattern for function names?

What's the convention for class names?

Are similar types of variables using the same capitalization?

Accepted Answer

The Principle:

Consistent capitalization serves as a visual cue for the purpose and behavior of code elements in JavaScript. Since JavaScript is untyped, these naming conventions help developers quickly understand the role of different variables, functions, and classes. Following consistent patterns makes code more readable and maintainable.

Solution:

Here's the consistently formatted version:

const DAYS_IN_WEEK = 7;
const DAYS_IN_MONTH = 30;

const SONGS = ['Back In Black', 'Stairway to Heaven', 'Hey Jude'];
const ARTISTS = ['ACDC', 'Led Zeppelin', 'The Beatles'];

function eraseDatabase() {}
function restoreDatabase() {}

class Animal {}
class Alpaca {}

Why is this better?

This code follows clear conventions:

UPPER_CASE for constants

PascalCase for class names

camelCase for functions and variables

Consistent naming patterns for similar elements

The consistent formatting makes the code more predictable and easier to read at a glance.

Question 3

Are you handling rejected Promises correctly?

Look at this Promise error handling. What's missing from this approach?

getdata()
.then((data) => {
functionThatMightThrow(data);
})
.catch((error) => {
console.log(error);
});

🔍 Hints

Think about:

Is just logging enough for production code?

What happens after the error is caught?

How would you handle temporary failures?

What information do users receive?

Accepted Answer

The Principle:

Proper handling of Promise rejections is crucial for maintaining robust applications. Just like with try/catch blocks, ignoring Promise rejections can lead to silent failures, making debugging difficult and potentially leaving your application in an inconsistent state.

Solution:

Here's a more robust approach:

getdata()
.then((data) => {
functionThatMightThrow(data);
})
.catch((error) => {
// Detailed logging for debugging purposes
console.error(error);

// Provide user-friendly error messages
notifyUserOfError(error);

// Log to error tracking service for monitoring
reportErrorToService(error);

// Consider retrying the operation if appropriate
// retry(operation, retryCount);
});

Why is this better?

This approach provides comprehensive error handling with proper logging, user feedback, and the possibility of recovery strategies like retrying operations.

Question 4

Are you properly handling caught errors?

Look at this error handling code. What's problematic about this approach?

try {
functionThatMightThrow();
} catch (error) {
console.log(error);
}

🔍 Hints

Think about:

What happens to this error after it's logged?

How would you debug issues in production?

How would users know something went wrong?

What information are you capturing about the error?

Accepted Answer

The Principle:

Proper error handling is crucial for maintaining robust applications. Simply catching errors without handling them appropriately can lead to silent failures and make debugging extremely difficult.

Solution:

Here's a better approach:

try {
functionThatMightThrow();
} catch (error) {
// Detailed error logging for debugging
console.error(error);

// Inform the user in a user-friendly way
notifyUserOfError(error);

// Track errors in your monitoring system
reportErrorToService(error);
// OR do all three!
}

Why is this better?

This approach ensures errors are properly logged, users are informed, and the development team can track and monitor issues effectively.

Question 5

Are you using Async/Await for even cleaner code?

Look at this Promise-based code. While it's better than callbacks, how could we make it even more readable?

import { get } from 'request-promise';
import { writeFile } from 'fs-extra';

get('https://en.wikipedia.org/wiki/Robert_Cecil_Martin')
.then((body) => {
return writeFile('article.html', body);
})
.then(() => {
console.log('File written');
})
.catch((err) => {
console.error(err);
});

🔍 Hints

Think about:

How could we make this read more like synchronous code?

What modern JavaScript feature could help here?

How could we make error handling more familiar?

Accepted Answer

The Principle:

Async/await is a modern JavaScript feature that makes asynchronous code look and behave more like synchronous code. It provides the best readability while maintaining all the benefits of Promises, making code easier to understand and maintain.

Solution:

Here's an even better approach:

import { get } from 'request-promise';
import { writeFile } from 'fs-extra';

async function getCleanCodeArticle() {
try {
const body = await get(
'https://en.wikipedia.org/wiki/Robert_Cecil_Martin',
);
await writeFile('article.html', body);
console.log('File written');
} catch (err) {
console.error(err);
}
}

getCleanCodeArticle();

Why is this better?

The code reads like synchronous code while maintaining asynchronous behavior. The try/catch block provides familiar error handling syntax that most developers are comfortable with.

Question 6

Are you using Promises instead of callbacks?

Look at this code. What problems do you see with this approach?

import { get } from 'request';
import { writeFile } from 'fs';

get(
'https://en.wikipedia.org/wiki/Robert_Cecil_Martin',
(requestErr, response, body) => {
if (requestErr) {
console.error(requestErr);
} else {
writeFile('article.html', body, (writeErr) => {
if (writeErr) {
console.error(writeErr);
} else {
console.log('File written');
}
});
}
},
);

🔍 Hints

Think about:

How easy is it to follow the flow of operations?

What happens to the indentation as more callbacks are added?

How scattered is the error handling?

Accepted Answer

The Principle:

Promises provide a cleaner way to handle asynchronous operations compared to callbacks. They help avoid "callback hell" (deeply nested callbacks) and provide better error-handling mechanisms. Promises make asynchronous code more readable and maintainable.

Solution:

Here's a better way to write it:

import { get } from 'request-promise';
import { writeFile } from 'fs-extra';

get('https://en.wikipedia.org/wiki/Robert_Cecil_Martin')
.then((body) => {
return writeFile('article.html', body);
})
.then(() => {
console.log('File written');
})
.catch((err) => {
console.error(err);
});

Why is this better?

The Promise chain creates a flat structure that's easier to read and maintain. Error handling is consolidated into a single .catch() block, making the code more concise and clearer.

Question 7

Are your tests focused on a single concept?

Look at this test code. What makes it hard to understand and maintain?

import assert from 'assert';

describe('MomentJS', () => {
it('handles date boundaries', () => {
let date;

date = new MomentJS('1/1/2015');
date.addDays(30);
assert.equal('1/31/2015', date);

date = new MomentJS('2/1/2016');
date.addDays(28);
assert.equal('02/29/2016', date);

date = new MomentJS('2/1/2015');
date.addDays(28);
assert.equal('03/01/2015', date);
});
});

🔍 Hints

Think about:

What happens if one assertion fails? How easy is it to identify which case failed?

How many different concepts are being tested in a single test?

What's the story this test is trying to tell?

How could you make the test's purpose clearer?

Accepted Answer

The Principle:

Tests should be clear, focused, and test one specific concept or behavior. This makes tests easier to maintain, debug, and understand when they fail. Each test should tell a story about a specific behavior of your code.

Solution:

Here's a better way to structure these tests:

import assert from 'assert';

describe('MomentJS date handling', () => {
describe('when adding days to a date', () => {
it('correctly handles the last day of a 31-day month', () => {
const date = new MomentJS('1/1/2015');
date.addDays(30);
assert.equal('1/31/2015', date);
});

it('properly handles leap year February', () => {
const date = new MomentJS('2/1/2016');
date.addDays(28);
assert.equal('02/29/2016', date);
});

it('correctly transitions to next month in non-leap year February', () => {
const date = new MomentJS('2/1/2015');
date.addDays(28);
assert.equal('03/01/2015', date);
});
});
});

Why is this better?

Each test has a single, clear purpose

Test names clearly describe the behavior being tested

When a test fails, you immediately know which scenario failed

Tests are organized hierarchically by functionality

Each test tells a complete story

Question 8

Are you applying the Dependency Inversion Principle (DIP)?

Look at this code. What's problematic about how the InventoryTracker is implemented?

class InventoryRequester {
constructor() {
this.REQ_METHODS = ['HTTP'];
}

requestItem(item) {
// ... HTTP request implementation
}
}

class InventoryTracker {
constructor(items) {
this.items = items;
// Bad: Directly creating an instance of a specific requester
this.requester = new InventoryRequester();
}

requestItems() {
this.items.forEach((item) => {
this.requester.requestItem(item);
});
}
}

// Usage:
const inventoryTracker = new InventoryTracker(['apples', 'bananas']);
inventoryTracker.requestItems();

🔍 Hints

Think about:

What if we want to use a different type of requester?

How tightly coupled is InventoryTracker to InventoryRequester?

What if we want to use WebSockets instead of HTTP?

How could we make this more flexible?

Accepted Answer

The Principle:

The Dependency Inversion Principle states that high-level modules should not depend on low-level modules. Both should depend on abstractions. This promotes loose coupling and makes the code more flexible and easier to modify.

Reference:

https://github.com/ryanmcdermott/clean-code-javascript#dependency-inversion-principle-dip

Solution:

Here's a better approach:

class InventoryTracker {
constructor(items, requester) {
this.items = items;
this.requester = requester;
}

requestItems() {
this.items.forEach((item) => {
this.requester.requestItem(item);
});
}
}

class InventoryRequesterHTTP {
constructor() {
this.REQ_METHODS = ['HTTP'];
}

requestItem(item) {
// ... HTTP request implementation
}
}

class InventoryRequesterWS {
constructor() {
this.REQ_METHODS = ['WS'];
}

requestItem(item) {
// ... WebSocket request implementation
}
}

// Usage:
const httpTracker = new InventoryTracker(
['apples', 'bananas'],
new InventoryRequesterHTTP(),
);

const wsTracker = new InventoryTracker(
['apples', 'bananas'],
new InventoryRequesterWS(),
);

httpTracker.requestItems();
wsTracker.requestItems();

Why is this better?

InventoryTracker is no longer tightly coupled to a specific requester

We can easily swap different types of requesters

Testing becomes easier as we can mock the requester

New request methods can be added without modifying existing code

Question 9

Are you adhering to the Interface Segregation Principle (ISP)?

Look at this code. What's problematic about how the DOMTraverser class is designed?

class DOMTraverser {
constructor(settings) {
this.settings = settings;
this.setup();
}

setup() {
this.rootNode = this.settings.rootNode;
this.settings.animationModule.setup();
}

traverse() {
// ... traversal logic
}
}

// Usage:
const $ = new DOMTraverser({
rootNode: document.getElementsByTagName('body'),
animationModule() {}, // Forced to provide animation module even if not needed
// ...
});

🔍 Hints

Think about:

What if we don't need animations?

Are we forcing users to provide unnecessary functionality?

How could we make the animation module optional?

What if we want to add more optional features later?

Accepted Answer

The Principle:

The Interface Segregation Principle states that clients should not be forced to depend on interfaces they don't use. In JavaScript, this means classes shouldn't be forced to implement methods or provide dependencies they don't need.

Reference:

https://github.com/ryanmcdermott/clean-code-javascript#interface-segregation-principle-isp

Solution:

Here's a better approach:

class DOMTraverser {
constructor(settings) {
this.settings = settings;
this.options = settings.options;
this.setup();
}

setup() {
this.rootNode = this.settings.rootNode;
this.setupOptions();
}

setupOptions() {
if (this.options.animationModule) {
// ...
}
}

traverse() {
// ...
}
}

const $ = new DOMTraverser({
rootNode: document.getElementsByTagName('body'),
options: {
animationModule() {},
},
});

Why is this better?

Animation module is now optional

Users only need to provide what they actually use

The code is more flexible and extensible

New optional features can be easily added without breaking existing code

Question 10

Are you following the Liskov Substitution Principle (LSP)?

Look at this code. Can you spot what's wrong with this inheritance relationship?

class Rectangle {
constructor() {
this.width = 0;
this.height = 0;
}

setColor(color) {
// ...
}

render(area) {
// ...
}

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

// This breaks LSP because Square doesn't behave like a Rectangle
function renderLargeRectangles(rectangles) {
rectangles.forEach((rectangle) => {
rectangle.setWidth(4);
rectangle.setHeight(5);
const area = rectangle.getArea(); // BAD: Returns 25 for Square. Should be 20.
rectangle.render(area);
});
}

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

🔍 Hints

Think about*:

What happens when you set width=4 and height=5 on a Square?

Should a Square really extend Rectangle?

What assumptions might programmers make about a Rectangle?

Is a Square really behaving like a Rectangle in all situations?

Accepted Answer

The Principle:

The Liskov Substitution Principle states that objects of a superclass should be replaceable with objects of its subclasses without breaking the application. In other words, if it looks like a duck and quacks like a duck but needs batteries, you probably have the wrong abstraction!

Reference:

https://github.com/ryanmcdermott/clean-code-javascript#liskov-substitution-principle-lsp

Solution:

Here's a better way to model these shapes:

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

render(area) {
// ...
}
}

class Rectangle extends Shape {
constructor(width, height) {
super();
this.width = width;
this.height = height;
}

getArea() {
return this.width  this.height;
}
}

class Square extends Shape {
constructor(length) {
super();
this.length = length;
}

getArea() {
return this.length  this.length;
}
}

function renderLargeShapes(shapes) {
shapes.forEach((shape) => {
const area = shape.getArea();
shape.render(area);
});
}

const shapes = [new Rectangle(4, 5), new Rectangle(4, 5), new Square(5)];
renderLargeShapes(shapes);

Why is this better?

Each shape manages its own properties correctly

Square and Rectangle are separate types with a common ancestor

No unexpected behavior when calculating areas

The code follows "tell, don't ask" principle

Question 11

Are you adhering to the Open/Closed Principle (OCP)?

Look at this code. What problems do you see with its design?

class AjaxAdapter extends Adapter {
constructor() {
super();
this.name = 'ajaxAdapter';
}
}

class NodeAdapter extends Adapter {
constructor() {
super();
this.name = 'nodeAdapter';
}
}

class HttpRequester {
constructor(adapter) {
this.adapter = adapter;
}

fetch(url) {
if (this.adapter.name === 'ajaxAdapter') {
return makeAjaxCall(url).then((response) => {
// transform response and return
});
} else if (this.adapter.name === 'nodeAdapter') {
return makeHttpCall(url).then((response) => {
// transform response and return
});
}
}
}

function makeAjaxCall(url) {
// request and return promise
}

function makeHttpCall(url) {
// request and return promise
}

🔍 Hints

Think about:

What happens if you need to add a new type of adapter?

How many places would you need to modify the code?

Is there a way to make the code more flexible?

How could you avoid checking the adapter type?

Accepted Answer

The Principle:

The Open/Closed Principle states that software entities should be open for extension but closed for modification. This means you should be able to add new functionality without changing existing code, typically achieved through inheritance and polymorphism.

Solution:

Here's a better approach:

class AjaxAdapter extends Adapter {
constructor() {
super();
this.name = 'ajaxAdapter';
}

request(url) {
// request and return promise
}
}

class NodeAdapter extends Adapter {
constructor() {
super();
this.name = 'nodeAdapter';
}

request(url) {
// request and return promise
}
}

class HttpRequester {
constructor(adapter) {
this.adapter = adapter;
}

fetch(url) {
return this.adapter.request(url).then((response) => {
// transform response and return
});
}
}

Why is this better?

Each adapter implements a common interface (the request method)

New adapters can be added without modifying the HttpRequester class

The system is now extensible through inheritance and polymorphism

No need for if/else statements checking adapter types

Question 12

Are you following the Single Responsibility Principle (SRP)?

Look at this code. What's wrong with it?

class UserSettings {
constructor(user) {
this.user = user;
}

changeSettings(settings) {
if (this.verifyCredentials()) {
// ... change settings
}
}

verifyCredentials() {
// ... check user credentials
}
}

🔍 Hints

Think about:

How many different responsibilities does this class have?

What if authentication rules change?

What if settings logic changes?

Could these responsibilities be separated?

What's the "single reason to change" for this class?

Accepted Answer

The Principle:

The Single Responsibility Principle states that a class should have only one reason to change. This means each class should focus on doing one specific thing well, rather than trying to handle multiple responsibilities. This makes code more maintainable, testable, and easier to understand.

Solution:

class UserAuth {
constructor(user) {
this.user = user;
}

verifyCredentials() {
// ... check user credentials
}
}

class UserSettings {
constructor(user) {
this.user = user;
this.auth = new UserAuth(user);
}

changeSettings(settings) {
if (this.auth.verifyCredentials()) {
// ... change settings
}
}
}

Why is this better?

Each class has a single responsibility

Authentication logic is separate from settings management

Changes to auth don't affect settings and vice versa

Easier to test each component independently

More maintainable and flexible design

Question 13

Are you favoring composition over inheritance?

Look at this code. What's wrong with it?

class Employee {
constructor(name, email) {
this.name = name;
this.email = email;
}

// ...
}

// Bad because Employees "have" tax data, they aren't a type of tax data
class EmployeeTaxData extends Employee {
constructor(ssn, salary) {
super();
this.ssn = ssn;
this.salary = salary;
}

// ...
}

🔍 Hints

Think about:

Is an EmployeeTaxData really a type of Employee?

What's the relationship between Employee and tax data?

Would this design be flexible if requirements change?

Is this a "has-a" or "is-a" relationship?

Accepted Answer

The Principle:

Composition and inheritance are two fundamental approaches to code reuse in object-oriented programming. While inheritance is powerful, composition often provides more flexibility and looser coupling between components.

Key reasons to prefer composition:

Clear "has-a" relationships exist

More flexible design

Avoids deep inheritance hierarchies

Easier to change behavior at runtime

Better encapsulation

Reduces coupling between classes

When to use inheritance:

Clear "is-a" relationships exist

Base class behavior can be reused

You need to make global changes across all derived classes

Reference:

https://github.com/ryanmcdermott/clean-code-javascript#prefer-composition-over-inheritance

Solution:

class EmployeeTaxData {
constructor(ssn, salary) {
this.ssn = ssn;
this.salary = salary;
}

// ...
}

class Employee {
constructor(name, email) {
this.name = name;
this.email = email;
}

setTaxData(ssn, salary) {
this.taxData = new EmployeeTaxData(ssn, salary);
}
// ...
}

Why is this better?

Correctly models that an Employee has tax data

More flexible design

Easier to modify either class independently

Better encapsulation

Follows "has-a" relationship pattern

Question 14

Are you using method chaining for fluent interfaces?

Look at this code. What's wrong with it?

class Car {
constructor(make, model, color) {
this.make = make;
this.model = model;
this.color = color;
}

setMake(make) {
this.make = make;
}

setModel(model) {
this.model = model;
}

setColor(color) {
this.color = color;
}

save() {
console.log(this.make, this.model, this.color);
}
}

const car = new Car('Ford', 'F-150', 'red');
car.setColor('pink');
car.setModel('Mustang');
car.save();

🔍 Hints

Think about:

How many lines of code are needed to modify the car?

Is there a way to make this more fluent?

What do popular libraries like jQuery do?

What needs to be returned from each method?

Accepted Answer

The Principle:

Method chaining is a pattern that allows multiple methods to be called in a single statement, making code more fluent and expressive. This pattern is commonly seen in libraries like jQuery and Lodash.

Key benefits of method chaining:

More natural to read and understand

Matches how we think about the condition

Easier to maintain and debug

Question 26

Are you encapsulating conditionals for better readability?

What could make this conditional easier to understand?

if (fsm.state === 'fetching' && isEmpty(listNode)) {
// ...
}

🔍 Hints

Think about:

What is this condition actually checking for?

How could we make the intent clearer?

Could we extract this logic into a well-named function?

How would this help with code reuse?

Accepted Answer

The Principle:

Complex conditional statements can make code harder to read and understand. Extracting conditions into well-named functions improves code readability and reusability.

Solution:

Here's a clearer way to write it:

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

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

Why is this better?

The condition's purpose is immediately clear

Logic can be reused elsewhere

Easier to test the condition independently

More self-documenting code

Easier to modify the condition if requirements change

Question 27

Are you favoring functional programming over imperative programming?

Look at this code for calculating total lines of code. What could be improved?

const programmerOutput = [
{
name: 'Uncle Bobby',
linesOfCode: 500,
},
{
name: 'Suzie Q',
linesOfCode: 1500,
},
{
name: 'Jimmy Gosling',
linesOfCode: 150,
},
{
name: 'Gracie Hopper',
linesOfCode: 1000,
},
];

let totalOutput = 0;

for (let i = 0; i < programmerOutput.length; i++) {
totalOutput += programmerOutput[i].linesOfCode;
}

🔍 Hints

Think about:

What state is being maintained in this code?

How could we make this more declarative?

What array methods could simplify this?

How could we make this more readable?

Accepted Answer

The Principle:

Functional programming leads to more predictable code by avoiding state changes and side effects. It focuses on what should be computed rather than how it should be computed.

Solution:

Here's a more functional approach:

const programmerOutput = [
{
name: 'Uncle Bobby',
linesOfCode: 500,
},
{
name: 'Suzie Q',
linesOfCode: 1500,
},
{
name: 'Jimmy Gosling',
linesOfCode: 150,
},
{
name: 'Gracie Hopper',
linesOfCode: 1000,
},
];

const totalOutput = programmerOutput.reduce(
(totalLines, output) => totalLines + output.linesOfCode,
0,
);

Why is this better?

No mutable variables

More declarative - shows what we want to achieve

Shorter and more expressive

Easier to chain with other operations if needed

Less prone to off-by-one errors

Question 28

Are you avoiding writing to global functions?

What's problematic about this code's approach to extending Array functionality?

Array.prototype.diff = function diff(comparisonArray) {
const hash = new Set(comparisonArray);
return this.filter((elem) => !hash.has(elem));
};

🔍 Hints

Think about:

What happens if different libraries try to add a diff method?

Could this break existing code that uses arrays?

How could we add this functionality without modifying the Array prototype?

What's a more maintainable way to extend functionality?

Accepted Answer

The Principle:

Extending built-in objects through their prototypes (like Array, String, etc.) can lead to naming conflicts and unexpected behavior in your application, especially when multiple libraries are involved.

Reference:

https://github.com/ryanmcdermott/clean-code-javascript#dont-write-to-global-functions

Solution:

Here's a safer way to implement this functionality:

class SuperArray extends Array {
diff(comparisonArray) {
const hash = new Set(comparisonArray);
return this.filter((elem) => !hash.has(elem));
}
}

Why is this better?

Doesn't modify global Array prototype

Functionality is isolated to specific use cases

No risk of naming conflicts with other libraries

Clear where the enhanced functionality comes from

Easier to maintain and test

Question 29

Are you avoiding side effects in your functions? (Part 2)

What potential problems could this cart implementation cause?

const addItemToCart = (cart, item) => {
cart.push({ item, date: Date.now() });
};

🔍 Hints

Think about:

What happens to the original cart array?

Could this cause issues if multiple parts of the code use the same cart?

How would you track changes to the cart?

What's a more predictable way to handle this?

Accepted Answer

The Principle:

Mutable data structures can lead to unintended side effects when shared across different parts of your application. When modifying objects or arrays that are passed as parameters, you risk affecting code elsewhere that uses those same references. Instead, create new copies with your changes to prevent unintended side effects.

Reference:

https://github.com/ryanmcdermott/clean-code-javascript#avoid-side-effects-part-2

Solution:

Here's a safer approach:

const addItemToCart = (cart, item) => {
return [...cart, { item, date: Date.now() }];
};

Why is this better?

Returns a new cart array instead of modifying the existing one

Original cart remains unchanged for other code to use

Easier to track changes (good for state management)

Prevents bugs from unexpected mutations

Makes it easier to implement features like undo/redo

Question 30

Are you avoiding side effects in your functions? (Part 1)

What's wrong with this code's approach to string manipulation?

// Global variable referenced by following function.
// If we had another function that used this name, now it'd be an array and it could break it.
let name = 'Ryan McDermott';

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

splitIntoFirstAndLastName();

console.log(name); // ['Ryan', 'McDermott'];

🔍 Hints

Think about:

What happens to the original name variable?

Could this function cause problems in other parts of the code?

How would you test this function?

What if multiple functions need to use the original name?

Accepted Answer

The Principle:

Side effects are changes that a function makes to any state outside its own scope. These can include modifying global variables, changing input parameters, or affecting the environment. They make code harder to test and can lead to unexpected bugs.

Reference:

https://github.com/ryanmcdermott/clean-code-javascript#avoid-side-effects-part-1

Solution:

Here's a better way to write it:

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

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

console.log(name); // 'Ryan McDermott';
console.log(newName); // ['Ryan', 'McDermott'];

Why is this better?

Original data remains unchanged

Function is predictable and easier to test

Clear input/output relationship

Can use the original name value elsewhere in the code

Question 31

Are you using function flags appropriately?

Look at this code. What design issue do you see?

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

🔍 Hints

Think about:

What does the boolean parameter tell you about the function's responsibilities?

Is this function doing more than one thing?

How could we make the function's purpose clearer?

Accepted Answer

The Principle:

Boolean flags in function parameters often indicate that a function is doing too much. When a function's behavior changes based on a boolean parameter, it's violating the Single Responsibility Principle.

Solution:

Here's a clearer way to write it:

function createFile(name) {
fs.create(name);
}

function createTempFile(name) {
createFile(`./temp/${name}`);
}

Why is this better?

Each function has a single, clear purpose

Function names clearly describe what they do

No need to remember what the boolean parameter means

Easier to test and maintain

Question 32

Are you setting default objects with Object.assign?

What's problematic about this code?

const menuConfig = {
title: null,
body: 'Bar',
buttonText: null,
cancellable: true,
};

function createMenu(config) {
config.title = config.title || 'Foo';
config.body = config.body || 'Bar';
config.buttonText = config.buttonText || 'Baz';
config.cancellable =
config.cancellable !== undefined ? config.cancellable : true;
}

createMenu(menuConfig);

🔍 Hints

Think about:

What happens to the original config object?

Is this code immutable?

How could we make this cleaner using modern JavaScript?

Accepted Answer

The Principle:

When working with objects that need default values, using Object.assign or the spread operator provides a cleaner and more maintainable way to set default properties compared to manual property assignments.

Solution:

Here's a better approach:

const menuConfig = {
title: 'Order',
// User did not include 'body' key
buttonText: 'Send',
cancellable: true,
};

function createMenu(config) {
let finalConfig = Object.assign(
{
title: 'Foo',
body: 'Bar',
buttonText: 'Baz',
cancellable: true,
},
config,
);
return finalConfig;
// config now equals: {title: "Order", body: "Bar", buttonText: "Send", cancellable: true}
// ...
}

createMenu(menuConfig);

Why is this better?

Original config object remains unchanged

All defaults are clearly defined in one place

More declarative and easier to understand

Returns a new object instead of modifying the input

Question 33

Have you removed duplicate code?

Look at this code. What problems can you spot?

function showDeveloperList(developers) {
developers.forEach((developer) => {
const expectedSalary = developer.calculateExpectedSalary();
const experience = developer.getExperience();
const githubLink = developer.getGithubLink();
const data = {
expectedSalary,
experience,
githubLink,
};

render(data);
});
}

function showManagerList(managers) {
managers.forEach((manager) => {
const expectedSalary = manager.calculateExpectedSalary();
const experience = manager.getExperience();
const portfolio = manager.getMBAProjects();
const data = {
expectedSalary,
experience,
portfolio,
};

render(data);
});
}

🔍 Hints

Think about:

What code is being repeated in both functions?

Could these functions be combined?

How could we handle the differences between developers and managers?

Accepted Answer

The Principle:

Code duplication is one of the biggest enemies of maintainable software. When code is duplicated, changes need to be made in multiple places, increasing the chance of errors.

Reference:

https://github.com/ryanmcdermott/clean-code-javascript#remove-duplicate-code

Solution:

Here's a better way to write it:

function showEmployeeList(employees) {
employees.forEach((employee) => {
const expectedSalary = employee.calculateExpectedSalary();
const experience = employee.getExperience();

const data = {
expectedSalary,
experience,
};

switch (employee.type) {
case 'manager':
data.portfolio = employee.getMBAProjects();
break;
case 'developer':
data.githubLink = employee.getGithubLink();
break;
}

render(data);
});
}

Why is this better?

Common code is now in one place

Employee-specific data is handled through a switch statement

Only one function to maintain and update

Easier to add new employee types in the future

Question 34

Should functions operate at only one level of abstraction?

What makes this code difficult to understand and maintain?

function parseBetterJSAlternative(code) {
const REGEXES = [
// ...
];

const statements = code.split(' ');
const tokens = [];
REGEXES.forEach((REGEX) => {
statements.forEach((statement) => {
// ...
});
});

const ast = [];
tokens.forEach((token) => {
// lex...
});

ast.forEach((node) => {
// parse...
});
}

🔍 Hints

Think about:

How many different operations are happening here?

Are all these operations at the same conceptual level?

How could we organize this to make the process clearer?

Accepted Answer

The Principle:

Each function should work at a single level of abstraction. This means all operations within the function should be at a similar conceptual level, making the code easier to understand and maintain.

Solution:

function parseBetterJSAlternative(code) {
const tokens = tokenize(code);
const syntaxTree = parse(tokens);
syntaxTree.forEach((node) => {
// parse...
});
}

function tokenize(code) {
const REGEXES = [
// ...
];

const statements = code.split(' ');
const tokens = [];
REGEXES.forEach((REGEX) => {
statements.forEach((statement) => {
tokens.push(/ ... /);
});
});

return tokens;
}

function parse(tokens) {
const syntaxTree = [];
tokens.forEach((token) => {
syntaxTree.push(/ ... /);
});

return syntaxTree;
}

Why is this better?

The main function now operates at a single, high level of abstraction

Each operation has been broken into its own function with a clear purpose

The code tells a story: tokenize → parse → return

Each function handles one specific task at one specific level of detail

Question 35

Does your function name clearly describe its purpose?

What's unclear about this function name and usage?

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

const date = new Date();

// It's hard to tell from the function name what is added
addToDate(date, 1);

🔍 Hints

Think about:

What exactly is being added to the date?

Is the parameter order intuitive?

How could you make the function's purpose more obvious?

Accepted Answer

The Principle:

Function names should be verbs that clearly describe the action being performed. Avoid vague or ambiguous names that don't convey the function's purpose.

Solution:

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

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

Why is this better?

The function name now explicitly states what it's adding (a month), and the parameter order follows the natural language order of the function name.

Question 36

Are you avoiding positional markers in your code?

Look at this code. What's wrong with how it's organized?

////////////////////////////////////////////////////////////////////////////////
// Scope Model Instantiation
////////////////////////////////////////////////////////////////////////////////
$scope.model = {
menu: 'foo',
nav: 'bar',
};

////////////////////////////////////////////////////////////////////////////////
// Action setup
////////////////////////////////////////////////////////////////////////////////
const actions = function () {
// ...
};

🔍 Hints

Think about:

What purpose do these dividers serve?

Do they make the code more maintainable?

How else could we make code structure clear?

What happens when you need to add new sections?

Accepted Answer

The Principle:

Positional markers or section dividers add visual noise without providing real value. Well-structured code with meaningful names and proper organization should make the code's structure clear without needing these artificial divisions.

Solution:

Here's how to better structure the code:

// Consider splitting into separate files if these are distinct concerns
$scope.model = {
menu: 'foo',
nav: 'bar',
};

const actions = function () {
// ...
};

Why is this better?

No visual noise from ASCII dividers

Better naming makes the purpose of each section clear

Code organization is achieved through proper structure, not visual markers

Suggests better architectural decisions (like separating concerns into modules)

Easier to maintain and modify without needing to update dividers

Question 37

Are you avoiding journal comments in your code?

Look at this function. What's problematic about its documentation?

/*
 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)
/
function combine(a, b) {
return a + b;
}

🔍 Hints

Think about:

How useful is this history in the code itself?

What happens when the code changes again?

What tools could better track this information?

How does this affect code readability?

Accepted Answer

The Principle:

Journal comments that track changes are redundant when using version control systems. They become outdated quickly and add unnecessary noise to the codebase. Modern version control systems provide better tools for tracking changes and their authors.

Solution:

Here's how the code should look:

function combine(a, b) {
return a + b;
}

Why is this better?

Clean code without historical clutter

History is properly maintained in version control (git)

Changes can be tracked with git log or git blame

Documentation stays focused on current code behavior

Easier to maintain and update

Question 38

Have you removed commented-out code from your codebase?

Look at this code snippet. What problems might it cause in a real project?

doStuff();
// doOtherStuff();
// doSomeMoreStuff();
// doSoMuchStuff();

🔍 Hints

Think about:

Why might these lines be commented out?

How does this affect code readability?

What questions might other developers have when they see this?

What's a better way to handle old/unused code?

Accepted Answer

The Principle:

Commented-out code creates confusion and clutters the codebase. It raises questions about whether the code is still needed or why it was commented out. Modern version control systems like Git are the proper tools for maintaining code history.

Solution:

Here's how the code should look:

doStuff();

Why is this better?

Clean and clear - no ambiguity about what code is actually being used

No mental overhead wondering if the commented code is important

If you need the old code, you can find it in your version control history

Easier to maintain and review code without the clutter

Question 39

Are your comments focused on explaining complex business logic?

Look at this code. What's wrong with these comments?

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

🔍 Hints

Think about:

What value do these comments add?

Are they explaining anything that isn't obvious from the code?

What would be more useful to explain here?

When should we actually use comments?

Accepted Answer

The Principle:

Comments should explain the "why" behind complex business logic, not the "what" or "how" that should be evident from well-written code. Excessive comments often indicate that the code itself could be clearer or better structured.

Solution:

Here's better use of comments:

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;
// Converting to 32-bit integer is necessary to handle large strings
// and maintain consistent hash values across different platforms
hash &= hash;
}
}

Why is this better?

Removed comments that merely repeat what the code says

Kept only the comment that explains the reasoning behind the 32-bit conversion

The code is now cleaner and easier to read

The remaining comment adds actual value by explaining the "why"

Question 40

Are you using default parameters instead of short-circuiting or conditionals?

Look at this code. What could be improved?

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

🔍 Hints

Think about:

What happens if name is an empty string?

What happens if name is 0?

Is there a clearer way to express the default value?

Accepted Answer

The Principle:

Use ES6 default parameters to handle missing values instead of logical operators that might have unexpected behavior with falsy values.

Reference:

https://github.com/ryanmcdermott/clean-code-javascript#use-default-parameters-instead-of-short-circuiting-or-conditionals

Solution:

function createMicrobrewery(name = 'Hipster Brew Co.') {
// ...
}

Why is this better?

Default parameters only trigger when the value is undefined, providing more predictable behavior than the logical OR operator which can override any falsy value.

Question 41

Are you avoiding unneeded context in your names?

What's redundant about this code?

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

function paintCar(car, color) {
car.carColor = color;
}

🔍 Hints

Think about:

What context does the Car object already provide?

Are the 'car' prefixes adding any valuable information?

How could we make this more concise while maintaining clarity?

Accepted Answer

The Principle:

When the context is already clear from the class or object name, don't repeat it in the property names. This creates unnecessary verbosity and redundancy.

Solution:

const Car = {
make: 'Honda',
model: 'Accord',
color: 'Blue',
};

function paintCar(car, color) {
car.color = color;
}

Why is this better?

We've removed the redundant 'car' prefix from car.carColor since the Car object already provides that context. The code is more concise while remaining just as clear.

Question 42

Are you avoiding mental mapping in your code?

What's problematic about this code?

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

🔍 Hints

Think about:

What does 'l' represent?

How much mental effort is needed to keep track of what 'l' means?

What happens if you come back to this code after a few weeks?

Accepted Answer

The Principle:

Don't force readers to translate between different contexts. Using single-letter variables or abbreviated names requires the reader to mentally map these to their actual meaning.

Solution:

const locations = ['Austin', 'New York', 'San Francisco'];
locations.forEach((location) => {
doStuff();
doSomeOtherStuff();
// ...
// ...
// ...
dispatch(location);
});

Why is this better?

Using the full word location instead of l makes the code immediately understandable without requiring any mental translation.

Question 43

Are you using explanatory variables to simplify complex expressions?

Look at this code. What makes it hard to understand?

const address = 'One Infinite Loop, Cupertino 95014';
const cityZipCodeRegex = /^[^,\]+[,\\s]+(.+?)\s(\d{5})?$/;
saveCityZipCode(
address.match(cityZipCodeRegex)[1],
address.match(cityZipCodeRegex)[2],
);

🔍 Hints

Think about*:

How many times is the regex match performed?

What do the array indices [1] and [2] represent?

How could we make this code more readable?

Accepted Answer

The Principle:

Break down complex expressions into smaller, well-named parts to make the code's intent clear and avoid repeating operations.

Solution:

const address = 'One Infinite Loop, Cupertino 95014';
const cityZipCodeRegex = /^[^,\]+[,\\s]+(.+?)\s(\d{5})?$/;
const [_, city, zipCode] = address.match(cityZipCodeRegex) || [];
saveCityZipCode(city, zipCode);

Why is this better?*

We've used destructuring to give meaningful names to the regex matches and eliminated repeated operations. Each part of the code now clearly shows its purpose.

Question 44

Are your variable names easily searchable?

Look at this code. What's the problem here?

// What the heck is 86400000 for?
setTimeout(blastOff, 86400000);

🔍 Hints

Think about:

What does 86400000 represent?

How would you find all places using this number?

How could you make this value's purpose clearer?

Accepted Answer

The Principle:

We will read more code than we will ever write. It's important that the code we do write is readable and searchable. By not naming variables that end up being meaningful for understanding our program, we hurt our readers. Make your names searchable. Tools like buddy.js and ESLint can help identify unnamed constants.

Solution:

// Declare them as capitalized named constants.
const MILLISECONDS_PER_DAY = 60  60  24  1000; //86400000;

setTimeout(blastOff, MILLISECONDS_PER_DAY);

Why is this better?*

The constant name clearly explains what the number represents, making the code self-documenting and easier to search for and update.

Question 45

Are you using the same vocabulary for the same type of variable?

Look at this code. What's wrong with it?

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

🔍 Hints

Think about:

What concept do all these functions deal with?

Is the naming consistent?

How could this create confusion for other developers?

Accepted Answer

The Principle:

When referring to the same type of data or operation, maintain consistency in your naming conventions throughout your codebase. This reduces cognitive load and makes the code more predictable.

Solution:

getUser();

Why is this better?

We've chosen one term (user) and used it consistently. This avoids confusion about whether a client, customer, or user might be handled differently.

Question 46

Are your variable names meaningful and pronounceable?

Look at this code. What's wrong with it?

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

🔍 Hints

Think about:

What does this variable store?

Would other developers understand it easily?

How would you improve it?

Accepted Answer

The Principle:

When naming variables, use clear, descriptive names that explain their purpose. Avoid abbreviations and cryptic shorthand that might confuse other developers.

Solution:

Here's a better way to write it:

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

Why is this better?

The variable name clearly indicates that we're working with a date. Using currentDate immediately tells other developers the purpose of this variable.

Clean Code JavaScript

Guarda tu progreso

Qué incluye

Atajos de teclado

Cualquier modo

Voltear

Opción / Hueco con opciones

Escribir / Hueco escrito

Emparejar