Skip to content

Sérgio Kopplin

Clean Code - Other Concepts

Notebook, Clean Code1 min read

Concurrency

Use Promises, not callbacks

Bad:

1import { get } from "request";
2import { writeFile } from "fs";
3
4get(
5 "https://en.wikipedia.org/wiki/Robert_Cecil_Martin",
6 (requestErr, response, body) => {
7 if (requestErr) {
8 console.error(requestErr);
9 } else {
10 writeFile("article.html", body, writeErr => {
11 if (writeErr) {
12 console.error(writeErr);
13 } else {
14 console.log("File written");
15 }
16 });
17 }
18 }
19);

Good:

1import { get } from "request-promise";
2import { writeFile } from "fs-extra";
3
4get("https://en.wikipedia.org/wiki/Robert_Cecil_Martin")
5 .then(body => {
6 return writeFile("article.html", body);
7 })
8 .then(() => {
9 console.log("File written");
10 })
11 .catch(err => {
12 console.error(err);
13 });

Async/Await are even cleaner than Promises

Bad:

1import { get } from "request-promise";
2import { writeFile } from "fs-extra";
3
4get("https://en.wikipedia.org/wiki/Robert_Cecil_Martin")
5 .then(body => {
6 return writeFile("article.html", body);
7 })
8 .then(() => {
9 console.log("File written");
10 })
11 .catch(err => {
12 console.error(err);
13 });

Good:

1import { get } from "request-promise";
2import { writeFile } from "fs-extra";
3
4async function getCleanCodeArticle() {
5 try {
6 const body = await get(
7 "https://en.wikipedia.org/wiki/Robert_Cecil_Martin"
8 );
9 await writeFile("article.html", body);
10 console.log("File written");
11 } catch (err) {
12 console.error(err);
13 }
14}
15
16getCleanCodeArticle()

Error Handling

Don't ignore caught errors

Bad:

1try {
2 functionThatMightThrow();
3} catch (error) {
4 console.log(error);
5}

Good:

1try {
2 functionThatMightThrow();
3} catch (error) {
4 // One option (more noisy than console.log):
5 console.error(error);
6 // Another option:
7 notifyUserOfError(error);
8 // Another option:
9 reportErrorToService(error);
10 // OR do all three!
11}

Don't ignore rejected promises

Bad:

1getdata()
2 .then(data => {
3 functionThatMightThrow(data);
4 })
5 .catch(error => {
6 console.log(error);
7 });

Good:

1getdata()
2 .then(data => {
3 functionThatMightThrow(data);
4 })
5 .catch(error => {
6 // One option (more noisy than console.log):
7 console.error(error);
8 // Another option:
9 notifyUserOfError(error);
10 // Another option:
11 reportErrorToService(error);
12 // OR do all three!
13 });

Comments

Do: Business logic complexity

Bad:

1function hashIt(data) {
2 // The hash
3 let hash = 0;
4
5 // Length of string
6 const length = data.length;
7
8 // Loop through every character in data
9 for (let i = 0; i < length; i++) {
10 // Get character code.
11 const char = data.charCodeAt(i);
12 // Make the hash
13 hash = (hash << 5) - hash + char;
14 // Convert to 32-bit integer
15 hash &= hash;
16 }
17}

Good:

1function hashIt(data) {
2 let hash = 0;
3 const length = data.length;
4
5 for (let i = 0; i < length; i++) {
6 const char = data.charCodeAt(i);
7 hash = (hash << 5) - hash + char;
8
9 // Convert to 32-bit integer
10 hash &= hash;
11 }
12}

Don't: Old comments on old code

Bad:

1doStuff();
2// doOtherStuff();
3// doSomeMoreStuff();
4// doSoMuchStuff();

Good:

1doStuff();

Don't: Journal comments

Bad:

1/**
2 * 2016-12-20: Removed monads, didn't understand them (RM)
3 * 2016-10-01: Improved using special monads (JP)
4 * 2016-02-03: Removed type-checking (LI)
5 * 2015-03-14: Added combine with type-checking (JR)
6 */
7function combine(a, b) {
8 return a + b;
9}

Good:

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

