diff --git a/.editorconfig b/.editorconfig new file mode 100644 index 00000000..14869adf --- /dev/null +++ b/.editorconfig @@ -0,0 +1,10 @@ +root = true + +[*] +end_of_line = lf +insert_final_newline = true + +[*.{js,json,md}] +charset = utf-8 +indent_style = space +indent_size = 2 diff --git a/.eslintrc.json b/.eslintrc.json new file mode 100644 index 00000000..9acea47c --- /dev/null +++ b/.eslintrc.json @@ -0,0 +1,24 @@ +{ + "parserOptions": { + "ecmaVersion": "2017", + "sourceType": "module" + }, + "rules": { + "eqeqeq": 2, + "no-console": 0, + "quotes": [2, "single"], + "linebreak-style": [2, "unix"], + "semi": [2, "always"], + "no-unused-vars": 0, + "react/prop-types": 0 + }, + "globals": { + "define": true + }, + "env": { + "es6": true, + "browser": true, + "commonjs": true + }, + "extends": ["eslint:recommended"] +} diff --git a/.gitignore b/.gitignore new file mode 100644 index 00000000..c2658d7d --- /dev/null +++ b/.gitignore @@ -0,0 +1 @@ +node_modules/ diff --git a/README.md b/README.md index f8a58b56..5dea1fbe 100644 --- a/README.md +++ b/README.md @@ -1,18 +1,19 @@ -# clean-code-javascript +# clean-code-javascript (Davids func remix) ## 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. [Testing](#testing) - 7. [Concurrency](#concurrency) - 8. [Error Handling](#error-handling) - 9. [Formatting](#formatting) - 10. [Comments](#comments) - 11. [Translation](#translation) + 5. [SOLID](#solid) + 6. [Classes](#classes) + 7. [Testing](#testing) + 8. [Concurrency](#concurrency) + 9. [Error Handling](#error-handling) + 10. [Formatting](#formatting) + 11. [Comments](#comments) + 12. [Translation](#translation) ## Introduction ![Humorous image of software quality estimation as a count of how many expletives @@ -395,7 +396,7 @@ 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 +principles laid out in the *SOLID* 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. @@ -633,8 +634,13 @@ extend JavaScript's native Array method to have a `diff` method that could show the difference between two arrays? You could write your new function to the `Array.prototype`, but it could clash with another library that tried to do the same thing. What if that other library was just using `diff` to find -the difference between the first and last elements of an array? This is why it -would be much better to just use ES2015/ES6 classes and simply extend the `Array` global. +the difference between the first and last elements of an array? + +One option is to use ES2015/ES6 classes. Classes are useful when you need state. +Consider writing a simple function if state is not an issue. Also, when extending an object, +features will be limited to instances of that subclass only. Another drawback is +(especially when extending built in globals like Array) that instances must be +created using the 'new' keyword. A better option is to write a stateless function. **Bad:** ```javascript @@ -646,11 +652,9 @@ Array.prototype.diff = function diff(comparisonArray) { **Good:** ```javascript -class SuperArray extends Array { - diff(comparisonArray) { - const hash = new Set(comparisonArray); - return this.filter(elem => !hash.has(elem)); - } +function diff(first, second) { + const hash = new Set(second); + return first.filter(elem => !hash.has(elem)); } ``` **[⬆ back to top](#table-of-contents)** @@ -769,48 +773,54 @@ just do one thing. **Bad:** ```javascript -class Airplane { +function makeAirplane(config) { // ... - getCruisingAltitude() { - switch (this.type) { + function getCruisingAltitude() { + switch (config.type) { case '777': - return this.getMaxAltitude() - this.getPassengerCount(); + return getMaxAltitude() - getPassengerCount(); case 'Air Force One': - return this.getMaxAltitude(); + return getMaxAltitude(); case 'Cessna': - return this.getMaxAltitude() - this.getFuelExpenditure(); + return getMaxAltitude() - getFuelExpenditure(); } } + // ... } ``` **Good:** ```javascript -class Airplane { +function makeAirplane(config) { // ... } -class Boeing777 extends Airplane { - // ... - getCruisingAltitude() { - return this.getMaxAltitude() - this.getPassengerCount(); - } +function makeBoeing777(config) { + const plane = makeAirplane(config); + + plane.getCruisingAltitude = () => plane.getMaxAltitude() - plane.getPassengerCount(); + + return plane; } -class AirForceOne extends Airplane { - // ... - getCruisingAltitude() { - return this.getMaxAltitude(); - } +function makeAirForceOne(config) { + const plane = makeAirplane(config); + + plane.getCruisingAltitude = () => plane.getMaxAltitude(); + + return plane; } -class Cessna extends Airplane { - // ... - getCruisingAltitude() { - return this.getMaxAltitude() - this.getFuelExpenditure(); - } +function makeCessna(config) { + const plane = makeAirplane(config); + + plane.getCruisingAltitude = () => plane.getMaxAltitude() - plane.getFuelExpenditure(); + + return plane; } ``` +[full example](examples/avoid-conditionals-GOOD.js) + **[⬆ back to top](#table-of-contents)** ### Avoid type-checking (part 1) @@ -982,6 +992,8 @@ function makeBankAccount() { const account = makeBankAccount(); account.setBalance(100); ``` +[full example](examples/use-getters-and-setters-GOOD.js) + **[⬆ back to top](#table-of-contents)** @@ -1023,14 +1035,13 @@ console.log(`Employee name: ${employee.getName()}`); // Employee name: John Doe **[⬆ back to top](#table-of-contents)** -## **Classes** +## **SOLID** ### 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 +It's tempting to jam-pack a class or a module 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 times 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, +that your class or module won't be conceptually cohesive and it will give it many reasons +to change. Minimizing the amount of times you need to change code is important. +It's important because if too much functionality is in one class or module and you modify a piece of it, it can be difficult to understand how that will affect other dependent modules in your codebase. @@ -1042,12 +1053,12 @@ class UserSettings { } changeSettings(settings) { - if (this.verifyCredentials()) { + if (this.hasValidCredentials()) { // ... } } - verifyCredentials() { + hasValidCredentials() { // ... } } @@ -1055,28 +1066,20 @@ class UserSettings { **Good:** ```javascript -class UserAuth { - constructor(user) { - this.user = user; - } - - verifyCredentials() { +function hasValidCredentials(user) { // ... - } } +export default hasValidCredentials; +``` -class UserSettings { - constructor(user) { - this.user = user; - this.auth = new UserAuth(user); - } +```javascript +import hasValidCredentials from 'userAuth.js'; - changeSettings(settings) { - if (this.auth.verifyCredentials()) { - // ... +function changeSettings(user, settings) { + if (hasValidCredentials(user)) { + // ... } - } } ``` **[⬆ back to top](#table-of-contents)** @@ -1089,16 +1092,14 @@ add new functionalities without changing existing code. **Bad:** ```javascript -class AjaxAdapter extends Adapter { +class AjaxAdapter { constructor() { - super(); this.name = 'ajaxAdapter'; } } -class NodeAdapter extends Adapter { +class NodeAdapter { constructor() { - super(); this.name = 'nodeAdapter'; } } @@ -1113,7 +1114,7 @@ class HttpRequester { return makeAjaxCall(url).then((response) => { // transform response and return }); - } else if (this.adapter.name === 'httpNodeAdapter') { + } else if (this.adapter.name === 'nodeAdapter') { return makeHttpCall(url).then((response) => { // transform response and return }); @@ -1132,40 +1133,40 @@ function makeHttpCall(url) { **Good:** ```javascript -class AjaxAdapter extends Adapter { - constructor() { - super(); - this.name = 'ajaxAdapter'; +function makeAjaxAdapter() { + function request(url) { + // ... make an http request, do AJAX work and return promise } - request(url) { - // request and return promise - } + return { + request + }; } -class NodeAdapter extends Adapter { - constructor() { - super(); - this.name = 'nodeAdapter'; +function makeNodeAdapter() { + function request(url) { + // ... make an http request, do NODE work and return promise } - request(url) { - // request and return promise - } + return { + request + }; } -class HttpRequester { - constructor(adapter) { - this.adapter = adapter; - } - - fetch(url) { - return this.adapter.request(url).then((response) => { +function makeHttpRequester(adapter) { + function fetch(url) { + return adapter.request(url).then((response) => { // transform response and return }); } + + return { + fetch, + }; } ``` +[full example](examples/open-closed-principle-GOOD.js) + **[⬆ back to top](#table-of-contents)** @@ -1239,122 +1240,137 @@ renderLargeRectangles(rectangles); **Good:** ```javascript -class Shape { - setColor(color) { +function makeShape() { + function setColor(color) { // ... } - render(area) { + function render(area) { // ... } + + return { + setColor, + render + }; } -class Rectangle extends Shape { - constructor(width, height) { - super(); - this.width = width; - this.height = height; - } +function makeRectangle(width, height) { + let shape = makeShape(); + shape.getArea = () => width * height; - getArea() { - return this.width * this.height; - } + return shape; } -class Square extends Shape { - constructor(length) { - super(); - this.length = length; - } +function makeSquare(length) { + let shape = makeShape(); + shape.getArea = () => length * length; - getArea() { - return this.length * this.length; - } + return shape; } function renderLargeShapes(shapes) { 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)]; +const shapes = [makeRectangle(3, 4), makeRectangle(4, 5), makeSquare(4)]; renderLargeShapes(shapes); ``` **[⬆ back to top](#table-of-contents)** ### Interface Segregation Principle (ISP) -JavaScript doesn't have interfaces so this principle doesn't apply as strictly -as others. However, it's important and relevant even with JavaScript's lack of -type system. - -ISP states that "Clients should not be forced to depend upon interfaces that -they do not use." Interfaces are implicit contracts in JavaScript because of -duck typing. - -A good example to look at that demonstrates this principle in JavaScript is for -classes that require large settings objects. Not requiring clients to setup -huge amounts of options is beneficial, because most of the time they won't need -all of the settings. Making them optional helps prevent having a "fat interface". +ISP states that "___Clients should not be forced to depend upon interfaces that +they do not use.___" In JavaScript, an interface is the contract of an object (i.e. the public properties and methods). In the "Bad" example below, the `Feedback` class inherits everything from the `Message` class, but only a couple of the features will +make sense to the subclass. **Bad:** ```javascript -class DOMTraverser { - constructor(settings) { - this.settings = settings; - this.setup(); +class Message { + constructor() { + this._feedback = []; + this._rating = 0; } - setup() { - this.rootNode = this.settings.rootNode; - this.animationModule.setup(); + addFeedback(message) { + this._feedback.push(message); } - traverse() { - // ... + getFeedback() { + return this._feedback; + } + + // this method makes no sense to the Feedback subclass + getRating() { + return this._rating; + } + + // this method makes no sense to the Feedback subclass + setRating(stars) { + this._rating = stars; + } + + // this method makes no sense to the Feedback subclass + postMessage(message) { + console.log(message); } } -const $ = new DOMTraverser({ - rootNode: document.getElementsByTagName('body'), - animationModule() {} // Most of the time, we won't need to animate when traversing. - // ... -}); +class MessageForFeedback extends Message { + share(message) { + return super.addFeedback(message); + } + get() { + return super.getFeedback(); + } +} ``` **Good:** ```javascript -class DOMTraverser { - constructor(settings) { - this.settings = settings; - this.options = settings.options; - this.setup(); - } +// several "interfaces", separated by feature +function postMessage(message) { + console.log(message); +} - setup() { - this.rootNode = this.settings.rootNode; - this.setupOptions(); - } +function makeRating() { + let stars = 0; - setupOptions() { - if (this.options.animationModule) { - // ... - } - } + const get = () => stars; + const set = (numberOfStars) => stars = numberOfStars; - traverse() { - // ... - } + return { + get, + set + }; } -const $ = new DOMTraverser({ - rootNode: document.getElementsByTagName('body'), - options: { - animationModule() {} - } -}); +function makeFeedback() { + const messages = []; + + const get = () => messages; + const add = (message) => messages.push(message); + + return { + get, + add + }; +} +``` + +```javascript +// example usage: different "clients", composed with features that makes sense +const feedback = makeFeedback(); +feedback.add('Good job!'); + +const ratings = makeRating(); +ratings.set(5); + +postMessage(feedback.get()); +postMessage(ratings.get()); ``` **[⬆ back to top](#table-of-contents)** @@ -1453,6 +1469,17 @@ inventoryTracker.requestItems(); ``` **[⬆ back to top](#table-of-contents)** + +## **Classes** +Classes are useful when you need state. Consider writing a function if state is not an issue. +Avoid creating potentially complex hierarchies by extending classes, +unless you are using a library like React and need a component using state (by extending React.Component). +Another thing to have in mind is, when extending classes, features will be limited to instances +of that subclass only. When extending built in globals like Array, instances must be created using +the 'new' keyword and cannot use shorthand syntax like `const arr = [];`. +A better option could be to write a stateless function. + + ### 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 @@ -1654,25 +1681,24 @@ class EmployeeTaxData extends Employee { **Good:** ```javascript -class EmployeeTaxData { - constructor(ssn, salary) { - this.ssn = ssn; - this.salary = salary; - } - - // ... +function makeTaxData(ssn, salary) { + return { + ssn, + salary + }; } -class Employee { - constructor(name, email) { - this.name = name; - this.email = email; - } +function makeEmployee(name, email) { + const obj = { + name, + email + }; - setTaxData(ssn, salary) { - this.taxData = new EmployeeTaxData(ssn, salary); - } - // ... + obj.setTaxData = function (ssn, salary) { + obj.taxData = makeTaxData(ssn, salary); + }; + + return obj; } ``` **[⬆ back to top](#table-of-contents)** diff --git a/examples/avoid-conditionals-BAD.js b/examples/avoid-conditionals-BAD.js new file mode 100644 index 00000000..cbb4ed77 --- /dev/null +++ b/examples/avoid-conditionals-BAD.js @@ -0,0 +1,31 @@ +function makeAirplane(config) { + function getMaxAltitude() { + return config.altitude; + } + + function getPassengerCount() { + return config.passengers; + } + + function getFuelExpenditure() { + return config.fuelExpenditure; + } + + function getCruisingAltitude() { + switch (config.type) { + case '777': + return getMaxAltitude() - getPassengerCount(); + case 'Air Force One': + return getMaxAltitude(); + case 'Cessna': + return getMaxAltitude() - getFuelExpenditure(); + } + } + + return { + getMaxAltitude, + getPassengerCount, + getFuelExpenditure, + getCruisingAltitude + }; +} diff --git a/examples/avoid-conditionals-GOOD.js b/examples/avoid-conditionals-GOOD.js new file mode 100644 index 00000000..a7cc48e9 --- /dev/null +++ b/examples/avoid-conditionals-GOOD.js @@ -0,0 +1,58 @@ +function makeAirplane(config) { + function getMaxAltitude() { + return config.altitude; + } + + function getPassengerCount() { + return config.passengers; + } + + function getFuelExpenditure() { + return config.fuelExpenditure; + } + + function getCruisingAltitude() { + return config.cruisingAltitude; + } + + return { + getMaxAltitude, + getPassengerCount, + getFuelExpenditure, + getCruisingAltitude + }; +} + +function makeBoeing777(config) { + const plane = makeAirplane(config); + + plane.getCruisingAltitude = () => plane.getMaxAltitude() - plane.getPassengerCount(); + + return plane; +} + +function makeAirForceOne(config) { + const plane = makeAirplane(config); + + plane.getCruisingAltitude = () => plane.getMaxAltitude(); + + return plane; +} + +function makeCessna(config) { + const plane = makeAirplane(config); + + plane.getCruisingAltitude = () => plane.getMaxAltitude() - plane.getFuelExpenditure(); + + return plane; +} + +// example usage +const config = { + altitude: 1000, + passengers: 200, + fuelExpenditure: 10, + cruisingAltitude: 800 +}; + +const cessna = makeCessna(config); diff --git a/examples/interface-segregation-principle-GOOD.js b/examples/interface-segregation-principle-GOOD.js new file mode 100644 index 00000000..fcec5ca0 --- /dev/null +++ b/examples/interface-segregation-principle-GOOD.js @@ -0,0 +1,38 @@ +// several "interfaces", separated by feature +function postMessage(message) { + console.log(message); +} + +function makeRating() { + let stars = 0; + + const get = () => stars; + const set = (numberOfStars) => stars = numberOfStars; + + return { + get, + set + }; +} + +function makeFeedback() { + const messages = []; + + const get = () => messages; + const add = (message) => messages.push(message); + + return { + get, + add + }; +} + +// example usage: different "clients", composed with features that makes sense +const feedback = makeFeedback(); +feedback.add('Good job!'); + +const ratings = makeRating(); +ratings.set(5); + +postMessage(feedback.get()); +postMessage(ratings.get()); diff --git a/examples/interface-segregation-principle-class-version-BAD.js b/examples/interface-segregation-principle-class-version-BAD.js new file mode 100644 index 00000000..1ee7b050 --- /dev/null +++ b/examples/interface-segregation-principle-class-version-BAD.js @@ -0,0 +1,39 @@ +class Message { + constructor() { + this._feedback = []; + this._rating = 0; + } + + addFeedback(message) { + this._feedback.push(message); + } + + getFeedback() { + return this._feedback; + } + + // this method makes no sense to the Feedback subclass + getRating() { + return this._rating; + } + + // this method makes no sense to the Feedback subclass + setRating(stars) { + this._rating = stars; + } + + // this method makes no sense to the Feedback subclass + postMessage(message) { + console.log(message); + } +} + +class MessageForFeedback extends Message { + share(message) { + return super.addFeedback(message); + } + + get() { + return super.getFeedback(); + } +} diff --git a/examples/interface-segregation-principle-class-version-GOOD.js b/examples/interface-segregation-principle-class-version-GOOD.js new file mode 100644 index 00000000..624200d2 --- /dev/null +++ b/examples/interface-segregation-principle-class-version-GOOD.js @@ -0,0 +1,69 @@ +// "interfaces" +function postMessage(message) { + console.log(message); +} + +class Rating { + constructor() { + this._rating = 0; + } + + get() { + return this._rating; + } + + set(stars) { + this._rating = stars; + } +} + +class Feedback { + constructor() { + this._feedback = []; + } + + get() { + return this._feedback; + } + + add(message) { + this._feedback.push(message); + } +} +// "clients" +class MessageForFeedback { + constructor() { + this._feedback = new Feedback(); + } + + share(message) { + return this._feedback.add(message); + } + + get() { + return this._feedback.get(); + } +} + +class MessageForRating { + constructor() { + this._ratings = new Rating(); + } + + rate(stars) { + return this._ratings.set(stars); + } + + send() { + postMessage(this._ratings.get()); + } +} + +// example usage +const feedback = new MessageForFeedback(); +feedback.share('Good job!'); +const result = feedback.get(); + +const ratings = new MessageForRating(); +ratings.rate(5); +ratings.send(); diff --git a/examples/liskov-substitution-GOOD.js b/examples/liskov-substitution-GOOD.js new file mode 100644 index 00000000..381a85ee --- /dev/null +++ b/examples/liskov-substitution-GOOD.js @@ -0,0 +1,38 @@ +function makeShape() { + function setColor(color) { + // ... + } + + function render(area) { + console.log(area); + } + + return { + setColor, + render + }; +} + +function makeRectangle(width, height) { + let shape = makeShape(); + shape.getArea = () => width * height; + + return shape; +} + +function makeSquare(length) { + let shape = makeShape(); + shape.getArea = () => length * length; + + return shape; +} + +function renderLargeShapes(shapes) { + shapes.forEach((shape) => { + const area = shape.getArea(); + shape.render(area); + }); +} + +const shapes = [makeRectangle(3, 4), makeRectangle(4, 5), makeSquare(4)]; +renderLargeShapes(shapes); diff --git a/examples/open-closed-principle-BAD.js b/examples/open-closed-principle-BAD.js new file mode 100644 index 00000000..c6f159b2 --- /dev/null +++ b/examples/open-closed-principle-BAD.js @@ -0,0 +1,30 @@ +/*global makeAjaxCall, makeHttpCall */ +class AjaxAdapter { + constructor() { + this.name = 'ajaxAdapter'; + } +} + +class NodeAdapter { + constructor() { + this.name = 'nodeAdapter'; + } +} + +class HttpRequester { + constructor(adapter) { + this.adapter = adapter; + } + + fetch(url) { + if (this.adapter.name === 'ajaxAdapter') { + return makeAjaxCall(url).then((response) => { + // transform response and return + }); + } else if (this.adapter.name === 'nodeAdapter') { + return makeHttpCall(url).then((response) => { + // transform response and return + }); + } + } +} diff --git a/examples/open-closed-principle-GOOD.js b/examples/open-closed-principle-GOOD.js new file mode 100644 index 00000000..452c2e2c --- /dev/null +++ b/examples/open-closed-principle-GOOD.js @@ -0,0 +1,37 @@ +/*global fetch */ +function makeAjaxAdapter() { + function request(url) { + // ... make an http request, do AJAX work and return promise + } + + return { + request + }; +} + +function makeNodeAdapter() { + function request(url) { + // ... make an request, do NODE work and return promise + } + + return { + request + }; +} + +function makeHttpRequester(adapter) { + function fetch(url) { + return adapter.request(url).then((response) => { + // transform response and return + }); + } + + return { + fetch + }; +} + +// example usage +const adapter = makeNodeAdapter(); +const requester = makeHttpRequester(adapter); +requester.fetch('http://github.com').then((res) => console.log(res)); diff --git a/examples/use-getters-and-setters-BAD.js b/examples/use-getters-and-setters-BAD.js new file mode 100644 index 00000000..8a268dbf --- /dev/null +++ b/examples/use-getters-and-setters-BAD.js @@ -0,0 +1,11 @@ +function makeBankAccount() { + // ... + + return { + balance: 0, + // ... + }; +} + +const account = makeBankAccount(); +account.balance = 100; diff --git a/examples/use-getters-and-setters-GOOD.js b/examples/use-getters-and-setters-GOOD.js new file mode 100644 index 00000000..55994faf --- /dev/null +++ b/examples/use-getters-and-setters-GOOD.js @@ -0,0 +1,24 @@ +function makeBankAccount() { + // this one is private + let balance = 0; + + // a "getter", made public via the returned object below + function getBalance() { + return balance; + } + + // a "setter", made public via the returned object below + function setBalance(amount) { + // ... validate before updating the balance + balance = amount; + } + + return { + // ... + getBalance, + setBalance, + }; +} + +const account = makeBankAccount(); +account.setBalance(100); diff --git a/package.json b/package.json new file mode 100644 index 00000000..29be94e9 --- /dev/null +++ b/package.json @@ -0,0 +1,25 @@ +{ + "name": "clean-code-javascript-func-remix", + "version": "1.0.0", + "description": "Clean Code JavaScript (Davids func remix)", + "main": "index.js", + "directories": { + "example": "examples" + }, + "scripts": { + "test": "echo \"Error: no test specified\" && exit 1" + }, + "repository": { + "type": "git", + "url": "git+https://github.com/DavidVujic/clean-code-javascript.git" + }, + "author": "David Vujic", + "license": "MIT", + "bugs": { + "url": "https://github.com/DavidVujic/clean-code-javascript/issues" + }, + "homepage": "https://github.com/DavidVujic/clean-code-javascript#readme", + "devDependencies": { + "eslint": "^3.14.1" + } +}