diff --git a/README.md b/README.md index 34438930..16c038c8 100644 --- a/README.md +++ b/README.md @@ -1,3 +1,4 @@ + # clean-code-javascript ## Table of Contents @@ -11,6 +12,7 @@ 8. [Error Handling](#error-handling) 9. [Formatting](#formatting) 10. [Comments](#comments) + 11. [Translation](#translation) ## Introduction ![Humorous image of software quality estimation as a count of how many expletives @@ -47,9 +49,9 @@ improvement. Beat up the code instead! const yyyymmdstr = moment().format('YYYY/MM/DD'); ``` -**Good**: +**Good:** ```javascript -const yearMonthDay = moment().format('YYYY/MM/DD'); +const currentDate = moment().format('YYYY/MM/DD'); ``` **[⬆ back to top](#table-of-contents)** @@ -62,7 +64,7 @@ getClientData(); getCustomerRecord(); ``` -**Good**: +**Good:** ```javascript getUser(); ``` @@ -80,20 +82,16 @@ can help identify unnamed constants. **Bad:** ```javascript // What the heck is 86400000 for? -setTimeout(() => { - this.blastOff(); -}, 86400000); +setTimeout(blastOff, 86400000); ``` -**Good**: +**Good:** ```javascript // Declare them as capitalized `const` globals. const MILLISECONDS_IN_A_DAY = 86400000; -setTimeout(() => { - this.blastOff(); -}, MILLISECONDS_IN_A_DAY); +setTimeout(blastOff, MILLISECONDS_IN_A_DAY); ``` **[⬆ back to top](#table-of-contents)** @@ -106,11 +104,11 @@ const cityZipCodeRegex = /^[^,\\]+[,\\\s]+(.+?)\s*(\d{5})?$/; saveCityZipCode(address.match(cityZipCodeRegex)[1], address.match(cityZipCodeRegex)[2]); ``` -**Good**: +**Good:** ```javascript const address = 'One Infinite Loop, Cupertino 95014'; const cityZipCodeRegex = /^[^,\\]+[,\\\s]+(.+?)\s*(\d{5})?$/; -const [, city, zipCode] = address.match(cityZipCodeRegex); +const [, city, zipCode] = address.match(cityZipCodeRegex) || []; saveCityZipCode(city, zipCode); ``` **[⬆ back to top](#table-of-contents)** @@ -132,7 +130,7 @@ locations.forEach((l) => { }); ``` -**Good**: +**Good:** ```javascript const locations = ['Austin', 'New York', 'San Francisco']; locations.forEach((location) => { @@ -163,7 +161,7 @@ function paintCar(car) { } ``` -**Good**: +**Good:** ```javascript const Car = { make: 'Honda', @@ -178,6 +176,10 @@ function paintCar(car) { **[⬆ back to top](#table-of-contents)** ### Use default arguments instead of short circuiting or conditionals +Default arguments are often cleaner than short circuiting. Be aware that if you +use them, your function will only provide default values for `undefined` +arguments. Other "falsy" values such as `''`, `""`, `false`, `null`, `0`, and +`NaN`, will not be replaced by a default value. **Bad:** ```javascript @@ -188,7 +190,7 @@ function createMicrobrewery(name) { ``` -**Good**: +**Good:** ```javascript function createMicrobrewery(breweryName = 'Hipster Brew Co.') { // ... @@ -204,16 +206,28 @@ 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. -Zero arguments is the ideal case. One or two arguments is ok, and three should -be avoided. Anything more than that should be consolidated. Usually, if you have +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. -Since JavaScript allows us to make objects on the fly, without a lot of class +Since JavaScript allows you to make objects on the fly, without a lot of class boilerplate, you can use an object if you are finding yourself needing a lot of arguments. +To make it obvious what properties the function expects, you can use the ES2015/ES6 +destructuring syntax. This has a few advantages: + +1. When someone looks at the function signature, it's immediately clear what +properties are being used. +2. Destructuring also clones the specified primitive values of the argument +object passed into the function. This can help prevent side effects. Note: +objects and arrays that are destructured from the argument object are NOT +cloned. +3. Linters can warn you about unused properties, which would be impossible +without destructuring. + **Bad:** ```javascript function createMenu(title, body, buttonText, cancellable) { @@ -221,19 +235,18 @@ function createMenu(title, body, buttonText, cancellable) { } ``` -**Good**: +**Good:** ```javascript -const menuConfig = { +function createMenu({ title, body, buttonText, cancellable }) { + // ... +} + +createMenu({ title: 'Foo', body: 'Bar', buttonText: 'Baz', cancellable: true -}; - -function createMenu(config) { - // ... -} - +}); ``` **[⬆ back to top](#table-of-contents)** @@ -257,7 +270,7 @@ function emailClients(clients) { } ``` -**Good**: +**Good:** ```javascript function emailClients(clients) { clients @@ -286,7 +299,7 @@ const date = new Date(); addToDate(date, 1); ``` -**Good**: +**Good:** ```javascript function addMonthToDate(month, date) { // ... @@ -328,7 +341,7 @@ function parseBetterJSAlternative(code) { } ``` -**Good**: +**Good:** ```javascript function tokenize(code) { const REGEXES = [ @@ -366,13 +379,26 @@ function parseBetterJSAlternative(code) { **[⬆ back to top](#table-of-contents)** ### Remove duplicate code -Never ever, ever, under any circumstance, have duplicate code. There's no reason -for it and it's quite possibly the worst sin you can commit as a professional -developer. Duplicate code means there's more than one place to alter something -if you need to change some logic. JavaScript is untyped, so it makes having -generic functions quite easy. Take advantage of that! Tools like -[jsinspect](https://github.com/danielstjules/jsinspect) can help you find duplicate -code eligible for refactoring. +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. + +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! + +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 principles laid out in the *Classes* section. 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. **Bad:** ```javascript @@ -407,9 +433,9 @@ function showManagerList(managers) { } ``` -**Good**: +**Good:** ```javascript -function showList(employees) { +function showEmployeeList(employees) { employees.forEach((employee) => { const expectedSalary = employee.calculateExpectedSalary(); const experience = employee.getExperience(); @@ -453,7 +479,7 @@ function createMenu(config) { createMenu(menuConfig); ``` -**Good**: +**Good:** ```javascript const menuConfig = { title: 'Order', @@ -493,7 +519,7 @@ function createFile(name, temp) { } ``` -**Good**: +**Good:** ```javascript function createFile(name) { fs.create(name); @@ -505,7 +531,7 @@ function createTempFile(name) { ``` **[⬆ back to top](#table-of-contents)** -### Avoid Side Effects +### 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 @@ -536,7 +562,7 @@ splitIntoFirstAndLastName(); console.log(name); // ['Ryan', 'McDermott']; ``` -**Good**: +**Good:** ```javascript function splitIntoFirstAndLastName(name) { return name.split(' '); @@ -550,6 +576,55 @@ console.log(newName); // ['Ryan', 'McDermott']; ``` **[⬆ back to top](#table-of-contents)** +### Avoid Side Effects (part 2) +In JavaScript, primitives are passed by value and objects/arrays are passed by +reference. In the case of objects and arrays, if your function makes a change +in a shopping cart array, for example, by adding an item to purchase, +then any other function that uses that `cart` array will be affected by this +addition. That may be great, however it can be bad too. Let's imagine a bad +situation: + +The user clicks the "Purchase", button which calls a `purchase` function that +spawns a network request and sends the `cart` array to the server. Because +of a bad network connection, the `purchase` function has to keep retrying the +request. Now, what if in the meantime the user accidentally clicks "Add to Cart" +button on an item they don't actually want before the network request begins? +If that happens and the network request begins, then that purchase function +will send the accidentally added item because it has a reference to a shopping +cart array that the `addItemToCart` function modified by adding an unwanted +item. + +A great solution would be for the `addItemToCart` to always clone the `cart`, +edit it, and return the clone. This ensures that no other functions that are +holding onto a reference of the shopping cart will be affected by any changes. + +Two caveats to mention to this approach: + 1. There might be cases where you actually want to modify the input object, +but when you adopt this programming practice you will find that those case +are pretty rare. Most things can be refactored to have no side effects! + + 2. Cloning big objects can be very expensive in terms of performance. Luckily, +this isn't a big issue in practice because there are +[great libraries](https://facebook.github.io/immutable-js/) that allow +this kind of programming approach to be fast and not as memory intensive as +it would be for you to manually clone objects and arrays. + +**Bad:** +```javascript +const addItemToCart = (cart, item) => { + cart.push({ item, date: Date.now() }); +}; +``` + +**Good:** +```javascript +const addItemToCart = (cart, item) => { + return [...cart, { item, date : Date.now() }]; +}; +``` + +**[⬆ back to top](#table-of-contents)** + ### Don't write to global functions Polluting globals is a bad practice in JavaScript because you could clash with another library and the user of your API would be none-the-wiser until they get an @@ -610,7 +685,7 @@ for (let i = 0; i < programmerOutput.length; i++) { } ``` -**Good**: +**Good:** ```javascript const programmerOutput = [ { @@ -628,9 +703,11 @@ const programmerOutput = [ } ]; +const INITIAL_VALUE = 0; + const totalOutput = programmerOutput .map((programmer) => programmer.linesOfCode) - .reduce((acc, linesOfCode) => acc + linesOfCode, 0); + .reduce((acc, linesOfCode) => acc + linesOfCode, INITIAL_VALUE); ``` **[⬆ back to top](#table-of-contents)** @@ -643,7 +720,7 @@ if (fsm.state === 'fetching' && isEmpty(listNode)) { } ``` -**Good**: +**Good:** ```javascript function shouldShowSpinner(fsm, listNode) { return fsm.state === 'fetching' && isEmpty(listNode); @@ -668,7 +745,7 @@ if (!isDOMNodeNotPresent(node)) { } ``` -**Good**: +**Good:** ```javascript function isDOMNodePresent(node) { // ... @@ -707,7 +784,7 @@ class Airplane { } ``` -**Good**: +**Good:** ```javascript class Airplane { // ... @@ -746,14 +823,14 @@ The first thing to consider is consistent APIs. ```javascript function travelToTexas(vehicle) { if (vehicle instanceof Bicycle) { - vehicle.peddle(this.currentLocation, new Location('texas')); + vehicle.pedal(this.currentLocation, new Location('texas')); } else if (vehicle instanceof Car) { vehicle.drive(this.currentLocation, new Location('texas')); } } ``` -**Good**: +**Good:** ```javascript function travelToTexas(vehicle) { vehicle.move(this.currentLocation, new Location('texas')); @@ -784,7 +861,7 @@ function combine(val1, val2) { } ``` -**Good**: +**Good:** ```javascript function combine(val1, val2) { return val1 + val2; @@ -809,7 +886,7 @@ for (let i = 0, len = list.length; i < len; i++) { } ``` -**Good**: +**Good:** ```javascript for (let i = 0; i < list.length; i++) { // ... @@ -837,7 +914,7 @@ inventoryTracker('apples', req, 'www.inventory-awesome.io'); ``` -**Good**: +**Good:** ```javascript function newRequestModule(url) { // ... @@ -850,9 +927,7 @@ inventoryTracker('apples', req, 'www.inventory-awesome.io'); ## **Objects and Data Structures** ### Use getters and setters -JavaScript doesn't have interfaces or types so it is very hard to enforce this -pattern, because we don't have keywords like `public` and `private`. As it is, -using getters and setters to access data on objects is far better than simply +Using getters and setters to access data on objects could be better than simply looking for a property on an object. "Why?" you might ask. Well, here's an unorganized list of reasons why: @@ -861,56 +936,51 @@ to look up and change every accessor in your codebase. * Makes adding validation simple when doing a `set`. * Encapsulates the internal representation. * Easy to add logging and error handling when getting and setting. -* Inheriting this class, you can override default functionality. * You can lazy load your object's properties, let's say getting it from a server. **Bad:** ```javascript -class BankAccount { - constructor() { - this.balance = 1000; - } -} +function makeBankAccount() { + // ... -const bankAccount = new BankAccount(); + return { + balance: 0, + // ... + }; +} -// Buy shoes... -bankAccount.balance -= 100; +const account = makeBankAccount(); +account.balance = 100; ``` -**Good**: +**Good:** ```javascript -class BankAccount { - constructor(balance = 1000) { - this._balance = balance; - } +function makeBankAccount() { + // this one is private + let balance = 0; - // It doesn't have to be prefixed with `get` or `set` to be a getter/setter - set balance(amount) { - if (verifyIfAmountCanBeSetted(amount)) { - this._balance = amount; - } + // a "getter", made public via the returned object below + function getBalance() { + return balance; } - get balance() { - return this._balance; + // a "setter", made public via the returned object below + function setBalance(amount) { + // ... validate before updating the balance + balance = amount; } - verifyIfAmountCanBeSetted(val) { + return { // ... - } + getBalance, + setBalance, + }; } -const bankAccount = new BankAccount(); - -// Buy shoes... -bankAccount.balance -= shoesPrice; - -// Get balance -let balance = bankAccount.balance; - +const account = makeBankAccount(); +account.setBalance(100); ``` **[⬆ back to top](#table-of-contents)** @@ -935,15 +1005,17 @@ delete employee.name; console.log(`Employee name: ${employee.getName()}`); // Employee name: undefined ``` -**Good**: +**Good:** ```javascript -const Employee = function (name) { - this.getName = function getName() { - return name; +function makeEmployee(name) { + return { + getName() { + return name; + }, }; -}; +} -const employee = new Employee('John Doe'); +const employee = makeEmployee('John Doe'); console.log(`Employee name: ${employee.getName()}`); // Employee name: John Doe delete employee.name; console.log(`Employee name: ${employee.getName()}`); // Employee name: John Doe @@ -981,7 +1053,7 @@ class UserSettings { } ``` -**Good**: +**Good:** ```javascript class UserAuth { constructor(user) { @@ -1013,38 +1085,84 @@ class UserSettings { 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 -extend the functionality of your module without having to open up the `.js` -source code file and manually manipulate it. +add new functionalities without changing existing code. **Bad:** ```javascript -class AjaxRequester { +class AjaxAdapter extends Adapter { constructor() { - // What if we wanted another HTTP Method, like DELETE? We would have to - // open this file up and modify this and put it in manually. - this.HTTP_METHODS = ['POST', 'PUT', 'GET']; + super(); + this.name = 'ajaxAdapter'; } +} - get(url) { - // ... +class NodeAdapter extends Adapter { + constructor() { + super(); + this.name = 'nodeAdapter'; } +} +class HttpRequester { + constructor(adapter) { + this.adapter = adapter; + } + + fetch(url) { + if (this.adapter.name === 'ajaxAdapter') { + return makeAjaxCall(url).then((response) => { + // transform response and return + }); + } else if (this.adapter.name === 'httpNodeAdapter') { + return makeHttpCall(url).then((response) => { + // transform response and return + }); + } + } +} + +function makeAjaxCall(url) { + // request and return promise +} + +function makeHttpCall(url) { + // request and return promise } ``` -**Good**: +**Good:** ```javascript -class AjaxRequester { +class AjaxAdapter extends Adapter { constructor() { - this.HTTP_METHODS = ['POST', 'PUT', 'GET']; + super(); + this.name = 'ajaxAdapter'; } - get(url) { - // ... + request(url) { + // request and return promise } +} - addHTTPMethod(method) { - this.HTTP_METHODS.push(method); +class NodeAdapter extends Adapter { + constructor() { + super(); + this.name = 'nodeAdapter'; + } + + request(url) { + // request and return promise + } +} + +class HttpRequester { + constructor(adapter) { + this.adapter = adapter; + } + + fetch(url) { + return this.adapter.request(url).then((response) => { + // transform response and return + }); } } ``` @@ -1119,7 +1237,7 @@ const rectangles = [new Rectangle(), new Rectangle(), new Square()]; renderLargeRectangles(rectangles); ``` -**Good**: +**Good:** ```javascript class Shape { setColor(color) { @@ -1132,17 +1250,9 @@ class Shape { } class Rectangle extends Shape { - constructor() { + constructor(width, height) { super(); - this.width = 0; - this.height = 0; - } - - setWidth(width) { this.width = width; - } - - setHeight(height) { this.height = height; } @@ -1152,12 +1262,8 @@ class Rectangle extends Shape { } class Square extends Shape { - constructor() { + constructor(length) { super(); - this.length = 0; - } - - setLength(length) { this.length = length; } @@ -1168,21 +1274,12 @@ class Square extends Shape { function renderLargeShapes(shapes) { shapes.forEach((shape) => { - switch (shape.constructor.name) { - case 'Square': - shape.setLength(5); - break; - case 'Rectangle': - shape.setWidth(4); - shape.setHeight(5); - } - - const area = shape.getArea(); - shape.render(area); - }); -} + const area = shape.getArea(); + shape.render(area); + }); + } -const shapes = [new Rectangle(), new Rectangle(), new Square()]; +const shapes = [new Rectangle(4, 5), new Rectangle(4, 5), new Square(5)]; renderLargeShapes(shapes); ``` **[⬆ back to top](#table-of-contents)** @@ -1227,7 +1324,7 @@ const $ = new DOMTraverser({ ``` -**Good**: +**Good:** ```javascript class DOMTraverser { constructor(settings) { @@ -1314,7 +1411,7 @@ const inventoryTracker = new InventoryTracker(['apples', 'bananas']); inventoryTracker.requestItems(); ``` -**Good**: +**Good:** ```javascript class InventoryTracker { constructor(items, requester) { @@ -1359,7 +1456,7 @@ inventoryTracker.requestItems(); ### Prefer ES2015/ES6 classes over ES5 plain functions It's very difficult to get readable class inheritance, construction, and method definitions for classical ES5 classes. If you need inheritance (and be aware -that you might not), then prefer classes. However, prefer small functions over +that you might not), then prefer ES2015/ES6 classes. However, prefer small functions over classes until you find yourself needing larger and more complex objects. **Bad:** @@ -1472,7 +1569,7 @@ car.setModel('F-150'); car.save(); ``` -**Good**: +**Good:** ```javascript class Car { constructor() { @@ -1527,7 +1624,7 @@ depends on your problem at hand, but this is a decent list of when inheritance makes more sense than composition: 1. Your inheritance represents an "is-a" relationship and not a "has-a" -relationship (Animal->Human vs. User->UserDetails). +relationship (Human->Animal vs. User->UserDetails). 2. You can reuse code from the base classes (Humans can move like all animals). 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). @@ -1555,7 +1652,7 @@ class EmployeeTaxData extends Employee { } ``` -**Good**: +**Good:** ```javascript class EmployeeTaxData { constructor(ssn, salary) { @@ -1609,7 +1706,7 @@ describe('MakeMomentJSGreatAgain', () => { date = new MakeMomentJSGreatAgain('1/1/2015'); date.addDays(30); - date.shouldEqual('1/31/2015'); + assert.equal('1/31/2015', date); date = new MakeMomentJSGreatAgain('2/1/2016'); date.addDays(28); @@ -1622,7 +1719,7 @@ describe('MakeMomentJSGreatAgain', () => { }); ``` -**Good**: +**Good:** ```javascript const assert = require('assert'); @@ -1630,7 +1727,7 @@ describe('MakeMomentJSGreatAgain', () => { it('handles 30-day months', () => { const date = new MakeMomentJSGreatAgain('1/1/2015'); date.addDays(30); - date.shouldEqual('1/31/2015'); + assert.equal('1/31/2015', date); }); it('handles leap year', () => { @@ -1671,7 +1768,7 @@ require('request').get('https://en.wikipedia.org/wiki/Robert_Cecil_Martin', (req ``` -**Good**: +**Good:** ```javascript require('request-promise').get('https://en.wikipedia.org/wiki/Robert_Cecil_Martin') .then((response) => { @@ -1709,15 +1806,12 @@ require('request-promise').get('https://en.wikipedia.org/wiki/Robert_Cecil_Marti ``` -**Good**: +**Good:** ```javascript async function getCleanCodeArticle() { try { - const request = await require('request-promise'); - const response = await request.get('https://en.wikipedia.org/wiki/Robert_Cecil_Martin'); - const fileHandle = await require('fs-promise'); - - await fileHandle.writeFile('article.html', response); + const response = await require('request-promise').get('https://en.wikipedia.org/wiki/Robert_Cecil_Martin'); + await require('fs-promise').writeFile('article.html', response); console.log('File written'); } catch(err) { console.error(err); @@ -1830,7 +1924,7 @@ class animal {} class Alpaca {} ``` -**Good**: +**Good:** ```javascript const DAYS_IN_WEEK = 7; const DAYS_IN_MONTH = 30; @@ -1891,7 +1985,7 @@ const review = new PerformanceReview(user); review.perfReview(); ``` -**Good**: +**Good:** ```javascript class PerformanceReview { constructor(employee) { @@ -1957,7 +2051,7 @@ function hashIt(data) { } ``` -**Good**: +**Good:** ```javascript function hashIt(data) { @@ -1987,7 +2081,7 @@ doStuff(); // doSoMuchStuff(); ``` -**Good**: +**Good:** ```javascript doStuff(); ``` @@ -2010,7 +2104,7 @@ function combine(a, b) { } ``` -**Good**: +**Good:** ```javascript function combine(a, b) { return a + b; @@ -2040,7 +2134,7 @@ const actions = function() { }; ``` -**Good**: +**Good:** ```javascript $scope.model = { menu: 'foo', @@ -2052,3 +2146,18 @@ const actions = function() { }; ``` **[⬆ back to top](#table-of-contents)** + +## Translation + +This is also available in other languages: + + - ![br](https://raw.githubusercontent.com/gosquared/flags/master/flags/flags/shiny/24/Brazil.png) **Brazilian Portuguese**: [fesnt/clean-code-javascript](https://github.com/fesnt/clean-code-javascript) + - ![cn](https://raw.githubusercontent.com/gosquared/flags/master/flags/flags/shiny/24/China.png) **Chinese**: [alivebao/clean-code-js](https://github.com/alivebao/clean-code-js) + - ![de](https://raw.githubusercontent.com/gosquared/flags/master/flags/flags/shiny/24/Germany.png) **German**: [marcbruederlin/clean-code-javascript](https://github.com/marcbruederlin/clean-code-javascript) + - ![kr](https://raw.githubusercontent.com/gosquared/flags/master/flags/flags/shiny/24/South-Korea.png) **Korean**: [qkraudghgh/clean-code-javascript-ko](https://github.com/qkraudghgh/clean-code-javascript-ko) + - ![ru](https://raw.githubusercontent.com/gosquared/flags/master/flags/flags/shiny/24/Russia.png) **Russian**: + - [BoryaMogila/clean-code-javascript-ru/](https://github.com/BoryaMogila/clean-code-javascript-ru/) + - [maksugr/clean-code-javascript](https://github.com/maksugr/clean-code-javascript) + - ![vi](https://raw.githubusercontent.com/gosquared/flags/master/flags/flags/shiny/24/Vietnam.png) **Vietnamese**: [hienvd/clean-code-javascript/](https://github.com/hienvd/clean-code-javascript/) + +**[⬆ back to top](#table-of-contents)**