Don't: Positional markers

I've used that a lot. haha

Bad:

1///////////////////////////////////////
2// Scope Model Instantiation
3//////////////////////////////////////
4$scope.model = {
5 menu: "foo",
6 nav: "bar"
7};
8
9//////////////////////////////////////
10// Action setup
11//////////////////////////////////////
12const actions = function() {
13 // ...
14};

Good:

1$scope.model = {
2 menu: "foo",
3 nav: "bar"
4};
5
6const actions = function() {
7 // ...
8};

Formatting

Disclaimer

DO NOT ARGUE over formatting. There are tons of tools to automate this. Use one!


Consistent capitalization

Bad:

1const DAYS_IN_WEEK = 7;
2const daysInMonth = 30;
3
4const songs = ["Back In Black", "Stairway to Heaven", "Hey Jude"];
5const Artists = ["ACDC", "Led Zeppelin", "The Beatles"];
6
7function eraseDatabase() {}
8function restore_database() {}
9
10class animal {}
11class Alpaca {}

Good:

1const DAYS_IN_WEEK = 7;
2const DAYS_IN_MONTH = 30;
3
4const SONGS = ["Back In Black", "Stairway to Heaven", "Hey Jude"];
5const ARTISTS = ["ACDC", "Led Zeppelin", "The Beatles"];
6
7function eraseDatabase() {}
8function restoreDatabase() {}
9
10class Animal {}
11class Alpaca {}

Function callers and callees should be close

Bad:

1class PerformanceReview {
2 constructor(employee) {
3 this.employee = employee;
4 }
5
6 lookupPeers() {
7 return db.lookup(this.employee, "peers");
8 }
9
10 lookupManager() {
11 return db.lookup(this.employee, "manager");
12 }
13
14 getPeerReviews() {
15 const peers = this.lookupPeers();
16 // ...
17 }
18
19 perfReview() {
20 this.getPeerReviews();
21 this.getManagerReview();
22 this.getSelfReview();
23 }
24
25 getManagerReview() {
26 const manager = this.lookupManager();
27 }
28
29 getSelfReview() {
30 // ...
31 }
32}
33
34const review = new PerformanceReview(employee);
35review.perfReview();

Good:

1class PerformanceReview {
2 constructor(employee) {
3 this.employee = employee;
4 }
5
6 perfReview() {
7 this.getPeerReviews();
8 this.getManagerReview();
9 this.getSelfReview();
10 }
11
12 getPeerReviews() {
13 const peers = this.lookupPeers();
14 // ...
15 }
16
17 lookupPeers() {
18 return db.lookup(this.employee, "peers");
19 }
20
21 getManagerReview() {
22 const manager = this.lookupManager();
23 }
24
25 lookupManager() {
26 return db.lookup(this.employee, "manager");
27 }
28
29 getSelfReview() {
30 // ...
31 }
32}
33
34const review = new PerformanceReview(employee);
35review.perfReview();

Testing

Single concept per test

Bad:

1import assert from "assert";
2
3describe("MomentJS", () => {
4 it("handles date boundaries", () => {
5 let date;
6
7 date = new MomentJS("1/1/2015");
8 date.addDays(30);
9 assert.equal("1/31/2015", date);
10
11 date = new MomentJS("2/1/2016");
12 date.addDays(28);
13 assert.equal("02/29/2016", date);
14
15 date = new MomentJS("2/1/2015");
16 date.addDays(28);
17 assert.equal("03/01/2015", date);
18 });
19});

Good:

1import assert from "assert";
2
3describe("MomentJS", () => {
4 it("handles 30-day months", () => {
5 const date = new MomentJS("1/1/2015");
6 date.addDays(30);
7 assert.equal("1/31/2015", date);
8 });
9
10 it("handles leap year", () => {
11 const date = new MomentJS("2/1/2016");
12 date.addDays(28);
13 assert.equal("02/29/2016", date);
14 });
15
16 it("handles non-leap year", () => {
17 const date = new MomentJS("2/1/2015");
18 date.addDays(28);
19 assert.equal("03/01/2015", date);
20 });
21});
© 2020 by Sérgio Kopplin.