From e556af0e90e5fe9d61a8b9cb6ef2e6d0c4941e65 Mon Sep 17 00:00:00 2001 From: Chris Jaynes Date: Fri, 14 Jul 2023 15:21:07 -0500 Subject: [PATCH] Include our team preferences in the guide --- README.md | 2195 +++++++++++++++++++++----------------------- prettier.config.js | 20 + 2 files changed, 1064 insertions(+), 1151 deletions(-) create mode 100644 prettier.config.js diff --git a/README.md b/README.md index 7bff168..a2617bf 100644 --- a/README.md +++ b/README.md @@ -1,37 +1,33 @@ # clean-code-typescript [![Tweet](https://img.shields.io/twitter/url/http/shields.io.svg?style=social)](https://twitter.com/intent/tweet?text=Clean%20Code%20Typescript&url=https://github.com/labs42io/clean-code-typescript) -Clean Code concepts adapted for TypeScript. +Clean Code concepts adapted for TypeScript. Inspired from [clean-code-javascript](https://github.com/ryanmcdermott/clean-code-javascript). ## Table of Contents - 1. [Introduction](#introduction) - 2. [Variables](#variables) - 3. [Functions](#functions) - 4. [Objects and Data Structures](#objects-and-data-structures) - 5. [Classes](#classes) - 6. [SOLID](#solid) - 7. [Testing](#testing) - 8. [Concurrency](#concurrency) - 9. [Error Handling](#error-handling) - 10. [Formatting](#formatting) - 11. [Comments](#comments) - 12. [Translations](#translations) +1. [Introduction](#introduction) +2. [Variables](#variables) +3. [Functions](#functions) +4. [Objects and Data Structures](#objects-and-data-structures) +5. [Classes](#classes) +6. [SOLID](#solid) +7. [Testing](#testing) +8. [Concurrency](#concurrency) +9. [Error Handling](#error-handling) +10. [Formatting](#formatting) +11. [Comments](#comments) ## Introduction -![Humorous image of software quality estimation as a count of how many expletives -you shout when reading code](https://www.osnews.com/images/comics/wtfm.jpg) - Software engineering principles, from Robert C. Martin's book -[*Clean Code*](https://www.amazon.com/Clean-Code-Handbook-Software-Craftsmanship/dp/0132350882), +[_Clean Code_](https://www.amazon.com/Clean-Code-Handbook-Software-Craftsmanship/dp/0132350882), adapted for TypeScript. This is not a style guide. It's a guide to producing [readable, reusable, and refactorable](https://github.com/ryanmcdermott/3rs-of-software-architecture) software in TypeScript. Not every principle herein has to be strictly followed, and even fewer will be universally agreed upon. These are guidelines and nothing more, but they are ones codified over many years of collective experience by the authors of -*Clean Code*. +_Clean Code_. Our craft of software engineering is just a bit over 50 years old, and we are still learning a lot. When software architecture is as old as architecture @@ -58,16 +54,15 @@ Distinguish names in such a way that the reader knows what the differences offer ```ts function between(a1: T, a2: T, a3: T): boolean { - return a2 <= a1 && a1 <= a3; + return a2 <= a1 && a1 <= a3 } - ``` **Good:** ```ts function between(value: T, left: T, right: T): boolean { - return left <= value && value <= right; + return left <= value && value <= right } ``` @@ -81,9 +76,9 @@ If you can’t pronounce it, you can’t discuss it without sounding like an idi ```ts type DtaRcrd102 = { - genymdhms: Date; - modymdhms: Date; - pszqint: number; + genymdhms: Date + modymdhms: Date + pszqint: number } ``` @@ -91,9 +86,9 @@ type DtaRcrd102 = { ```ts type Customer = { - generationTimestamp: Date; - modificationTimestamp: Date; - recordId: number; + generationTimestamp: Date + modificationTimestamp: Date + recordId: number } ``` @@ -104,37 +99,37 @@ type Customer = { **Bad:** ```ts -function getUserInfo(): User; -function getUserDetails(): User; -function getUserData(): User; +function getUserInfo(): User +function getUserDetails(): User +function getUserData(): User ``` **Good:** ```ts -function getUser(): User; +function getUser(): User ``` **[⬆ back to top](#table-of-contents)** ### Use searchable names -We will read more code than we will ever write. It's important that the code we do write must be 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 [ESLint](https://typescript-eslint.io/) can help identify unnamed constants (also known as magic strings and magic numbers). +We will read more code than we will ever write. It's important that the code we do write must be 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 [ESLint](https://typescript-eslint.io/) can help identify unnamed constants (also known as magic strings and magic numbers). **Bad:** ```ts // What the heck is 86400000 for? -setTimeout(restart, 86400000); +setTimeout(restart, 86400000) ``` **Good:** ```ts -// Declare them as capitalized named constants. -const MILLISECONDS_PER_DAY = 24 * 60 * 60 * 1000; // 86400000 +// Declare them as named constants. +const MillisecondsPerDay = 24 * 60 * 60 * 1000 // 86400000 -setTimeout(restart, MILLISECONDS_PER_DAY); +setTimeout(restart, MillisecondsPerDay) ``` **[⬆ back to top](#table-of-contents)** @@ -144,7 +139,7 @@ setTimeout(restart, MILLISECONDS_PER_DAY); **Bad:** ```ts -declare const users: Map; +declare const users: Map for (const keyValue of users) { // iterate through users map @@ -154,34 +149,48 @@ for (const keyValue of users) { **Good:** ```ts -declare const users: Map; +declare const users: Map for (const [id, user] of users) { // iterate through users map } ``` -**[⬆ back to top](#table-of-contents)** - -### Avoid Mental Mapping - -Explicit is better than implicit. -*Clarity is king.* - **Bad:** ```ts -const u = getUser(); -const s = getSubscription(); -const t = charge(u, s); +const result = await fetchUser() +const data = await fetchComments(result.id) ``` **Good:** ```ts -const user = getUser(); -const subscription = getSubscription(); -const transaction = charge(user, subscription); +const user = await fetchUser() +const comments = await fetchComments(user.id) +``` + +**[⬆ back to top](#table-of-contents)** + +### Avoid Mental Mapping + +Explicit is better than implicit. +_Clarity is king._ + +**Bad:** + +```ts +const u = getUser() +const s = getSubscription() +const t = charge(u, s) +``` + +**Good:** + +```ts +const user = getUser() +const subscription = getSubscription() +const transaction = charge(user, subscription) ``` **[⬆ back to top](#table-of-contents)** @@ -194,13 +203,13 @@ If your class/type/object name tells you something, don't repeat that in your va ```ts type Car = { - carMake: string; - carModel: string; - carColor: string; + carMake: string + carModel: string + carColor: string } function print(car: Car): void { - console.log(`${car.carMake} ${car.carModel} (${car.carColor})`); + console.log(`${car.carMake} ${car.carModel} (${car.carColor})`) } ``` @@ -208,13 +217,13 @@ function print(car: Car): void { ```ts type Car = { - make: string; - model: string; - color: string; + make: string + model: string + color: string } function print(car: Car): void { - console.log(`${car.make} ${car.model} (${car.color})`); + console.log(`${car.make} ${car.model} (${car.color})`) } ``` @@ -228,7 +237,7 @@ Default arguments are often cleaner than short circuiting. ```ts function loadPages(count?: number) { - const loadCount = count !== undefined ? count : 10; + const loadCount = count !== undefined ? count : 10 // ... } ``` @@ -243,29 +252,29 @@ function loadPages(count: number = 10) { **[⬆ back to top](#table-of-contents)** -### Use enum to document the intent +### Prefer string union types over enum types for constrained strings -Enums can help you document the intent of the code. For example when we are concerned about values being -different rather than the exact value of those. +To constrain a string to a particular set of values, use a string union type. + +String unions require less code, fewer imports, and are usually simpler to work with, overall. **Bad:** ```ts const GENRE = { - ROMANTIC: 'romantic', + ROMANCE: 'romance', DRAMA: 'drama', COMEDY: 'comedy', - DOCUMENTARY: 'documentary', + DOCUMENTARY: 'documentary' } -projector.configureFilm(GENRE.COMEDY); +projector.configureFilm(GENRE.COMEDY) class Projector { // declaration of Projector configureFilm(genre) { - switch (genre) { - case GENRE.ROMANTIC: - // some logic to be executed + if (genre === GENRE.ROMANCE) { + // some logic to be executed } } } @@ -274,21 +283,15 @@ class Projector { **Good:** ```ts -enum GENRE { - ROMANTIC, - DRAMA, - COMEDY, - DOCUMENTARY, -} +type Genre = 'romance' | 'drama' | 'comedy' | 'documentary' -projector.configureFilm(GENRE.COMEDY); +projector.configureFilm(GENRE.COMEDY) class Projector { // declaration of Projector configureFilm(genre) { - switch (genre) { - case GENRE.ROMANTIC: - // some logic to be executed + if (genre === 'romance') { + // some logic to be executed } } } @@ -301,13 +304,13 @@ class Projector { ### Function arguments (2 or fewer ideally) Limiting the number of function parameters is incredibly important because it makes testing your function easier. -Having more than three leads to a combinatorial explosion where you have to test tons of different cases with each separate argument. +Having more than three leads to a combinatorial explosion where you have to test tons of different cases with each separate argument. One or two arguments is the ideal case, and three should be avoided if possible. Anything more than that should be consolidated. Usually, if you have more than two arguments then your function is trying to do too much. -In cases where it's not, most of the time a higher-level object will suffice as an argument. +In cases where it's not, most of the time a higher-level object will suffice as an argument. -Consider using object literals if you are finding yourself needing a lot of arguments. +Consider using object literals if you are finding yourself needing a lot of arguments. To make it obvious what properties the function expects, you can use the [destructuring](https://basarat.gitbook.io/typescript/future-javascript/destructuring) syntax. This has a few advantages: @@ -323,17 +326,27 @@ This has a few advantages: **Bad:** ```ts -function createMenu(title: string, body: string, buttonText: string, cancellable: boolean) { +function createMenu( + title: string, + body: string, + buttonText: string, + cancellable: boolean +) { // ... } -createMenu('Foo', 'Bar', 'Baz', true); +createMenu('Foo', 'Bar', 'Baz', true) ``` **Good:** ```ts -function createMenu(options: { title: string, body: string, buttonText: string, cancellable: boolean }) { +function createMenu(options: { + title: string + body: string + buttonText: string + cancellable: boolean +}) { // ... } @@ -342,14 +355,18 @@ createMenu({ body: 'Bar', buttonText: 'Baz', cancellable: true -}); +}) ``` You can further improve readability by using [type aliases](https://www.typescriptlang.org/docs/handbook/advanced-types.html#type-aliases): ```ts - -type MenuOptions = { title: string, body: string, buttonText: string, cancellable: boolean }; +type MenuOptions = { + title: string + body: string + buttonText: string + cancellable: boolean +} function createMenu(options: MenuOptions) { // ... @@ -360,7 +377,7 @@ createMenu({ body: 'Bar', buttonText: 'Baz', cancellable: true -}); +}) ``` **[⬆ back to top](#table-of-contents)** @@ -374,11 +391,11 @@ This is by far the most important rule in software engineering. When functions d ```ts function emailActiveClients(clients: Client[]) { clients.forEach((client) => { - const clientRecord = database.lookup(client); + const clientRecord = database.lookup(client) if (clientRecord.isActive()) { - email(client); + email(client) } - }); + }) } ``` @@ -386,12 +403,12 @@ function emailActiveClients(clients: Client[]) { ```ts function emailActiveClients(clients: Client[]) { - clients.filter(isActiveClient).forEach(email); + clients.filter(isActiveClient).forEach(email) } function isActiveClient(client: Client) { - const clientRecord = database.lookup(client); - return clientRecord.isActive(); + const clientRecord = database.lookup(client) + return clientRecord.isActive() } ``` @@ -406,10 +423,10 @@ function addToDate(date: Date, month: number): Date { // ... } -const date = new Date(); +const date = new Date() // It's hard to tell from the function name what is added -addToDate(date, 1); +addToDate(date, 1) ``` **Good:** @@ -419,75 +436,113 @@ function addMonthToDate(date: Date, month: number): Date { // ... } -const date = new Date(); -addMonthToDate(date, 1); +const date = new Date() +addMonthToDate(date, 1) ``` -**[⬆ back to top](#table-of-contents)** - -### Functions should only be one level of abstraction - -When you have more than one level of abstraction your function is usually doing too much. Splitting up functions leads to reusability and easier testing. - **Bad:** ```ts -function parseCode(code: string) { - const REGEXES = [ /* ... */ ]; - const statements = code.split(' '); - const tokens = []; +// This _might_ be okay if we're consistent, but what if we want to import +// two or more `fetch` functions in some other module? +export const fetch(userId: string) { + // ... +} - REGEXES.forEach((regex) => { - statements.forEach((statement) => { - // ... - }); - }); - - const ast = []; - tokens.forEach((token) => { - // lex... - }); - - ast.forEach((node) => { - // parse... - }); +// What do we call the next function, `fetchWithArguments`? +const fetchWithParams(email?: string, lastName?: string) { + //... } ``` **Good:** ```ts -const REGEXES = [ /* ... */ ]; - -function parseCode(code: string) { - const tokens = tokenize(code); - const syntaxTree = parse(tokens); - - syntaxTree.forEach((node) => { - // parse... - }); +// +export const userGetById(userId: string) { + // ... } -function tokenize(code: string): Token[] { - const statements = code.split(' '); - const tokens: Token[] = []; +// Combine the domain object name `user` with a standard convention `Search` +// Now we can import this just about anywhere without naming collisions +interface UserSearchFilter { + email?: string + lastName? : string +} +const userSearch({ email, lastName }: UserSearchFilter) { + //... +} +``` + +**[⬆ back to top](#table-of-contents)** + +### Functions should only work on a single level of abstraction + +When we have more than one level of abstraction our function is probably doing too much. Splitting up functions leads to reusability, more composable code, and easier testing. + +**Bad:** + +```ts +function parseCode(code: string) { + const REGEXES = [ + /* ... */ + ] + const statements = code.split(' ') + const tokens = [] REGEXES.forEach((regex) => { statements.forEach((statement) => { - tokens.push( /* ... */ ); - }); - }); + // ... + }) + }) - return tokens; + const ast = [] + tokens.forEach((token) => { + // lex... + }) + + ast.forEach((node) => { + // parse... + }) +} +``` + +**Good:** + +```ts +const REGEXES = [ + /* ... */ +] + +function parseCode(code: string) { + const tokens = tokenize(code) + const syntaxTree = parse(tokens) + + syntaxTree.forEach((node) => { + // parse... + }) +} + +function tokenize(code: string): Token[] { + const statements = code.split(' ') + const tokens: Token[] = [] + + REGEXES.forEach((regex) => { + statements.forEach((statement) => { + tokens.push(/* ... */) + }) + }) + + return tokens } function parse(tokens: Token[]): SyntaxTree { - const syntaxTree: SyntaxTree[] = []; + const syntaxTree: SyntaxTree[] = [] tokens.forEach((token) => { - syntaxTree.push( /* ... */ ); - }); + syntaxTree.push(/* ... */) + }) - return syntaxTree; + return syntaxTree } ``` @@ -496,13 +551,13 @@ function parse(tokens: Token[]): SyntaxTree { ### Remove duplicate code Do your absolute best to avoid duplicate code. -Duplicate code is bad because it means that there's more than one place to alter something if you need to change some logic. +Duplicate code is bad because it means that there's more than one place to alter something if you need to change some logic. Imagine if you run a restaurant and you keep track of your inventory: all your tomatoes, onions, garlic, spices, etc. If you have multiple lists that you keep this on, then all have to be updated when you serve a dish with tomatoes in them. -If you only have one list, there's only one place to update! +If you only have one list, there's only one place to update! -Oftentimes you have duplicate code because you have two or more slightly different things, that share a lot in common, but their differences force you to have two or more separate functions that do much of the same things. Removing duplicate code means creating an abstraction that can handle this set of different things with just one function/module/class. +Oftentimes you have duplicate code because you have two or more slightly different things, that share a lot in common, but their differences force you to have two or more separate functions that do much of the same things. Removing duplicate code means creating an abstraction that can handle this set of different things with just one function/module/class. Getting the abstraction right is critical, that's why you should follow the [SOLID](#solid) principles. Bad abstractions can be worse than duplicate code, so be careful! Having said this, if you can make a good abstraction, do it! Don't repeat yourself, otherwise, you'll find yourself updating multiple places anytime you want to change one thing. @@ -511,34 +566,34 @@ Getting the abstraction right is critical, that's why you should follow the [SOL ```ts function showDeveloperList(developers: Developer[]) { developers.forEach((developer) => { - const expectedSalary = developer.calculateExpectedSalary(); - const experience = developer.getExperience(); - const githubLink = developer.getGithubLink(); + const expectedSalary = developer.calculateExpectedSalary() + const experience = developer.getExperience() + const githubLink = developer.getGithubLink() const data = { expectedSalary, experience, githubLink - }; + } - render(data); - }); + render(data) + }) } function showManagerList(managers: Manager[]) { managers.forEach((manager) => { - const expectedSalary = manager.calculateExpectedSalary(); - const experience = manager.getExperience(); - const portfolio = manager.getMBAProjects(); + const expectedSalary = manager.calculateExpectedSalary() + const experience = manager.getExperience() + const portfolio = manager.getMBAProjects() const data = { expectedSalary, experience, portfolio - }; + } - render(data); - }); + render(data) + }) } ``` @@ -549,7 +604,7 @@ class Developer { // ... getExtraDetails() { return { - githubLink: this.githubLink, + githubLink: this.githubLink } } } @@ -558,29 +613,30 @@ class Manager { // ... getExtraDetails() { return { - portfolio: this.portfolio, + portfolio: this.portfolio } } } function showEmployeeList(employee: (Developer | Manager)[]) { employee.forEach((employee) => { - const expectedSalary = employee.calculateExpectedSalary(); - const experience = employee.getExperience(); - const extra = employee.getExtraDetails(); + const expectedSalary = employee.calculateExpectedSalary() + const experience = employee.getExperience() + const extra = employee.getExtraDetails() const data = { expectedSalary, experience, - extra, - }; + extra + } - render(data); - }); + render(data) + }) } ``` You may also consider adding a union type, or common parent class if it suits your abstraction. + ```ts class Developer { // ... @@ -594,12 +650,10 @@ type Employee = Developer | Manager function showEmployeeList(employee: Employee[]) { // ... - }); } - ``` -You should be critical about code duplication. Sometimes there is a tradeoff between duplicated code and increased complexity by introducing unnecessary abstraction. When two implementations from two different modules look similar but live in different domains, duplication might be acceptable and preferred over extracting the common code. The extracted common code, in this case, introduces an indirect dependency between the two modules. +We should be thoughtful about code duplication. Sometimes there is a tradeoff between duplicated code and increased complexity by introducing _unnecessary_ abstractions. When two implementations from two different modules look similar but live in different domains, duplication might be acceptable and preferred over extracting the common code. See the section below about "Manufactured Complexity". **[⬆ back to top](#table-of-contents)** @@ -608,37 +662,51 @@ You should be critical about code duplication. Sometimes there is a tradeoff bet **Bad:** ```ts -type MenuConfig = { title?: string, body?: string, buttonText?: string, cancellable?: boolean }; +type MenuConfig = { + title?: string + body?: string + buttonText?: string + cancellable?: boolean +} function createMenu(config: MenuConfig) { - config.title = config.title || 'Foo'; - config.body = config.body || 'Bar'; - config.buttonText = config.buttonText || 'Baz'; - config.cancellable = config.cancellable !== undefined ? config.cancellable : true; + config.title = config.title || 'Foo' + config.body = config.body || 'Bar' + config.buttonText = config.buttonText || 'Baz' + config.cancellable = + config.cancellable !== undefined ? config.cancellable : true // ... } -createMenu({ body: 'Bar' }); +createMenu({ body: 'Bar' }) ``` **Good:** ```ts -type MenuConfig = { title?: string, body?: string, buttonText?: string, cancellable?: boolean }; +type MenuConfig = { + title?: string + body?: string + buttonText?: string + cancellable?: boolean +} function createMenu(config: MenuConfig) { - const menuConfig = Object.assign({ - title: 'Foo', - body: 'Bar', - buttonText: 'Baz', - cancellable: true - }, config); + const menuConfig = Object.assign( + { + title: 'Foo', + body: 'Bar', + buttonText: 'Baz', + cancellable: true + }, + config + ) // ... } -createMenu({ body: 'Bar' }); +createMenu({ body: 'Bar' }) ``` Or, you could use the spread operator: @@ -650,25 +718,36 @@ function createMenu(config: MenuConfig) { body: 'Bar', buttonText: 'Baz', cancellable: true, - ...config, - }; + ...config + } // ... } ``` + The spread operator and `Object.assign()` are very similar. The main difference is that spreading defines new properties, while `Object.assign()` sets them. More detailed, the difference is explained in [this](https://stackoverflow.com/questions/32925460/object-spread-vs-object-assign) thread. Alternatively, you can use destructuring with default values: ```ts -type MenuConfig = { title?: string, body?: string, buttonText?: string, cancellable?: boolean }; +type MenuConfig = { + title?: string + body?: string + buttonText?: string + cancellable?: boolean +} -function createMenu({ title = 'Foo', body = 'Bar', buttonText = 'Baz', cancellable = true }: MenuConfig) { +function createMenu({ + title = 'Foo', + body = 'Bar', + buttonText = 'Baz', + cancellable = true +}: MenuConfig) { // ... } -createMenu({ body: 'Bar' }); +createMenu({ body: 'Bar' }) ``` To avoid any side effects and unexpected behavior by passing in explicitly the `undefined` or `null` value, you can tell the TypeScript compiler to not allow it. @@ -686,9 +765,9 @@ Functions should do one thing. Split out your functions if they are following di ```ts function createFile(name: string, temp: boolean) { if (temp) { - fs.create(`./temp/${name}`); + fs.create(`./temp/${name}`) } else { - fs.create(name); + fs.create(name) } } ``` @@ -697,24 +776,270 @@ function createFile(name: string, temp: boolean) { ```ts function createTempFile(name: string) { - createFile(`./temp/${name}`); + createFile(`./temp/${name}`) } function createFile(name: string) { - fs.create(name); + fs.create(name) } ``` +### Strive for "low density" code + +As with modules, classes, and functions, most lines of code should try to "do one thing". + +When we combine lots of logical operations into a single line of code, it becomes less readable, and more difficult to maintain. + +**Bad:** + +```ts +// Quick! What does this code do? +let userIds = [] +if (!(await fetchUserIdsBySignupDate(signUpDate)).length) + userIds = await db.query( + 'SELECT userId FROM signUpAudits WHERE createdDate AFTER ?', + [signUpDate instanceof Date ? signUpDate | new Date(signUpDate)] + ).then((rows) => { rows.map((row)=> row.userId ) }) +``` + +**Good:** + +```ts +// This may be more lines of code, but it is MUCH easier to read and maintain! +const ensureUserIdsBySignupDateOrAudit = (signUpDate: Date | string) => { + const userUserIds = await getUserIdsBySignupDate(signUpDate) + + if (userUserIds.length) { + return userUserIds + } + + const auditUserIds = await signUpAuditsGetUserIdsByCreatedAt(signUpDate) + return auditUserIds +} + +const signUpAuditsGetUserIdsByCreatedAt = (signUpDate: Date | string) => { + const createdAt = ensureDate(signUpDate) + const query = 'SELECT userId FROM signUpAudits WHERE createdDate AFTER ?' + const params = [createdAt] + const rows = db.query<{ userId }>(query, params) + return rows.map((row) => row.userId) +} + +// Quick! What does this code do? :D +const userIds = ensureUserIdsBySignupDateOrAudit(signUpDate) +``` + +### Avoid the ternary operator + +The ternary operator may seem convenient sometimes, but it is often more difficult to reason about than many developers realize. Use of the ternary operator, especially when nested, leads to code that is harder to read, harder to maintain, and prone to defects. + +When the ternary operator is used, try to use it only to set a variable value, or as a return from a function. Avoid nesting ternaries inside other code, and never combine multiple ternary operators in a single statement. + +**Bad:** + +```ts +export const ISODateToClientDate = (time: Date | string | undefined) => { + if (time instanceof Date) { + return utcToZonedTime(time, clientTimezone) + } + return utcToZonedTime(time ? new Date(time) : new Date(), clientTimezone) +} +``` + +**Good:** + +```ts +// This approach clearly delineates the different cases for values of `time` +export const ISODateToClientDate = (time: Date | string | undefined) => { + if (time instanceof Date) { + return utcToZonedTime(time, clientTimezone) + } + // This may be a few more lines of code, but that is the point! + // This code is less dense, making it easier to read and maintain. + if (time) { + return new utcToZonedTime(new Date(time), clientTimezone) + } + return utcToZonedTime(new Date(), clientTimezone) +} +``` + +Sometimes nested ternaries may _seem_ more readable, at least at first glance. + +This code looks _sort of_ readable, until we look at the other options. + +**Bad:** + +```ts +const query = ` + SELECT id + FROM carts + WHERE status = 'active' + isWebOrder = ? + ${filterByFailed === 'true' ? 'AND failureReason IS NOT NULL' : ''} + ${stateId ? 'AND stateId = ?' : ''} + ORDER BY createdAt DESC +` + +// Did you notice the bug in the code above? The nested ternaries provide an excellent distraction! +// Also note how the `?` and `:` tokens blend in visually with surrounding code. +``` + +**Better:** + +```ts +const maybeFailureReason = + filterByFailed === 'true' ? 'AND failureReason IS NOT NULL' : '' + +const maybeStateId = stateId ? 'AND stateId = ?' : '' + +const select = ` + SELECT id + FROM carts + WHERE status = 'active' + isWebOrder = ? + ${maybeFailureReason} + ${maybeStateId} + ORDER BY createdAt DESC +` +``` + +**Best:** + +```ts +import { buildWhereClause, isNotNull } from 'some-query-builder' + +const conditions = { + status: 'active', + isWebOrder, + stateId + failureReason: () { + if (filterByFailed === 'true') { + return isNotNull + } + } +} + +// Common logic like building queries should be behind an abstraction! +const whereClause = buildWhereClause(conditions) + +const select = sql` + SELECT id + FROM carts + ${whereClause} + ORDER BY createdAt DESC +` +``` + +### Prefer named exports over default exports + +Named exports are more explicit and tend to work better with our tools. + +**Bad:** + +```ts +// Defaults encourage too much creativity when importing +import user from './userModel' +import sessionModel from './sessionModel' + +const authenticate = (userId: string) => { + // Defaults tend to result in naming collisions + const theUser = user.getById(userId) + const session = sessionModel.getByUserId(userId) + // ... +} + +// +// ...a whole bunch of other functions +// + +export default { + // When we click "goToDefinition" in VSCode, it brings us here, and we have to click through again + authenticate +} +``` + +**Good:** + +```ts +import { userGetById } from './userModel' +import { sessionGetByUserId } from './sessionModel' + +export const authenticate = (userId: string) => { + const user = userGetById(userId) + const session = sessionGetByUserId(userId) + // ... +} + +// No need for extra cruft! +``` + +### Strive for simplicity, avoid "manufactured complexity" + +Some patterns start out with the best intentions, but end up being a burden, and end up making code more difficult to read and maintain. Working on our application code should feel sleek and ergonomic, not clumsy or burdensome. If we feel like one of our patterns is slowing us down, it probably is! We should raise the issue with the team, and figure out a way to make our code simpler, and easier to work with. + +A few common examples of manufactured complexity: + +- Widespread use of a low-level API, instead of creating a higher-level wrapper API +- Using multiple TS types to represent the same domain object +- Overuse of the type system: too many interfaces, overly-complex generic types, etc +- Misuse or overuse of well-known patterns: e.g. the Factory Pattern +- Strict adherence to one paradigm: OOP, functional programming, immutability + +**Bad:** + +```ts +import IUserAuth from './IUserAuth' +import IUser from './IUser' +import IUserFetcher from './IUserFetcher' +import UserFetcherFactory from './UserFetcherFactory' +import ISession from './ISession' +import ISessionFetcher from './ISessionFetcher' +import SessionFetcherFactory from './SessionFetcherFactory' + +export default class UserAuth implements IUserAuth { + userFetcher: IUserFetcher + sessionFetcher: ISessionFetcher + + constructor( + userFetcherFactory: UserFetcherFactory, + sessionFetcherFactory: SessionFetcherFactory + ) { + this.userFetcher = userFetcherFactory.createUserFetcher() + this.sessionFetcher = sessionFactorFetcher.createSessionFetcher() + } + + async authenticateUser(userId: string) { + const user = await this.userFetcher.fetchUser(userId) + const session = await this.sessionFetcher.fetchSession(userId) + // ... finish authenticating user + } +} +``` + +**Good:** + +```ts +import { sessionGetByUserId } from './sessionModel' +import { userGetById } from './userModel' + +export const authenticateUser(userId: string) { + const user = userGetById(userId) + const session = sessionGetByUserId(userId) + // ... finish authenticating user +} + +``` + **[⬆ back to top](#table-of-contents)** ### Avoid Side Effects (part 1) A function produces a side effect if it does anything other than take a value in and return another value or values. -A side effect could be writing to a file, modifying some global variable, or accidentally wiring all your money to a stranger. +A side effect could be writing to a file, modifying some global variable, or accidentally wiring all your money to a stranger. Now, you do need to have side effects in a program on occasion. Like the previous example, you might need to write to a file. What you want to do is to centralize where you are doing this. Don't have several functions and classes that write to a particular file. -Have one service that does it. One and only one. +Have one service that does it. One and only one. The main point is to avoid common pitfalls like sharing state between objects without any structure, using mutable data types that can be written to by anything, and not centralizing where your side effects occur. If you can do this, you will be happier than the vast majority of other programmers. @@ -722,36 +1047,36 @@ The main point is to avoid common pitfalls like sharing state between objects wi ```ts // Global variable referenced by following function. -let name = 'Robert C. Martin'; +let name = 'Robert C. Martin' function toBase64() { - name = btoa(name); + name = btoa(name) } -toBase64(); +toBase64() // If we had another function that used this name, now it'd be a Base64 value -console.log(name); // expected to print 'Robert C. Martin' but instead 'Um9iZXJ0IEMuIE1hcnRpbg==' +console.log(name) // expected to print 'Robert C. Martin' but instead 'Um9iZXJ0IEMuIE1hcnRpbg==' ``` **Good:** ```ts -const name = 'Robert C. Martin'; +const name = 'Robert C. Martin' function toBase64(text: string): string { - return btoa(text); + return btoa(text) } -const encodedName = toBase64(name); -console.log(name); +const encodedName = toBase64(name) +console.log(name) ``` **[⬆ back to top](#table-of-contents)** ### Avoid Side Effects (part 2) -Browsers and Node.js process only JavaScript, therefore any TypeScript code has to be compiled before running or debugging. In JavaScript, some values are unchangeable (immutable) and some are changeable (mutable). Objects and arrays are two kinds of mutable values so it's important to handle them carefully when they're passed as parameters to a function. A JavaScript function can change an object's properties or alter the contents of an array which could easily cause bugs elsewhere. +Browsers and Node.js process only JavaScript, therefore any TypeScript code has to be compiled before running or debugging. In JavaScript, some values are unchangeable (immutable) and some are changeable (mutable). Objects and arrays are two kinds of mutable values so it's important to handle them carefully when they're passed as parameters to a function. A JavaScript function can change an object's properties or alter the contents of an array which could easily cause bugs elsewhere. Suppose there's a function that accepts an array parameter representing a shopping cart. If the function makes a change in that shopping cart array - by adding an item to purchase, for example - then any other function that uses that same `cart` array will be affected by this addition. That may be great, however it could also be bad. Let's imagine a bad situation: @@ -769,16 +1094,16 @@ Two caveats to mention to this approach: ```ts function addItemToCart(cart: CartItem[], item: Item): void { - cart.push({ item, date: Date.now() }); -}; + cart.push({ item, date: Date.now() }) +} ``` **Good:** ```ts function addItemToCart(cart: CartItem[], item: Item): CartItem[] { - return [...cart, { item, date: Date.now() }]; -}; + return [...cart, { item, date: Date.now() }] +} ``` **[⬆ back to top](#table-of-contents)** @@ -792,15 +1117,15 @@ Polluting globals is a bad practice in JavaScript because you could clash with a ```ts declare global { interface Array { - diff(other: T[]): Array; + diff(other: T[]): Array } } if (!Array.prototype.diff) { Array.prototype.diff = function (other: T[]): T[] { - const hash = new Set(other); - return this.filter(elem => !hash.has(elem)); - }; + const hash = new Set(other) + return this.filter((elem) => !hash.has(elem)) + } } ``` @@ -809,9 +1134,9 @@ if (!Array.prototype.diff) { ```ts class MyArray extends Array { diff(other: T[]): T[] { - const hash = new Set(other); - return this.filter(elem => !hash.has(elem)); - }; + const hash = new Set(other) + return this.filter((elem) => !hash.has(elem)) + } } ``` @@ -828,22 +1153,25 @@ const contributions = [ { name: 'Uncle Bobby', linesOfCode: 500 - }, { + }, + { name: 'Suzie Q', linesOfCode: 1500 - }, { + }, + { name: 'Jimmy Gosling', linesOfCode: 150 - }, { + }, + { name: 'Gracie Hopper', linesOfCode: 1000 } -]; +] -let totalOutput = 0; +let totalOutput = 0 for (let i = 0; i < contributions.length; i++) { - totalOutput += contributions[i].linesOfCode; + totalOutput += contributions[i].linesOfCode } ``` @@ -854,20 +1182,25 @@ const contributions = [ { name: 'Uncle Bobby', linesOfCode: 500 - }, { + }, + { name: 'Suzie Q', linesOfCode: 1500 - }, { + }, + { name: 'Jimmy Gosling', linesOfCode: 150 - }, { + }, + { name: 'Gracie Hopper', linesOfCode: 1000 } -]; +] -const totalOutput = contributions - .reduce((totalLines, output) => totalLines + output.linesOfCode, 0); +const totalOutput = contributions.reduce( + (totalLines, output) => totalLines + output.linesOfCode, + 0 +) ``` **[⬆ back to top](#table-of-contents)** @@ -886,7 +1219,7 @@ if (subscription.isTrial || account.balance > 0) { ```ts function canActivateService(subscription: Subscription, account: Account) { - return subscription.isTrial || account.balance > 0; + return subscription.isTrial || account.balance > 0 } if (canActivateService(subscription, account)) { @@ -924,85 +1257,20 @@ if (!isEmailUsed(email)) { **[⬆ back to top](#table-of-contents)** -### Avoid conditionals - -This seems like an impossible task. Upon first hearing this, most people say, "how am I supposed to do anything without an `if` statement?" The answer is that you can use polymorphism to achieve the same task in many cases. The second question is usually, "well that's great but why would I want to do that?" The answer is a previous clean code concept we learned: a function should only do one thing. When you have classes and functions that have `if` statements, you are telling your user that your function does more than one thing. Remember, just do one thing. - -**Bad:** - -```ts -class Airplane { - private type: string; - // ... - - getCruisingAltitude() { - switch (this.type) { - case '777': - return this.getMaxAltitude() - this.getPassengerCount(); - case 'Air Force One': - return this.getMaxAltitude(); - case 'Cessna': - return this.getMaxAltitude() - this.getFuelExpenditure(); - default: - throw new Error('Unknown airplane type.'); - } - } - - private getMaxAltitude(): number { - // ... - } -} -``` - -**Good:** - -```ts -abstract class Airplane { - protected getMaxAltitude(): number { - // shared logic with subclasses ... - } - - // ... -} - -class Boeing777 extends Airplane { - // ... - getCruisingAltitude() { - return this.getMaxAltitude() - this.getPassengerCount(); - } -} - -class AirForceOne extends Airplane { - // ... - getCruisingAltitude() { - return this.getMaxAltitude(); - } -} - -class Cessna extends Airplane { - // ... - getCruisingAltitude() { - return this.getMaxAltitude() - this.getFuelExpenditure(); - } -} -``` - -**[⬆ back to top](#table-of-contents)** - ### Avoid type checking TypeScript is a strict syntactical superset of JavaScript and adds optional static type checking to the language. Always prefer to specify types of variables, parameters and return values to leverage the full power of TypeScript features. -It makes refactoring more easier. +It makes refactoring easier. **Bad:** ```ts function travelToTexas(vehicle: Bicycle | Car) { if (vehicle instanceof Bicycle) { - vehicle.pedal(currentLocation, new Location('texas')); + vehicle.pedal(currentLocation, new Location('texas')) } else if (vehicle instanceof Car) { - vehicle.drive(currentLocation, new Location('texas')); + vehicle.drive(currentLocation, new Location('texas')) } } ``` @@ -1010,10 +1278,10 @@ function travelToTexas(vehicle: Bicycle | Car) { **Good:** ```ts -type Vehicle = Bicycle | Car; +type Vehicle = Bicycle | Car function travelToTexas(vehicle: Vehicle) { - vehicle.move(currentLocation, new Location('texas')); + vehicle.move(currentLocation, new Location('texas')) } ``` @@ -1059,8 +1327,8 @@ function requestModule(url: string) { // ... } -const req = requestModule; -inventoryTracker('apples', req, 'www.inventory-awesome.io'); +const req = requestModule +inventoryTracker('apples', req, 'www.inventory-awesome.io') ``` **Good:** @@ -1070,91 +1338,8 @@ function requestModule(url: string) { // ... } -const req = requestModule; -inventoryTracker('apples', req, 'www.inventory-awesome.io'); -``` - -**[⬆ back to top](#table-of-contents)** - -### Use iterators and generators - -Use generators and iterables when working with collections of data used like a stream. -There are some good reasons: - -- decouples the callee from the generator implementation in a sense that callee decides how many -items to access -- lazy execution, items are streamed on-demand -- built-in support for iterating items using the `for-of` syntax -- iterables allow implementing optimized iterator patterns - -**Bad:** - -```ts -function fibonacci(n: number): number[] { - if (n === 1) return [0]; - if (n === 2) return [0, 1]; - - const items: number[] = [0, 1]; - while (items.length < n) { - items.push(items[items.length - 2] + items[items.length - 1]); - } - - return items; -} - -function print(n: number) { - fibonacci(n).forEach(fib => console.log(fib)); -} - -// Print first 10 Fibonacci numbers. -print(10); -``` - -**Good:** - -```ts -// Generates an infinite stream of Fibonacci numbers. -// The generator doesn't keep the array of all numbers. -function* fibonacci(): IterableIterator { - let [a, b] = [0, 1]; - - while (true) { - yield a; - [a, b] = [b, a + b]; - } -} - -function print(n: number) { - let i = 0; - for (const fib of fibonacci()) { - if (i++ === n) break; - console.log(fib); - } -} - -// Print first 10 Fibonacci numbers. -print(10); -``` - -There are libraries that allow working with iterables in a similar way as with native arrays, by -chaining methods like `map`, `slice`, `forEach` etc. See [itiriri](https://www.npmjs.com/package/itiriri) for -an example of advanced manipulation with iterables (or [itiriri-async](https://www.npmjs.com/package/itiriri-async) for manipulation of async iterables). - -```ts -import itiriri from 'itiriri'; - -function* fibonacci(): IterableIterator { - let [a, b] = [0, 1]; - - while (true) { - yield a; - [a, b] = [b, a + b]; - } -} - -itiriri(fibonacci()) - .take(10) - .forEach(fib => console.log(fib)); +const req = requestModule +inventoryTracker('apples', req, 'www.inventory-awesome.io') ``` **[⬆ back to top](#table-of-contents)** @@ -1165,7 +1350,8 @@ itiriri(fibonacci()) TypeScript supports getter/setter syntax. Using getters and setters to access data from objects that encapsulate behavior could be better than simply looking for a property on an object. -"Why?" you might ask. Well, here's a list of reasons: + +Advantages: - When you want to do more beyond getting an object property, you don't have to look up and change every accessor in your codebase. - Makes adding validation simple when doing a `set`. @@ -1177,39 +1363,39 @@ Using getters and setters to access data from objects that encapsulate behavior ```ts type BankAccount = { - balance: number; + balance: number // ... } -const value = 100; +const value = 100 const account: BankAccount = { - balance: 0, + balance: 0 // ... -}; +} if (value < 0) { - throw new Error('Cannot set negative balance.'); + throw new Error('Cannot set negative balance.') } -account.balance = value; +account.balance = value ``` **Good:** ```ts class BankAccount { - private accountBalance: number = 0; + private _balance: number = 0 get balance(): number { - return this.accountBalance; + return this._balance } set balance(value: number) { if (value < 0) { - throw new Error('Cannot set negative balance.'); + throw new Error('Cannot set negative balance.') } - this.accountBalance = value; + this._balance = value } // ... @@ -1219,32 +1405,34 @@ class BankAccount { // If one day the specifications change, and we need extra validation rule, // we would have to alter only the `setter` implementation, // leaving all dependent code unchanged. -const account = new BankAccount(); -account.balance = 100; +const account = new BankAccount() +account.balance = 100 ``` **[⬆ back to top](#table-of-contents)** -### Make objects have private/protected members +### Use private/protected members where appropriate -TypeScript supports `public` *(default)*, `protected` and `private` accessors on class members. +TypeScript supports `public` _(default)_, `protected` and `private` accessors on class members. + +We can also use `readonly` to ensure that certain properties aren't updated (see below). **Bad:** ```ts class Circle { - radius: number; - + radius: number + constructor(radius: number) { - this.radius = radius; + this.radius = radius } perimeter() { - return 2 * Math.PI * this.radius; + return 2 * Math.PI * this.radius } surface() { - return Math.PI * this.radius * this.radius; + return Math.PI * this.radius * this.radius } } ``` @@ -1253,15 +1441,23 @@ class Circle { ```ts class Circle { - constructor(private readonly radius: number) { + private readonly _radius: number + + constructor(radius: number) { + this._radius = radius } perimeter() { - return 2 * Math.PI * this.radius; + return 2 * Math.PI * this._radius } surface() { - return Math.PI * this.radius * this.radius; + return Math.PI * this._radius * this._radius + } + + // If callers need access to the value, it can be exposed using a getter! + get radius() { + return this._radius } } ``` @@ -1270,16 +1466,17 @@ class Circle { ### Prefer immutability -TypeScript's type system allows you to mark individual properties on an interface/class as *readonly*. This allows you to work in a functional way (an unexpected mutation is bad). +TypeScript's type system allows you to mark individual properties on an interface/class as _readonly_. This allows you to work in a functional way (an unexpected mutation is bad). For more advanced scenarios there is a built-in type `Readonly` that takes a type `T` and marks all of its properties as readonly using mapped types (see [mapped types](https://www.typescriptlang.org/docs/handbook/advanced-types.html#mapped-types)). **Bad:** ```ts +// Assuming we don't want instances of Config to be changed after they're created interface Config { - host: string; - port: string; - db: string; + host: string + port: string + db: string } ``` @@ -1287,9 +1484,9 @@ interface Config { ```ts interface Config { - readonly host: string; - readonly port: string; - readonly db: string; + readonly host: string + readonly port: string + readonly db: string } ``` @@ -1299,24 +1496,16 @@ It doesn't allow changes such as `push()` and `fill()`, but can use features suc **Bad:** ```ts -const array: number[] = [ 1, 3, 5 ]; -array = []; // error -array.push(100); // array will be updated +const array: number[] = [1, 3, 5] +array = [] // error +array.push(100) // array will be updated ``` **Good:** -```ts -const array: ReadonlyArray = [ 1, 3, 5 ]; -array = []; // error -array.push(100); // error -``` - -Declaring read-only arguments in [TypeScript 3.4 is a bit easier](https://github.com/microsoft/TypeScript/wiki/What's-new-in-TypeScript#improvements-for-readonlyarray-and-readonly-tuples). - ```ts function hoge(args: readonly string[]) { - args.push(1); // error + args.push(1) // error } ``` @@ -1327,19 +1516,19 @@ Prefer [const assertions](https://github.com/microsoft/TypeScript/wiki/What's-ne ```ts const config = { hello: 'world' -}; -config.hello = 'world'; // value is changed +} +config.hello = 'world' // value is changed -const array = [ 1, 3, 5 ]; -array[0] = 10; // value is changed +const array = [1, 3, 5] +array[0] = 10 // value is changed // writable objects is returned function readonlyData(value: number) { - return { value }; + return { value } } -const result = readonlyData(100); -result.value = 200; // value is changed +const result = readonlyData(100) +result.value = 200 // value is changed ``` **Good:** @@ -1348,27 +1537,27 @@ result.value = 200; // value is changed // read-only object const config = { hello: 'world' -} as const; -config.hello = 'world'; // error +} as const +config.hello = 'world' // error // read-only array -const array = [ 1, 3, 5 ] as const; -array[0] = 10; // error +const array = [1, 3, 5] as const +array[0] = 10 // error // You can return read-only objects function readonlyData(value: number) { - return { value } as const; + return { value } as const } -const result = readonlyData(100); -result.value = 200; // error +const result = readonlyData(100) +result.value = 200 // error ``` **[⬆ back to top](#table-of-contents)** ### type vs. interface -Use type when you might need a union or intersection. Use an interface when you want `extends` or `implements`. There is no strict rule, however, use the one that works for you. +Use type when you might need a union or intersection. Use an interface when you want `extends` or `implements`. There is no strict rule, however, use the one that works for you. For a more detailed explanation refer to this [answer](https://stackoverflow.com/questions/37233735/typescript-interfaces-vs-types/54101543#54101543) about the differences between `type` and `interface` in TypeScript. **Bad:** @@ -1396,7 +1585,6 @@ type Shape = { **Good:** ```ts - type EmailConfig = { // ... } @@ -1405,7 +1593,7 @@ type DbConfig = { // ... } -type Config = EmailConfig | DbConfig; +type Config = EmailConfig | DbConfig // ... @@ -1428,54 +1616,92 @@ class Square implements Shape { ### Classes should be small -The class' size is measured by its responsibility. Following the *Single Responsibility principle* a class should be small. +The class' size is measured by its responsibility. Following the _Single Responsibility principle_ a class should be small. **Bad:** ```ts class Dashboard { - getLanguage(): string { /* ... */ } - setLanguage(language: string): void { /* ... */ } - showProgress(): void { /* ... */ } - hideProgress(): void { /* ... */ } - isDirty(): boolean { /* ... */ } - disable(): void { /* ... */ } - enable(): void { /* ... */ } - addSubscription(subscription: Subscription): void { /* ... */ } - removeSubscription(subscription: Subscription): void { /* ... */ } - addUser(user: User): void { /* ... */ } - removeUser(user: User): void { /* ... */ } - goToHomePage(): void { /* ... */ } - updateProfile(details: UserDetails): void { /* ... */ } - getVersion(): string { /* ... */ } + getLanguage(): string { + /* ... */ + } + setLanguage(language: string): void { + /* ... */ + } + showProgress(): void { + /* ... */ + } + hideProgress(): void { + /* ... */ + } + isDirty(): boolean { + /* ... */ + } + disable(): void { + /* ... */ + } + enable(): void { + /* ... */ + } + addSubscription(subscription: Subscription): void { + /* ... */ + } + removeSubscription(subscription: Subscription): void { + /* ... */ + } + addUser(user: User): void { + /* ... */ + } + removeUser(user: User): void { + /* ... */ + } + goToHomePage(): void { + /* ... */ + } + updateProfile(details: UserDetails): void { + /* ... */ + } + getVersion(): string { + /* ... */ + } // ... } - ``` **Good:** ```ts class Dashboard { - disable(): void { /* ... */ } - enable(): void { /* ... */ } - getVersion(): string { /* ... */ } + disable(): void { + /* ... */ + } + enable(): void { + /* ... */ + } + getVersion(): string { + /* ... */ + } } -// split the responsibilities by moving the remaining methods to other classes -// ... +// split the responsibilities by moving the remaining methods to other classes, for example +class DashboardUser { + // ... +} + +class DashboardSubscriptions { + //... +} ``` **[⬆ back to top](#table-of-contents)** -### High cohesion and low coupling +### Prefer high cohesion and loose coupling -Cohesion defines the degree to which class members are related to each other. Ideally, all fields within a class should be used by each method. -We then say that the class is *maximally cohesive*. In practice, this, however, is not always possible, nor even advisable. You should however prefer cohesion to be high. +Cohesion defines the degree to which class members are related to each other. If a class is _maximally cohesive_, all fields within a class should be used by each method. In practice, this level of cohesion is rarely possible, but we should consider low cohesion to be a code smell, and a sign that a class might be cleaner if split into two or more classes. -Coupling refers to how related or dependent are two classes toward each other. Classes are said to be low coupled if changes in one of them don't affect the other one. - -Good software design has **high cohesion** and **low coupling**. +Coupling refers to how related or dependent are two classes toward each other. Classes are said to be loosely coupled if changes in one of them don't affect the other one. + +Good software design has **high cohesion** and **loose coupling**. **Bad:** @@ -1487,23 +1713,23 @@ class UserManager { // I'm still forced to pass and instance of `emailSender`. constructor( private readonly db: Database, - private readonly emailSender: EmailSender) { - } + private readonly emailSender: EmailSender + ) {} async getUser(id: number): Promise { - return await db.users.findOne({ id }); + return await db.users.findOne({ id }) } async getTransactions(userId: number): Promise { - return await db.transactions.find({ userId }); + return await db.transactions.find({ userId }) } async sendGreeting(): Promise { - await emailSender.send('Welcome!'); + await emailSender.send('Welcome!') } async sendNotification(text: string): Promise { - await emailSender.send(text); + await emailSender.send(text) } async sendNewsletter(): Promise { @@ -1515,29 +1741,27 @@ class UserManager { **Good:** ```ts -class UserService { - constructor(private readonly db: Database) { - } - +class UserModel { async getUser(id: number): Promise { - return await this.db.users.findOne({ id }); + return await db.users.findOne({ id }) } +} +class TransactionModel { async getTransactions(userId: number): Promise { - return await this.db.transactions.find({ userId }); + return await db.transactions.find({ userId }) } } class UserNotifier { - constructor(private readonly emailSender: EmailSender) { - } + constructor(private readonly emailSender: EmailSender) {} async sendGreeting(): Promise { - await this.emailSender.send('Welcome!'); + await this.emailSender.send('Welcome!') } async sendNotification(text: string): Promise { - await this.emailSender.send(text); + await this.emailSender.send(text) } async sendNewsletter(): Promise { @@ -1550,24 +1774,21 @@ class UserNotifier { ### Prefer composition over inheritance -As stated famously in [Design Patterns](https://en.wikipedia.org/wiki/Design_Patterns) by the Gang of Four, you should *prefer composition over inheritance* where you can. There are lots of good reasons to use inheritance and lots of good reasons to use composition. The main point for this maxim is that if your mind instinctively goes for inheritance, try to think if composition could model your problem better. In some cases it can. - -You might be wondering then, "when should I use inheritance?" It depends on your problem at hand, but this is a decent list of when inheritance makes more sense than composition: +As stated famously in [Design Patterns](https://en.wikipedia.org/wiki/Design_Patterns) by the Gang of Four, we should _prefer composition over inheritance_ where we can. There are lots of good reasons to use inheritance and lots of good reasons to use composition. When tempted to use inheritance, we should first try to think if composition could model our problem better. In many cases it can. -1. Your inheritance represents an "is-a" relationship and not a "has-a" relationship (Human->Animal vs. User->UserDetails). +However, there are a few cases where we still might want to use inheritance: -2. You can reuse code from the base classes (Humans can move like all animals). +1. Our inheritance represents an "is-a" relationship and not a "has-a" relationship (Human->Animal vs. User->UserDetails). -3. You want to make global changes to derived classes by changing a base class. (Change the caloric expenditure of all animals when they move). +2. We can reuse code from the base classes (Humans can move like all animals). + +3. We want to make global changes to derived classes by changing a base class. (Change the caloric expenditure of all animals when they move). **Bad:** ```ts class Employee { - constructor( - private readonly name: string, - private readonly email: string) { - } + constructor(private readonly name: string, private readonly email: string) {} // ... } @@ -1578,8 +1799,9 @@ class EmployeeTaxData extends Employee { name: string, email: string, private readonly ssn: string, - private readonly salary: number) { - super(name, email); + private readonly salary: number + ) { + super(name, email) } // ... @@ -1590,26 +1812,20 @@ class EmployeeTaxData extends Employee { ```ts class Employee { - private taxData: EmployeeTaxData; + private taxData: EmployeeTaxData - constructor( - private readonly name: string, - private readonly email: string) { - } + constructor(private readonly name: string, private readonly email: string) {} setTaxData(ssn: string, salary: number): Employee { - this.taxData = new EmployeeTaxData(ssn, salary); - return this; + this.taxData = new EmployeeTaxData(ssn, salary) + return this } // ... } class EmployeeTaxData { - constructor( - public readonly ssn: string, - public readonly salary: number) { - } + constructor(public readonly ssn: string, public readonly salary: number) {} // ... } @@ -1617,70 +1833,33 @@ class EmployeeTaxData { **[⬆ back to top](#table-of-contents)** -### Use method chaining +### Avoid fluid interfaces and method chaining -This pattern is very useful and commonly used in many libraries. It allows your code to be expressive, and less verbose. For that reason, use method chaining and take a look at how clean your code will be. +Fluid interfaces make code more [difficult to maintain](https://www.yegor256.com/2018/03/13/fluent-interfaces.html). and place awkward constraints on the future expansion of functionality. **Bad:** ```ts class QueryBuilder { - private collection: string; - private pageNumber: number = 1; - private itemsPerPage: number = 100; - private orderByFields: string[] = []; - - from(collection: string): void { - this.collection = collection; - } - - page(number: number, itemsPerPage: number = 100): void { - this.pageNumber = number; - this.itemsPerPage = itemsPerPage; - } - - orderBy(...fields: string[]): void { - this.orderByFields = fields; - } - - build(): Query { - // ... - } -} - -// ... - -const queryBuilder = new QueryBuilder(); -queryBuilder.from('users'); -queryBuilder.page(1, 100); -queryBuilder.orderBy('firstName', 'lastName'); - -const query = queryBuilder.build(); -``` - -**Good:** - -```ts -class QueryBuilder { - private collection: string; - private pageNumber: number = 1; - private itemsPerPage: number = 100; - private orderByFields: string[] = []; + private collection: string + private pageNumber: number = 1 + private itemsPerPage: number = 100 + private orderByFields: string[] = [] from(collection: string): this { - this.collection = collection; - return this; + this.collection = collection + return this } page(number: number, itemsPerPage: number = 100): this { - this.pageNumber = number; - this.itemsPerPage = itemsPerPage; - return this; + this.pageNumber = number + this.itemsPerPage = itemsPerPage + return this } orderBy(...fields: string[]): this { - this.orderByFields = fields; - return this; + this.orderByFields = fields + return this } build(): Query { @@ -1694,7 +1873,44 @@ const query = new QueryBuilder() .from('users') .page(1, 100) .orderBy('firstName', 'lastName') - .build(); + .build() +``` + +**Good:** + +```ts +class QueryBuilder { + private collection: string + private pageNumber: number = 1 + private itemsPerPage: number = 100 + private orderByFields: string[] = [] + + from(collection: string): void { + this.collection = collection + } + + page(number: number, itemsPerPage: number = 100): void { + this.pageNumber = number + this.itemsPerPage = itemsPerPage + } + + orderBy(...fields: string[]): void { + this.orderByFields = fields + } + + build(): Query { + // ... + } +} + +// ... + +const queryBuilder = new QueryBuilder() +queryBuilder.from('users') +queryBuilder.page(1, 100) +queryBuilder.orderBy('firstName', 'lastName') + +const query = queryBuilder.build() ``` **[⬆ back to top](#table-of-contents)** @@ -1703,14 +1919,23 @@ const query = new QueryBuilder() ### Single Responsibility Principle (SRP) -As stated in Clean Code, "There should never be more than one reason for a class to change". It's tempting to jam-pack a class with a lot of functionality, like when you can only take one suitcase on your flight. The issue with this is that your class won't be conceptually cohesive and it will give it many reasons to change. Minimizing the amount of time you need to change a class is important. It's important because if too much functionality is in one class and you modify a piece of it, it can be difficult to understand how that will affect other dependent modules in your codebase. +A module or class should only ever have a single "reason to change". This is really just another way to describe High Cohesion, as discussed above. + +For example, suppose we have a module that generates a PDF report. It includes functions to fetch the data for the report, functions to format the data using templates, and functions to write the output PDF to a file. + +This module is operating at multiple levels of abstraction, and has three "reasons to change": + +1. The report input data could change +2. The format of the report could change +3. The expected output could change + +Identifying these "reasons to change" can help us understand how to break large modules down into smaller pieces, resulting in improved readability and maintainability. **Bad:** ```ts class UserSettings { - constructor(private readonly user: User) { - } + constructor(private readonly user: User) {} changeSettings(settings: UserSettings) { if (this.verifyCredentials()) { @@ -1728,20 +1953,18 @@ class UserSettings { ```ts class UserAuth { - constructor(private readonly user: User) { - } + constructor(private readonly user: User) {} verifyCredentials() { // ... } } - class UserSettings { - private readonly auth: UserAuth; + private readonly auth: UserAuth constructor(private readonly user: User) { - this.auth = new UserAuth(user); + this.auth = new UserAuth(user) } changeSettings(settings: UserSettings) { @@ -1756,112 +1979,59 @@ class UserSettings { ### Open/Closed Principle (OCP) -As stated by Bertrand Meyer, "software entities (classes, modules, functions, etc.) should be open for extension, but closed for modification." What does that mean though? This principle basically states that you should allow users to add new functionalities without changing existing code. +As stated by Bertrand Meyer, "software entities (classes, modules, functions, etc.) should be open for extension, but closed for modification." Although it was originally discussed primarily in terms of OOP and inheritance, at a high level it can be thought of as the "plugin principle". + +One great example of this principle is VSCode, with its vast library of extensions. It's plugin-driven architecture allows us to extend the functionality of the software, without having to modify the source code. **Bad:** ```ts -class AjaxAdapter extends Adapter { - constructor() { - super(); - } +type CellFormats = 'string' | 'dollars' - // ... +interface GridArgs { + rows: Array> + formats: Record } -class NodeAdapter extends Adapter { - constructor() { - super(); - } - - // ... +const DataGrid = ({ rows, formats }: GridArgs) => { + // ... renders the data in a grid } -class HttpRequester { - constructor(private readonly adapter: Adapter) { - } - - async fetch(url: string): Promise { - if (this.adapter instanceof AjaxAdapter) { - const response = await makeAjaxCall(url); - // transform response and return - } else if (this.adapter instanceof NodeAdapter) { - const response = await makeHttpCall(url); - // transform response and return - } - } -} - -function makeAjaxCall(url: string): Promise { - // request and return promise -} - -function makeHttpCall(url: string): Promise { - // request and return promise -} +// Usage: + ``` **Good:** ```ts -abstract class Adapter { - abstract async request(url: string): Promise; +type CellFormatter = (value: any) => ReactNode - // code shared to subclasses ... +interface GridArgs { + rows: Array> + formatters: Record } -class AjaxAdapter extends Adapter { - constructor() { - super(); - } - - async request(url: string): Promise{ - // request and return promise - } - - // ... +const DataGrid = ({ rows, formatters }: GridArgs) => { + // ... renders the data in a grid } -class NodeAdapter extends Adapter { - constructor() { - super(); - } - - async request(url: string): Promise{ - // request and return promise - } - - // ... -} - -class HttpRequester { - constructor(private readonly adapter: Adapter) { - } - - async fetch(url: string): Promise { - const response = await this.adapter.request(url); - // transform response and return - } -} +// Callers can use any formatter functions they prefer! + ``` **[⬆ back to top](#table-of-contents)** ### Liskov Substitution Principle (LSP) -This is a scary term for a very simple concept. It's formally defined as "If S is a subtype of T, then objects of type T may be replaced with objects of type S (i.e., objects of type S may substitute objects of type T) without altering any of the desirable properties of that program (correctness, task performed, etc.)." That's an even scarier definition. - -The best explanation for this is if you have a parent class and a child class, then the parent class and child class can be used interchangeably without getting incorrect results. This might still be confusing, so let's take a look at the classic Square-Rectangle example. Mathematically, a square is a rectangle, but if you model it using the "is-a" relationship via inheritance, you quickly get into trouble. +Formal Definition: "If S is a subtype of T, then objects of type T may be replaced with objects of type S (i.e., objects of type S may substitute objects of type T) without altering any of the desirable properties of that program (correctness, task performed, etc.)." + +More simply, if we have a parent class and a child class, then the parent class and child class can be used interchangeably without getting incorrect results. A classic example of this is the Square-Rectangle problem. Mathematically, a square is a rectangle, but if we model it using the "is-a" relationship via inheritance, we quickly get into trouble. **Bad:** ```ts class Rectangle { - constructor( - protected width: number = 0, - protected height: number = 0) { - - } + constructor(protected width: number = 0, protected height: number = 0) {} setColor(color: string): this { // ... @@ -1872,46 +2042,43 @@ class Rectangle { } setWidth(width: number): this { - this.width = width; - return this; + this.width = width + return this } setHeight(height: number): this { - this.height = height; - return this; + this.height = height + return this } getArea(): number { - return this.width * this.height; + return this.width * this.height } } class Square extends Rectangle { setWidth(width: number): this { - this.width = width; - this.height = width; - return this; + this.width = width + this.height = width + return this } setHeight(height: number): this { - this.width = height; - this.height = height; - return this; + this.width = height + this.height = height + return this } } function renderLargeRectangles(rectangles: Rectangle[]) { rectangles.forEach((rectangle) => { - const area = rectangle - .setWidth(4) - .setHeight(5) - .getArea(); // BAD: Returns 25 for Square. Should be 20. - rectangle.render(area); - }); + const area = rectangle.setWidth(4).setHeight(5).getArea() // BAD: Returns 25 for Square. Should be 20. + rectangle.render(area) + }) } -const rectangles = [new Rectangle(), new Rectangle(), new Square()]; -renderLargeRectangles(rectangles); +const rectangles = [new Rectangle(), new Rectangle(), new Square()] +renderLargeRectangles(rectangles) ``` **Good:** @@ -1926,63 +2093,62 @@ abstract class Shape { // ... } - abstract getArea(): number; + abstract getArea(): number } class Rectangle extends Shape { - constructor( - private readonly width = 0, - private readonly height = 0) { - super(); + constructor(private readonly width = 0, private readonly height = 0) { + super() } getArea(): number { - return this.width * this.height; + return this.width * this.height } } class Square extends Shape { constructor(private readonly length: number) { - super(); + super() } getArea(): number { - return this.length * this.length; + return this.length * this.length } } function renderLargeShapes(shapes: Shape[]) { shapes.forEach((shape) => { - const area = shape.getArea(); - shape.render(area); - }); + const area = shape.getArea() + shape.render(area) + }) } -const shapes = [new Rectangle(4, 5), new Rectangle(4, 5), new Square(5)]; -renderLargeShapes(shapes); +const shapes = [new Rectangle(4, 5), new Rectangle(4, 5), new Square(5)] +renderLargeShapes(shapes) ``` **[⬆ back to top](#table-of-contents)** ### Interface Segregation Principle (ISP) -ISP states that "Clients should not be forced to depend upon interfaces that they do not use.". This principle is very much related to the Single Responsibility Principle. -What it really means is that you should always design your abstractions in a way that the clients that are using the exposed methods do not get the whole pie instead. That also include imposing the clients with the burden of implementing methods that they don’t actually need. +ISP states that "Clients should not be forced to depend upon interfaces that they do not use.". This principle is very much related to the Single Responsibility Principle. We should try to design abstractions that don't burden the consumer of the abstraction with functionality they don't need. + +One classic example of this are overly-complex interfaces or abstract classes, which expect the implementer to include functionality they may not need. **Bad:** ```ts interface SmartPrinter { - print(); - fax(); - scan(); + print() + fax() + scan() } class AllInOnePrinter implements SmartPrinter { print() { // ... - } - + } + fax() { // ... } @@ -1995,14 +2161,14 @@ class AllInOnePrinter implements SmartPrinter { class EconomicPrinter implements SmartPrinter { print() { // ... - } - + } + fax() { - throw new Error('Fax not supported.'); + throw new Error('Fax not supported.') } scan() { - throw new Error('Scan not supported.'); + throw new Error('Scan not supported.') } } ``` @@ -2011,22 +2177,22 @@ class EconomicPrinter implements SmartPrinter { ```ts interface Printer { - print(); + print() } interface Fax { - fax(); + fax() } interface Scanner { - scan(); + scan() } class AllInOnePrinter implements Printer, Fax, Scanner { print() { // ... - } - + } + fax() { // ... } @@ -2053,17 +2219,19 @@ This principle states two essential things: 2. Abstractions should not depend upon details. Details should depend on abstractions. -This can be hard to understand at first, but if you've worked with Angular, you've seen an implementation of this principle in the form of Dependency Injection (DI). While they are not identical concepts, DIP keeps high-level modules from knowing the details of its low-level modules and setting them up. It can accomplish this through DI. A huge benefit of this is that it reduces the coupling between modules. Coupling is a very bad development pattern because it makes your code hard to refactor. - -DIP is usually achieved by a using an inversion of control (IoC) container. An example of a powerful IoC container for TypeScript is [InversifyJs](https://www.npmjs.com/package/inversify) +A great example of this is a generic shopping cart API, which can be used to checkout via multiple e-commerce platforms. (Sound familiar?) + +Rather than writing multiple checkout APIs with similar workflows, we can write a single set of APIs which operates at a high-level. + +The details of each e-commerce platform can be abstracted away into a set of adapters, and the high-level APIs don't need to know the details. **Bad:** ```ts -import { readFile as readFileCb } from 'fs'; -import { promisify } from 'util'; +import { readFile as readFileCb } from 'fs' +import { promisify } from 'util' -const readFile = promisify(readFileCb); +const readFile = promisify(readFileCb) type ReportData = { // .. @@ -2076,36 +2244,35 @@ class XmlFormatter { } class ReportReader { - // BAD: We have created a dependency on a specific request implementation. // We should just have ReportReader depend on a parse method: `parse` - private readonly formatter = new XmlFormatter(); + private readonly formatter = new XmlFormatter() async read(path: string): Promise { - const text = await readFile(path, 'UTF8'); - return this.formatter.parse(text); + const text = await readFile(path, 'UTF8') + return this.formatter.parse(text) } } // ... -const reader = new ReportReader(); -const report = await reader.read('report.xml'); +const reader = new ReportReader() +const report = await reader.read('report.xml') ``` **Good:** ```ts -import { readFile as readFileCb } from 'fs'; -import { promisify } from 'util'; +import { readFile as readFileCb } from 'fs' +import { promisify } from 'util' -const readFile = promisify(readFileCb); +const readFile = promisify(readFileCb) type ReportData = { // .. } interface Formatter { - parse(content: string): T; + parse(content: string): T } class XmlFormatter implements Formatter { @@ -2114,7 +2281,6 @@ class XmlFormatter implements Formatter { } } - class JsonFormatter implements Formatter { parse(content: string): T { // Converts a JSON string to an object T @@ -2122,41 +2288,32 @@ class JsonFormatter implements Formatter { } class ReportReader { - constructor(private readonly formatter: Formatter) { - } + constructor(private readonly formatter: Formatter) {} async read(path: string): Promise { - const text = await readFile(path, 'UTF8'); - return this.formatter.parse(text); + const text = await readFile(path, 'UTF8') + return this.formatter.parse(text) } } // ... -const reader = new ReportReader(new XmlFormatter()); -const report = await reader.read('report.xml'); +const reader = new ReportReader(new XmlFormatter()) +const report = await reader.read('report.xml') // or if we had to read a json report -const reader = new ReportReader(new JsonFormatter()); -const report = await reader.read('report.json'); +const reader = new ReportReader(new JsonFormatter()) +const report = await reader.read('report.json') ``` **[⬆ back to top](#table-of-contents)** ## Testing -Testing is more important than shipping. If you have no tests or an inadequate amount, then every time you ship code you won't be sure that you didn't break anything. -Deciding on what constitutes an adequate amount is up to your team, but having 100% coverage (all statements and branches) -is how you achieve very high confidence and developer peace of mind. This means that in addition to having a great testing framework, you also need to use a good [coverage tool](https://github.com/gotwarlost/istanbul). +Testing is just as important as shipping. If we don't have appropriate tests in place, we risk breaking something every time we ship. -There's no excuse to not write tests. There are [plenty of good JS test frameworks](http://jstherightway.org/#testing-tools) with typings support for TypeScript, so find one that your team prefers. When you find one that works for your team, then aim to always write tests for every new feature/module you introduce. If your preferred method is Test Driven Development (TDD), that is great, but the main point is to just make sure you are reaching your coverage goals before launching any feature, or refactoring an existing one. +Test Driven Development (TDD) and code coverage may not be appropriate for every team, but the team needs to have a shared goal: to have adequate testing in place before launching any feature, or refactoring existing functionality. -### The three laws of TDD - -1. You are not allowed to write any production code unless it is to make a failing unit test pass. - -2. You are not allowed to write any more of a unit test than is sufficient to fail, and; compilation failures are failures. - -3. You are not allowed to write any more production code than is sufficient to pass the one failing unit test. +There's no excuse to not write tests. **[⬆ back to top](#table-of-contents)** @@ -2170,59 +2327,9 @@ Clean tests should follow the rules: - **Repeatable** tests should be repeatable in any environment and there should be no excuse for why they fail. -- **Self-Validating** a test should answer with either *Passed* or *Failed*. You don't need to compare log files to answer if a test passed. +- **Self-Validating** a test should answer with either _Passed_ or _Failed_. You don't need to compare log files to answer if a test passed. -- **Timely** unit tests should be written before the production code. If you write tests after the production code, you might find writing tests too hard. - -**[⬆ back to top](#table-of-contents)** - -### Single concept per test - -Tests should also follow the *Single Responsibility Principle*. Make only one assert per unit test. - -**Bad:** - -```ts -import { assert } from 'chai'; - -describe('AwesomeDate', () => { - it('handles date boundaries', () => { - let date: AwesomeDate; - - date = new AwesomeDate('1/1/2015'); - assert.equal('1/31/2015', date.addDays(30)); - - date = new AwesomeDate('2/1/2016'); - assert.equal('2/29/2016', date.addDays(28)); - - date = new AwesomeDate('2/1/2015'); - assert.equal('3/1/2015', date.addDays(28)); - }); -}); -``` - -**Good:** - -```ts -import { assert } from 'chai'; - -describe('AwesomeDate', () => { - it('handles 30-day months', () => { - const date = new AwesomeDate('1/1/2015'); - assert.equal('1/31/2015', date.addDays(30)); - }); - - it('handles leap year', () => { - const date = new AwesomeDate('2/1/2016'); - assert.equal('2/29/2016', date.addDays(28)); - }); - - it('handles non-leap year', () => { - const date = new AwesomeDate('2/1/2015'); - assert.equal('3/1/2015', date.addDays(28)); - }); -}); -``` +- **Timely** tests should be written (and pass) before code leaves a feature branch. (Although this is not an excuse to avoid writing tests for existing code!) **[⬆ back to top](#table-of-contents)** @@ -2236,12 +2343,12 @@ When a test fails, its name is the first indication of what may have gone wrong. describe('Calendar', () => { it('2/29/2020', () => { // ... - }); + }) it('throws', () => { // ... - }); -}); + }) +}) ``` **Good:** @@ -2250,130 +2357,69 @@ describe('Calendar', () => { describe('Calendar', () => { it('should handle leap year', () => { // ... - }); + }) it('should throw when format is invalid', () => { // ... - }); -}); + }) +}) ``` **[⬆ back to top](#table-of-contents)** ## Concurrency -### Prefer promises vs callbacks - -Callbacks aren't clean, and they cause excessive amounts of nesting *(the callback hell)*. -There are utilities that transform existing functions using the callback style to a version that returns promises -(for Node.js see [`util.promisify`](https://nodejs.org/dist/latest-v8.x/docs/api/util.html#util_util_promisify_original), for general purpose see [pify](https://www.npmjs.com/package/pify), [es6-promisify](https://www.npmjs.com/package/es6-promisify)) - -**Bad:** - -```ts -import { get } from 'request'; -import { writeFile } from 'fs'; - -function downloadPage(url: string, saveTo: string, callback: (error: Error, content?: string) => void) { - get(url, (error, response) => { - if (error) { - callback(error); - } else { - writeFile(saveTo, response.body, (error) => { - if (error) { - callback(error); - } else { - callback(null, response.body); - } - }); - } - }); -} - -downloadPage('https://en.wikipedia.org/wiki/Robert_Cecil_Martin', 'article.html', (error, content) => { - if (error) { - console.error(error); - } else { - console.log(content); - } -}); -``` - -**Good:** - -```ts -import { get } from 'request'; -import { writeFile } from 'fs'; -import { promisify } from 'util'; - -const write = promisify(writeFile); - -function downloadPage(url: string, saveTo: string): Promise { - return get(url) - .then(response => write(saveTo, response)); -} - -downloadPage('https://en.wikipedia.org/wiki/Robert_Cecil_Martin', 'article.html') - .then(content => console.log(content)) - .catch(error => console.error(error)); -``` - -Promises supports a few helper methods that help make code more concise: - -| Pattern | Description | -| ------------------------ | ----------------------------------------- | -| `Promise.resolve(value)` | Convert a value into a resolved promise. | -| `Promise.reject(error)` | Convert an error into a rejected promise. | -| `Promise.all(promises)` | Returns a new promise which is fulfilled with an array of fulfillment values for the passed promises or rejects with the reason of the first promise that rejects. | -| `Promise.race(promises)`| Returns a new promise which is fulfilled/rejected with the result/error of the first settled promise from the array of passed promises. | - -`Promise.all` is especially useful when there is a need to run tasks in parallel. `Promise.race` makes it easier to implement things like timeouts for promises. - -**[⬆ back to top](#table-of-contents)** - -### Async/Await are even cleaner than Promises +### Prefer Async/Await over Promises With `async`/`await` syntax you can write code that is far cleaner and more understandable than chained promises. Within a function prefixed with `async` keyword, you have a way to tell the JavaScript runtime to pause the execution of code on the `await` keyword (when used on a promise). +Avoid old-style node callbacks (sometimes called "errorbacks"), at all costs. + **Bad:** ```ts -import { get } from 'request'; -import { writeFile } from 'fs'; -import { promisify } from 'util'; +import { get } from 'request' +import { writeFile } from 'fs' +import { promisify } from 'util' -const write = util.promisify(writeFile); +const write = util.promisify(writeFile) function downloadPage(url: string, saveTo: string): Promise { - return get(url).then(response => write(saveTo, response)); + return get(url).then((response) => write(saveTo, response)) } -downloadPage('https://en.wikipedia.org/wiki/Robert_Cecil_Martin', 'article.html') - .then(content => console.log(content)) - .catch(error => console.error(error)); +downloadPage( + 'https://en.wikipedia.org/wiki/Robert_Cecil_Martin', + 'article.html' +) + .then((content) => console.log(content)) + .catch((error) => console.error(error)) ``` **Good:** ```ts -import { get } from 'request'; -import { writeFile } from 'fs'; -import { promisify } from 'util'; +import { get } from 'request' +import { writeFile } from 'fs' +import { promisify } from 'util' -const write = promisify(writeFile); +const write = promisify(writeFile) async function downloadPage(url: string): Promise { - const response = await get(url); - return response; + const response = await get(url) + return response } // somewhere in an async function try { - const content = await downloadPage('https://en.wikipedia.org/wiki/Robert_Cecil_Martin'); - await write('article.html', content); - console.log(content); + const content = await downloadPage( + 'https://en.wikipedia.org/wiki/Robert_Cecil_Martin' + ) + await write('article.html', content) + console.log(content) } catch (error) { - console.error(error); + console.error(error) + throw error } ``` @@ -2386,21 +2432,17 @@ execution on the current stack, killing the process (in Node), and notifying you ### Always use Error for throwing or rejecting -JavaScript as well as TypeScript allow you to `throw` any object. A Promise can also be rejected with any reason object. +JavaScript as well as TypeScript allow you to `throw` any object. A Promise can also be rejected with any reason object. It is advisable to use the `throw` syntax with an `Error` type. This is because your error might be caught in higher level code with a `catch` syntax. It would be very confusing to catch a string message there and would make -[debugging more painful](https://basarat.gitbook.io/typescript/type-system/exceptions#always-use-error). +[debugging more painful](https://basarat.gitbook.io/typescript/type-system/exceptions#always-use-error). For the same reason you should reject promises with `Error` types. **Bad:** ```ts function calculateTotal(items: Item[]): number { - throw 'Not implemented.'; -} - -function get(): Promise { - return Promise.reject('Not implemented.'); + throw 'Not implemented.' } ``` @@ -2408,61 +2450,35 @@ function get(): Promise { ```ts function calculateTotal(items: Item[]): number { - throw new Error('Not implemented.'); -} - -function get(): Promise { - return Promise.reject(new Error('Not implemented.')); -} - -// or equivalent to: - -async function get(): Promise { - throw new Error('Not implemented.'); + throw new Error('calculateTotal is not implemented.') } ``` -The benefit of using `Error` types is that it is supported by the syntax `try/catch/finally` and implicitly all errors have the `stack` property which -is very powerful for debugging. -There are also other alternatives, not to use the `throw` syntax and instead always return custom error objects. TypeScript makes this even easier. -Consider the following example: - -```ts -type Result = { isError: false, value: R }; -type Failure = { isError: true, error: E }; -type Failable = Result | Failure; - -function calculateTotal(items: Item[]): Failable { - if (items.length === 0) { - return { isError: true, error: 'empty' }; - } - - // ... - return { isError: false, value: 42 }; -} -``` - -For the detailed explanation of this idea refer to the [original post](https://medium.com/@dhruvrajvanshi/making-exceptions-type-safe-in-typescript-c4d200ee78e9). - **[⬆ back to top](#table-of-contents)** -### Don't ignore caught errors +### When logging errors, include detailed error messages and related data -Doing nothing with a caught error doesn't give you the ability to ever fix or react to said error. Logging the error to the console (`console.log`) isn't much better as often it can get lost in a sea of things printed to the console. If you wrap any bit of code in a `try/catch` it means you think an error may occur there and therefore you should have a plan, or create a code path, for when it occurs. +Whenever we catch and log an error, we should try to imagine what information would be useful to us when debugging related problems. However, we also need to be careful not to log PII or secrets. + +A few basic guidelines around error logging: + +- When an operation fails, try to at least log the IDs of the data entities involved +- When an algorithm or computation fails, try to log the most important inputs +- Modern logging systems can query by JSON properties in logged data, logging objects directly is often preferable over constructing string-based messages **Bad:** ```ts try { - functionThatMightThrow(); + calculateSubTotal({ lineItems, discounts }) } catch (error) { - console.log(error); + console.log(error) } // or even worse try { - functionThatMightThrow(); + calculateSubTotal({ lineItems, discounts }) } catch (error) { // ignore error } @@ -2471,53 +2487,49 @@ try { **Good:** ```ts -import { logger } from './logging' - try { - functionThatMightThrow(); + calculateSubTotal({ lineItems, discounts }) } catch (error) { - logger.log(error); + logger.error('An error was thrown attempting to calculate cart subtotal', { + error, + lineItems, + discounts + }) + throw error } ``` **[⬆ back to top](#table-of-contents)** -### Don't ignore rejected promises +### Re-throw errors by default -For the same reason you shouldn't ignore caught errors from `try/catch`. +Unless we are certain we can recover from an error, we should usually re-throw the error **Bad:** ```ts -getUser() - .then((user: User) => { - return sendEmail(user.email, 'Welcome!'); - }) - .catch((error) => { - console.log(error); - }); +try { + calculateTotals({ lineItems, discounts, tax, shipping }) +} catch (error) { + console.log(error) + // Okay... but now our cart doesn't have any subtotals?! Is it safe to keep going? +} ``` **Good:** ```ts -import { logger } from './logging' - -getUser() - .then((user: User) => { - return sendEmail(user.email, 'Welcome!'); - }) - .catch((error) => { - logger.log(error); - }); - -// or using the async/await syntax: - try { - const user = await getUser(); - await sendEmail(user.email, 'Welcome!'); + calculateTotals({ lineItems, discounts, tax, shipping }) } catch (error) { - logger.log(error); + logger.error('An error was thrown attempting to calculate cart totals', { + lineItems, + discounts, + tax, + shipping + }) + logger.error(error) + throw error } ``` @@ -2525,102 +2537,98 @@ try { ## Formatting -Formatting is subjective. Like many rules herein, there is no hard and fast rule that you must follow. The main point is *DO NOT ARGUE* over formatting. There are tons of tools to automate this. Use one! It's a waste of time and money for engineers to argue over formatting. The general rule to follow is *keep consistent formatting rules*. +Make sure you understand how to use the provided eslint and prettier configurations, both in your editor, and from a terminal. -For TypeScript there is a powerful tool called [ESLint](https://typescript-eslint.io/). It's a static analysis tool that can help you improve dramatically the readability and maintainability of your code. There are ready to use ESLint configurations that you can reference in your projects: +### Prefer mixed-case names -- [ESLint Config Airbnb](https://www.npmjs.com/package/eslint-config-airbnb-typescript) - Airbnb style guide +This goes for variables, functions, classes, etc. Avoid snake_case, "dashed-case" or ALL_CAPS where possible. -- [ESLint Base Style Config](https://www.npmjs.com/package/eslint-plugin-base-style-config) - a Set of Essential ESLint rules for JS, TS and React +Prefer using `PascalCase` for class, interface, type and namespace names. -- [ESLint + Prettier](https://www.npmjs.com/package/eslint-config-prettier) - lint rules for [Prettier](https://github.com/prettier/prettier) code formatter - -Refer also to this great [TypeScript StyleGuide and Coding Conventions](https://basarat.gitbook.io/typescript/styleguide) source. - -### Migrating from TSLint to ESLint - -If you are looking for help in migrating from TSLint to ESLint, you can check out this project: - -### Use consistent capitalization - -Capitalization tells you a lot about your variables, functions, etc. These rules are subjective, so your team can choose whatever they want. The point is, no matter what you all choose, just *be consistent*. +Prefer using `camelCase` for variables, constants, functions and class members. **Bad:** ```ts -const DAYS_IN_WEEK = 7; -const daysInMonth = 30; +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']; +const songs = ['Back In Black', 'Stairway to Heaven', 'Hey Jude'] +const Artists = ['ACDC', 'Led Zeppelin', 'The Beatles'] function eraseDatabase() {} function restore_database() {} -type animal = { /* ... */ } -type Container = { /* ... */ } +type animal = { + /* ... */ +} +type Container = { + /* ... */ +} ``` **Good:** ```ts -const DAYS_IN_WEEK = 7; -const DAYS_IN_MONTH = 30; +const daysInMonth = 7 +const daysInMonth = 30 -const SONGS = ['Back In Black', 'Stairway to Heaven', 'Hey Jude']; -const ARTISTS = ['ACDC', 'Led Zeppelin', 'The Beatles']; +const songs = ['Back In Black', 'Stairway to Heaven', 'Hey Jude'] +const artists = ['ACDC', 'Led Zeppelin', 'The Beatles'] -const discography = getArtistDiscography('ACDC'); -const beatlesSongs = SONGS.filter((song) => isBeatlesSong(song)); +const discography = getArtistDiscography('ACDC') +const beatlesSongs = songs.filter((song) => isBeatlesSong(song)) function eraseDatabase() {} function restoreDatabase() {} -type Animal = { /* ... */ } -type Container = { /* ... */ } +type Animal = { + /* ... */ +} +type Container = { + /* ... */ +} ``` -Prefer using `PascalCase` for class, interface, type and namespace names. -Prefer using `camelCase` for variables, functions and class members. -Prefer using capitalized `SNAKE_CASE` for constants. - **[⬆ back to top](#table-of-contents)** ### Function callers and callees should be close If a function calls another, keep those functions vertically close in the source file. Ideally, keep the caller right above the callee. -We tend to read code from top-to-bottom, like a newspaper. Because of this, make your code read that way. + +We tend to read code from top-to-bottom, like a newspaper. It is helpful to make our code read that way, too! + +If this seems difficult, it is possible that a module or class may be too large. **Bad:** ```ts class PerformanceReview { - constructor(private readonly employee: Employee) { - } + constructor(private readonly employee: Employee) {} private lookupPeers() { - return db.lookup(this.employee.id, 'peers'); + return db.lookup(this.employee.id, 'peers') } private lookupManager() { - return db.lookup(this.employee, 'manager'); + return db.lookup(this.employee, 'manager') } private getPeerReviews() { - const peers = this.lookupPeers(); + const peers = this.lookupPeers() // ... } review() { - this.getPeerReviews(); - this.getManagerReview(); - this.getSelfReview(); + this.getPeerReviews() + this.getManagerReview() + this.getSelfReview() // ... } private getManagerReview() { - const manager = this.lookupManager(); + const manager = this.lookupManager() } private getSelfReview() { @@ -2628,40 +2636,39 @@ class PerformanceReview { } } -const review = new PerformanceReview(employee); -review.review(); +const review = new PerformanceReview(employee) +review.review() ``` **Good:** ```ts class PerformanceReview { - constructor(private readonly employee: Employee) { - } + constructor(private readonly employee: Employee) {} review() { - this.getPeerReviews(); - this.getManagerReview(); - this.getSelfReview(); + this.getPeerReviews() + this.getManagerReview() + this.getSelfReview() // ... } private getPeerReviews() { - const peers = this.lookupPeers(); + const peers = this.lookupPeers() // ... } private lookupPeers() { - return db.lookup(this.employee.id, 'peers'); + return db.lookup(this.employee.id, 'peers') } private getManagerReview() { - const manager = this.lookupManager(); + const manager = this.lookupManager() } private lookupManager() { - return db.lookup(this.employee, 'manager'); + return db.lookup(this.employee, 'manager') } private getSelfReview() { @@ -2669,123 +2676,49 @@ class PerformanceReview { } } -const review = new PerformanceReview(employee); -review.review(); +const review = new PerformanceReview(employee) +review.review() ``` **[⬆ back to top](#table-of-contents)** ### Organize imports -With clean and easy to read import statements you can quickly see the dependencies of current code. Make sure you apply following good practices for `import` statements: - -- Import statements should be alphabetized and grouped. -- Unused imports should be removed. -- Named imports must be alphabetized (i.e. `import {A, B, C} from 'foo';`) -- Import sources must be alphabetized within groups, i.e.: `import * as foo from 'a'; import * as bar from 'b';` -- Prefer using `import type` instead of `import` when importing only types from a file to avoid dependency cycles, as these imports are erased at runtime -- Groups of imports are delineated by blank lines. -- Groups must respect following order: - - Polyfills (i.e. `import 'reflect-metadata';`) - - Node builtin modules (i.e. `import fs from 'fs';`) - - external modules (i.e. `import { query } from 'itiriri';`) - - internal modules (i.e `import { UserService } from 'src/services/userService';`) - - modules from a parent directory (i.e. `import foo from '../foo'; import qux from '../../foo/qux';`) - - modules from the same or a sibling's directory (i.e. `import bar from './bar'; import baz from './bar/baz';`) - -**Bad:** - -```ts -import { TypeDefinition } from '../types/typeDefinition'; -import { AttributeTypes } from '../model/attribute'; -import { Customer, Credentials } from '../model/types'; -import { ApiCredentials, Adapters } from './common/api/authorization'; -import fs from 'fs'; -import { ConfigPlugin } from './plugins/config/configPlugin'; -import { BindingScopeEnum, Container } from 'inversify'; -import 'reflect-metadata'; -``` - -**Good:** - -```ts -import 'reflect-metadata'; - -import fs from 'fs'; -import { BindingScopeEnum, Container } from 'inversify'; - -import { AttributeTypes } from '../model/attribute'; -import { TypeDefinition } from '../types/typeDefinition'; -import type { Customer, Credentials } from '../model/types'; - -import { ApiCredentials, Adapters } from './common/api/authorization'; -import { ConfigPlugin } from './plugins/config/configPlugin'; -``` - -**[⬆ back to top](#table-of-contents)** - -### Use typescript aliases - -Create prettier imports by defining the paths and baseUrl properties in the compilerOptions section in the `tsconfig.json` - -This will avoid long relative paths when doing imports. - -**Bad:** - -```ts -import { UserService } from '../../../services/UserService'; -``` - -**Good:** - -```ts -import { UserService } from '@services/UserService'; -``` - -```js -// tsconfig.json -... - "compilerOptions": { - ... - "baseUrl": "src", - "paths": { - "@services": ["services/*"] - } - ... - } -... -``` +Use the configured eslint plugin to automatically organize imports. **[⬆ back to top](#table-of-contents)** ## Comments -The use of a comments is an indication of failure to express without them. Code should be the only source of truth. - -> Don’t comment bad code—rewrite it. -> — *Brian W. Kernighan and P. J. Plaugher* +The use of a comments is an indication of failure to express meaning in code. + +> Don’t comment bad code—rewrite it. +> — _Brian W. Kernighan and P. J. Plaugher_ ### Prefer self explanatory code instead of comments -Comments are an apology, not a requirement. Good code *mostly* documents itself. +Comments are an apology, not a requirement. Good code _mostly_ documents itself. **Bad:** ```ts // Check if subscription is active. -if (subscription.endDate > Date.now) { } +if (subscription.endDate > Date.now) { +} ``` **Good:** ```ts -const isSubscriptionActive = subscription.endDate > Date.now; -if (isSubscriptionActive) { /* ... */ } +const isSubscriptionActive = subscription.endDate > Date.now +if (isSubscriptionActive) { + /* ... */ +} ``` **[⬆ back to top](#table-of-contents)** -### Don't leave commented out code in your codebase +### Don't leave commented code in your codebase Version control exists for a reason. Leave old code in your history. @@ -2793,8 +2726,8 @@ Version control exists for a reason. Leave old code in your history. ```ts type User = { - name: string; - email: string; + name: string + email: string // age: number; // jobPosition: string; } @@ -2804,8 +2737,8 @@ type User = { ```ts type User = { - name: string; - email: string; + name: string + email: string } ``` @@ -2825,7 +2758,7 @@ Remember, use version control! There's no need for dead code, commented code, an * 2015-03-14: Implemented combine (JR) */ function combine(a: number, b: number): number { - return a + b; + return a + b } ``` @@ -2833,7 +2766,7 @@ function combine(a: number, b: number): number { ```ts function combine(a: number, b: number): number { - return a + b; + return a + b } ``` @@ -2841,7 +2774,7 @@ function combine(a: number, b: number): number { ### Avoid positional markers -They usually just add noise. Let the functions and variable names along with the proper indentation and formatting give the visual structure to your code. +They usually just add noise. Let the functions and variable names along with the proper indentation and formatting give the visual structure to your code. Most IDE support code folding feature that allows you to collapse/expand blocks of code (see Visual Studio Code [folding regions](https://code.visualstudio.com/updates/v1_17#_folding-regions)). **Bad:** @@ -2851,10 +2784,10 @@ Most IDE support code folding feature that allows you to collapse/expand blocks // Client class //////////////////////////////////////////////////////////////////////////////// class Client { - id: number; - name: string; - address: Address; - contact: Contact; + id: number + name: string + address: Address + contact: Contact //////////////////////////////////////////////////////////////////////////////// // public methods @@ -2873,17 +2806,17 @@ class Client { private describeContact(): string { // ... } -}; +} ``` **Good:** ```ts class Client { - id: number; - name: string; - address: Address; - contact: Contact; + id: number + name: string + address: Address + contact: Contact public describe(): string { // ... @@ -2896,57 +2829,17 @@ class Client { private describeContact(): string { // ... } -}; +} ``` **[⬆ back to top](#table-of-contents)** ### TODO comments -When you find yourself that you need to leave notes in the code for some later improvements, -do that using `// TODO` comments. Most IDE have special support for those kinds of comments so that -you can quickly go over the entire list of todos. +Many code editors have special support for TODO comments. They can be a convenient way to remind ourselves to finish something before we submit our code for review. -Keep in mind however that a *TODO* comment is not an excuse for bad code. +However, TODO comments should not be checked into version control, and should be removed before merging feature branches. -**Bad:** - -```ts -function getActiveSubscriptions(): Promise { - // ensure `dueDate` is indexed. - return db.subscriptions.find({ dueDate: { $lte: new Date() } }); -} -``` - -**Good:** - -```ts -function getActiveSubscriptions(): Promise { - // TODO: ensure `dueDate` is indexed. - return db.subscriptions.find({ dueDate: { $lte: new Date() } }); -} -``` - -**[⬆ back to top](#table-of-contents)** - -## Translations - -This is also available in other languages: -- ![br](https://raw.githubusercontent.com/gosquared/flags/master/flags/flags/shiny/24/Brazil.png) **Brazilian Portuguese**: [vitorfreitas/clean-code-typescript](https://github.com/vitorfreitas/clean-code-typescript) -- ![cn](https://raw.githubusercontent.com/gosquared/flags/master/flags/flags/shiny/24/China.png) **Chinese**: - - [beginor/clean-code-typescript](https://github.com/beginor/clean-code-typescript) - - [pipiliang/clean-code-typescript](https://github.com/pipiliang/clean-code-typescript) -- ![fr](https://raw.githubusercontent.com/gosquared/flags/master/flags/flags/shiny/24/France.png) **French**: [ralflorent/clean-code-typescript](https://github.com/ralflorent/clean-code-typescript) -- ![de](https://raw.githubusercontent.com/gosquared/flags/master/flags/flags/shiny/24/Germany.png) **German**: [mheob/clean-code-typescript](https://github.com/mheob/clean-code-typescript) -- ![ja](https://raw.githubusercontent.com/gosquared/flags/master/flags/flags/shiny/24/Japan.png) **Japanese**: [MSakamaki/clean-code-typescript](https://github.com/MSakamaki/clean-code-typescript) -- ![ko](https://raw.githubusercontent.com/gosquared/flags/master/flags/flags/shiny/24/South-Korea.png) **Korean**: [738/clean-code-typescript](https://github.com/738/clean-code-typescript) -- ![ru](https://raw.githubusercontent.com/gosquared/flags/master/flags/flags/shiny/24/Russia.png) **Russian**: [Real001/clean-code-typescript](https://github.com/Real001/clean-code-typescript) -- ![es](https://raw.githubusercontent.com/gosquared/flags/master/flags/flags/shiny/24/Spain.png) **Spanish**: [JoseDeFreitas/clean-code-typescript](https://github.com/JoseDeFreitas/clean-code-typescript) -- ![tr](https://raw.githubusercontent.com/gosquared/flags/master/flags/flags/shiny/24/Turkey.png) **Turkish**: [ozanhonamlioglu/clean-code-typescript](https://github.com/ozanhonamlioglu/clean-code-typescript) -- ![vi](https://raw.githubusercontent.com/gosquared/flags/master/flags/flags/shiny/24/Vietnam.png) **Vietnamese**: [hoangsetup/clean-code-typescript](https://github.com/hoangsetup/clean-code-typescript) - -References will be added once translations are completed. -Check this [discussion](https://github.com/labs42io/clean-code-typescript/issues/15) for more details and progress. -You can make an indispensable contribution to *Clean Code* community by translating this to your language. +If additional work is intended after a feature branch is merged, create a ticket so that the work can be estimated and tracked. It is too easy to forget about tasks that only exist as TODO comments. **[⬆ back to top](#table-of-contents)** diff --git a/prettier.config.js b/prettier.config.js new file mode 100644 index 0000000..5cf4ef7 --- /dev/null +++ b/prettier.config.js @@ -0,0 +1,20 @@ +module.exports = { + arrowParens: 'always', + bracketSameLine: true, + bracketSpacing: true, + embeddedLanguageFormatting: 'auto', + endOfLine: 'lf', + htmlWhitespaceSensitivity: 'css', + insertPragma: false, + jsxSingleQuote: false, + printWidth: 80, + proseWrap: 'preserve', + quoteProps: 'as-needed', + requirePragma: false, + semi: false, + singleQuote: true, + tabWidth: 2, + trailingComma: 'none', + useTabs: false, + vueIndentScriptAndStyle: false +}