ryanmcdermott / clean-code-javascript Goto Github PK

// It doesn't have to be prefixed with get or set to be a getter/setter

Well, how we can distinguish them from methods? With the example used there, the heading should be 'Prefer methods to simple properties and getters / setters'.

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 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 boilerplate, you can use an object if you are finding yourself needing a lot of arguments.

Unless there are multiple optional arguments it does more harm than good to pass an object with arguments.

1. Your IDE has no idea what arguments the function is expecting so hinting is a no go.
2. The user has no idea what the options are without good documentation.
3. func({foo: "bar", bar: "foo"}) looks worse than func("bar", "foo")

Zero arguments is the ideal case.

I don't think zero arguments is ideal, because it only applies to impure functions and objects or classes methods.

Actually pure functions might have zero arguments, but they would be kind of pointless right?

Since the guide favors single purpose pure functions, having zero arguments wouldn't make sense. In my opinion one argument is the ideal. And zero is just ok if the function is not pure or is a method of an object or class.

Just an opionion.

What are your thoughts on mutation testing?

Background info:
Mutation testing is a technique used to measure the quality of your tests. Code coverage only measures which code is executed during testing, mutation testing actually tests your tests. This is done by inserting bugs (mutations) into your code such as replacing > with < or >=.

After a mutation has been applied to the code, all tests belonging to that mutation are executed. If a tests fails due to mutated code, all is well. Your tests managed to detect the bug. If no tests failed, you may have an issue.

The current guide says:

the main point is to just make sure you are reaching your coverage goals before launching any feature

This does not mean that you have properly tested your application. Unfortunately there are people who temporarily disable assertions, only to forget to turn them back on again. Sometimes people even write tests without assertions (or bad ones) just for the sake of reaching their code coverage goals.

So, what are the downsides of mutation testing (in JavaScript)?

• It's (relatively) slow because tests have to be executed many, many times.
• It's hard to write a mutation testing framework for JavaScript. There are a lot of combinations of compilers, test frameworks and test runners.

Starting to translate to Korean.
Feel free to collaborate.

Check it out here.

What's the standard for Ternary Operators? I find that I use them a lot in my js code. Back in college we used to have these rules:

1. Absolutely 1 condition only
2. Atomic data in the true/false section
3. The ternary statement is in its own line, not nesting between other statements.

I wanted to state that in my opinion the "Good" part on the section on Liskov Substitution Principle is not a good interpretation.
(I'm really not sure about it, it's a feeling, so forgive me if I'm the one who is interpreting in the wrong way.)

What I think is that the "Bad" and "Good" are too different to be a refactoring example:

• they are not easily refactorable one in the other, because they are not sharing the inheritance structure
• it's not clear what problem the code tries to solve
• the principle is not even applicable in the "Good" example, because there are no instanced objects of type Shape:

  If Rectangle (S) is a subtype of Shape (T), then objects of type Shape (T) may be replaced with objects of type Rectangle (S)

• the "Good" example uses a switch (shape.constructor.name) which is very not-object-oriented:
  • it hardcodes the class name as a string in the code
  • it's not open to addition of new classes
  • it violates the Avoid conditionals principle, which suggests itself to use classes
  • it violates the Avoid type-checking (I think the Ruby world suggestion of "Tell, don't Ask" would be a good explanation of this principle!)

I'd be glad to discuss and help to build a more correct example, if anyone feels the same on my doubts.

Thanks for the great work!

This is absolutely not helpful, when you are trying to find out what kind of keys do the config object accept.

function createMenu(config) {
  // ...
}

createMenu({
  title: 'Foo',
  body: 'Bar',
  buttonText: 'Baz',
  cancellable: true
});

This however makes it perfectly clean exactly which keys are used:

function createMenu({ title, body, buttonText, cancellable }) {
  // ...
}

There are no sneaky magic properties accessed in the call chain later, since you don't have the reference to the original config object. I would highly recommend suggesting this version. Also while coding, if you have a linter, it will tell you if you have an unused prop in the destrucutured object while it will have no chance of warning you if you just take it as a regular argument.

I often find myself choosing between Don't use flags as function parameters and Remove duplicate code.

In the example for Don't use flags as function parameters we have as the suggested approach:

function createTempFile(name) {
  fs.create('./temp/' + name);
}

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

This has duplicated code (although just 1 line in this case) in the same way that the bad example for Remove duplicate code has:

function showDeveloperList(developers) {
...
    var githubLink = developer.getGithubLink();
...
}

function showManagerList(managers) {
...
    var portfolio = manager.getMBAProjects();
...
}

In the Remove duplicate code case we are suggested to instead do:

function showList(employees) {
...
    if (employee.type === 'manager') {
      portfolio = employee.getMBAProjects();
    } else {
      portfolio = employee.getGithubLink();
    }
...
}

My question is, how do you decide when to keep code DRY, or to keep functions only doing 1 thing (not multiple variations of 1 thing = many things)?

I'd love some thoughts on this. Thanks!

In JS, const is NOT immutable—it can be changed but cannot be reassigned

(See https://mathiasbynens.be/notes/es6-const for a more detailed explanation)

I've seen this confuse many new devs, so I'd like to suggest including this information. I'll make a PR with the edit if you'd like. 🎉

So I thought I'd personally share some philosophies that helped me personally distinguish when to use one over the other. Initially as a novice developer I loved convention over configuration and therefore loved inheritance, but nowadays I've come to greatly appreciate composition. However I've realized there's a very concrete reason for this:

Frameworks benefit from convention.
Libraries benefit from composition.

I found that in a framework, it's acceptable to build the assumption / expectation that fellow developers will have to eventually learn all the conventions of your framework in order to leverage it and move quickly. It is almost a form of 'environment' in which you choose to develop in, and in so doing, the conventions allow you to move quickly and create reusable, shareable modules with little to no learning curve across modules. However, this is an expensive requirement as there is a greater learning curve for new engineers joining the project / framework for them to gain what is essentially "common knowledge" of the conventions in the framework. Simply looking at code in angular, ember, and react you see properties of objects that are not quite self-explanatory on their own or you may seek functionality or behavior that requires a certain degree of knowledge of the framework in order to solve. But once this knowledge is common you can rapidly develop and focus on larger scale problems.

When developing a library, you can't expect knowledge of your conventions. Libraries are far more likely to be moved and used and added and removed. Composeable code creates a sort of self-documenting approach to someone joining the codebase or picking up the code. It becomes clearer to see how or why a library or object is behaving in a specific way because the behavior configuration is laid out before you vs being handled using hand-waving magic under the hood. All behavior is explicitly configured and created. This is handy for when a developer may only briefly interact with your code such as a library in order to achieve what they need and then will likely move on to focus on unrelated code (such as another section of their application).

TL;DR:

A possible example of inheritence / convention vs composeable / configuration is are you developing a framework vs a library.

Is the user entrenching themself completely in your codebase and expected to learn all the conventions, or is their interaction with your code only used for a brief subset of their application.

Composeable is self-documenting and can be gleamed from the surface quickly. Inheritence requires gaining knowledge of internal behavior but allows large productivity rapidly.

I don't think that the provided info about why not use switches in this section is complete. Maybe we should add info about smelly code, the strategy pattern and open/closed principles.

In this speak/presentation it is all made clear why not using switches. And it has not so much to do with functions doing on thing, but more about the open/closed principle and that with adding/removing/editing one case you just "touched" all cases in the switch and all have to be tested again.

Also he added a example using the strategy pattern to refactor the switch without violating the "open/closed principle". I like that example very much.

There's an ordered list in "Ojects and Data structures" section, but it says "Well, here's an unorganized list of reasons why" right before the list starts.

So which one is it? This could be a major issue for someone :)

https://github.com/ryanmcdermott/clean-code-javascript/blob/master/README.md#objects-and-data-structures

We should add some disclaimer in the "default arguments" section, since the example will work only if the argument passed is undefined, if the argument is null, it won't work in the second example, but it will work in the first one.

Since there are open issues with people translating the big markdown file - #55 #99 #146 ..., it would be a shame to have all the code duplicated in all of them. We could create a folder with all the snippets which could be then included by the different translation. It could be done, with

• remark - https://github.com/wooorm/remark
• remark-include - https://github.com/Qard/remark-include
• an additional transpilation step

I would like to create a PR if you are willing to include this in your repo, but it will be consist of a huge amount of changes and a lot to review. Let me know!

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.

I disagree. This simply is not true. Actually, there's ample evidence that the wrong abstraction can be far more costly than duplicate code. DRYing up code is generally a good idea to do, and for people who are new to the core concept itself it may be a good idea to remove duplication instantly, the moment it is spotted. However, allowing more evidence to accumulate before making that abstraction is usually the best approach. And in order to DRY your code, you should have the right automated tests that enable you to refactor, which brings me on to another criticism of your content as it currently stands - your testing section:

Testing is more important than shipping

I'm a massive fan of testing and TDD, but this is a poorly worded statement and not a good way to introduce the subject in my opinion. It kinda implies that you're choosing one or another. How about rewording to say something more positive (and accurate) such as "Excellent automated tests are key to constant incremental updates..." - you're saying the same thing effectively but in a more positive way that also links the connection between great tests and being able to ship often.

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.

Which should open the door to a discussion on how to write tests that scale in such a way that they help you make modifications (either via new features or via refactoring) to your code. Many people who start off with testing do it badly by over-using mocks for example, which tightly couples their tests to their implementation details, making it actually harder to keep shipping, not easier. A discussion on these techniques and an improvement in your overall description is required in my opinion.

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.

I disagree to a point here - 100% coverage can be misleading as a measure of confidence. Sure, code coverage tools are good in that they can help you spot areas of your code that are not covered by tests (more of an issue for people who are not doing TDD of course), but having 100% coverage doesn't mean your code is well tested, or even fully tested. You can have 100% code coverage and have almost nothing properly tested.

I could write a single test that covers a huge chunk of my code base but doesn't really prove anything that matters.

Tests should be written around behaviours that the business cares about, and if you're doing TDD and doing it well this should mean you basically always have 100% code coverage + you're confident that your tests will fail if the behaviours you care about no longer work as expected. This is what really matters.

This means that in addition to having a great testing framework, you also need to use a good coverage tool.

Disagree. It can be helpful, but you don't need it.

First, thanks a lot for this guide. Love it!
I just wanted to point out one little mistake.

Promises are a very clean alternative to callbacks, but ES7 brings async and await which offer an even cleaner solution. All you need is a function that is prefixed in an async keyword, and then you can write your logic imperatively without a then chain of functions. Use this if you can take advantage of ES7 features today!

Actually ES2016 (or ES7) only added two features: Array.prototype.includes and the exponentiation operator.

Async/Await is featured in ES2017 (or ES8).

You said:
Avoid legal comments in source files
That's what your LICENSE file at the top of your source tree is for

I partially disagree.
1\ You can combine multiple licences in your source tree
2\ You can not expect that everyone will take all your files in their project (so with time, the original license can be lost)
3\ When someone snoop your code on your web-based stuff in production, the LICENSE file is often absent or not reachable.

So, as author, you must be absolutely clear about your licenses intentions, especially if you want protects freedom of users of your code users. Here some clue :
https://www.gnu.org/philosophy/javascript-trap.html#AppendixA

Hi, may I translate it in Chinese and put on network？

In section Function names should say what they do you're explicitly saying that you're adding a month to a date, addMonthToDate().

In section Remove duplicate code you're showing the employees list using showList().

Shouldn't that be showEmployeesList() as well, in order to explicitly tell what will be shown, so you just have to pass the type of the employee as an argument (managers or developers in that context)?

I'm not really sure about it, which is why I wanted to discuss about. Thanks for this great work!

The 'Remove duplicate code' in section '3 - Functions' has an unnecessary misleading statement:

function showList(employees) {
    ... 
    // the following line is unnecessary
    var favoriteManagerBooks = getMBAList()
    ...
}

Some browsers (Safari mobile for example) are not accepting this resource from ECMAScript 6 yet.

In section Async/Await are even cleaner than Promises there are too many awaits, the ones on require() calls are not necessary.

I believe the code should be:

async function getCleanCodeArticle() {
  try {
    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);
  }
}

Though I would execute the require() calls in the code outside the function, in this and in all other examples.

I think Open/Closed Principle is not about ability to extend module functionality during app runtime. It's more about writing code which doesn't require from developers changing existing code during adding new functionality. I thought about this example and I think it could be slightly changed. Let's say we have class responsible for fetch data, handle response and return promise. It can be configured to use in both browser and node js environment. This could be bad example:

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

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

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

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

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

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

This is bad, because if we would like to add new adapter, we have to change httpRequester class. We can fix it this way:

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

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

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

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

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

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

What do You think about that ?

Let's say you have location and locations one next to each other, could this to lead to typing errors if you mistakenly write an extra s?

The closest I found in clean-code-javascript was Avoid Mental Mapping:

locations.forEach((location) => {
  ...
});

In the previous example, a proper editor (or its extension) could rapidly find out that location is not being used if you only used locations instead.

But what about in the following case?

function processLocationByExploringOtherLocations(location, locations) {
  ...
}

location and locations have the exact same priority for any editor, should we use something like locationsList instead?

Also, I am well aware that in this case TypeScript or flow would have recognized a typing mistake by simple checking their type. However, there are still many projects which do not use either of them.

The "Use getters and setters" section describes that it is hard to enforce the pattern, because JavaScript don't have the public and private keywords. I think this is actually an issue with JavaScript classes. With functions, properties can easily be private. Here's an example (also without the get and set keywords, that I think will cause confusion):

function myObject() {
  // this one is private
  let myPrivateProperty = 'hello';

  // a "getter", made public via the return statement below
  const getMyProperty = () => myPrivateProperty;

  // a "setter", made public via the return statement below
  const setMyProperty = (val) => myPrivateProperty = val;
  
  return {
    getMyProperty,
    setMyProperty
  };
}

In the advice about private members, what's the point of the extra IIFE over Employee definition?

var Employee = (function() {
  function Employee(name) {
    this.getName = function() {
      return name;
    };
  }

  return Employee;
}());

Since there are no extra variables in the outer function, this snippet could as well read:

function Employee(name) {
  this.getName = function() {
    return name;
  };
 }

yearMonthDay is Not meaningful name.

birthday, holiday name, that a meaningful name for a date.
yearMonthDay is just a format name and if the implementation is change a bit the name will also change.

what do you think?

Thanks :)

Well, as Sandy Metz beautifully has shown us here and here, "duplication is far cheaper than the wrong abstraction".

So be careful.

I figured out that for most complex code structures where I need nested loops due to performance, I can more easily read them once I chose to use iterators based on the variable's (object's or array's) name. I typically use the leading character as the iterator (e.g. dataset => d) and a trailing l to imply the length (e.g. dataset => dl).

While I know that functional programming is easier to read (e.g. using Object.values(dataset).forEach((chunk, c) => {}) it's hardly avoidable in timing-relevant methods and complex sorting algorithms that need performant behaviours (e.g. an update or render loop in a game engine).

Anyways, here's an example. Let me know whatcha think.

BAD example

const dataset = [{ foo: [1,2,3,4], bar: [1,2,3,4] }];

for (let i in dataset) {
    let chunk = dataset[i];
    for (let j = 0, l = chunk.length; j < l; j++) {
        console.log(chunk[j]); // you get the point
    }
}

GOOD example

const dataset = [{ foo: [1,2,3,4], bar: [1,2,3,4] }];

for (let d in dataset) {
    let chunk = dataset[d];
    for (let c = 0, cl = chunk.length; c < cl; c++) {
        console.log(chunk[c]); // you get the point
    }
}

Because:

a) You already have "Use default arguments instead of short circuiting", which is a better approach (if supported), and

b) I have seen bugs introduced by writing a short circuit when the function is supposed to accept falsy values:

function double(x) {
   var value = x || -1;
   return value * 2;
}

double(0);  // returns -2, should return 0

As per:
#1 (comment)

Do you think that can be good to include something about magic strings?

Bad:

let posts = getPostsFromCategory('sports');

Good:

const categories = {
  sports: 'sports',
  economy: 'economy'
};

let posts = getPostsFromCategory(categories.sports);

Hello,

I started translating this guide to pt-BR, if anyone would like to help I'm doing it at: (https://github.com/esganzerla/clean-code-javascript/blob/master/translations/pt-BR/README.md)

Thanks

The Object.assign example is currently incorrect.

function createMenu(config) {
  Object.assign(config, {
    title: 'Foo',
    body: 'Bar',
    buttonText: 'Baz',
    cancellable: true
  });
}

createMenu({ title: 'Not Foo' });

The title property will always be set to 'Foo'!

I was going to make a PR with a fix, but the approach really depends on whether the mutation of config is desirable.

How I would do it (this does not behave identically to the "bad" example):

function createMenu(config) {
  // makes a copy of `config`, with default values
  config = Object.assign({
    title: 'Foo',
    body: 'Bar',
    buttonText: 'Baz',
    cancellable: true
  }, config);
}

How you could do it with mutation (this is identical to the "bad" example):

function createMenu(config) {
  // mutates `config`, setting default values
  Object.assign(config, Object.assign({
    title: 'Foo',
    body: 'Bar',
    buttonText: 'Baz',
    cancellable: true
  }, config));
}

A thing I see very often is when we are changing a function's object variables. Those variables are passed by reference and this is a side effect. I would propose of adding it to the Side Effects section.

I think this part of the guide falls into the trap of thinking that the Law of Demeter (LOD) is simply about multiple method calls or the "2/3 dot rule".

It's more about "not talking to strangers". The paper "The Paperboy, The Wallet and The Law Of Demeter" sums it up nicely.

If your functions return the same type, it's totally fine. In your example, you're using the builder pattern, which always returns the same type to allow further operations.

Another example of not violating LOD would be a functional pipeline transforming an array:

const list = [1, 2, 3, 4, 5]

list.map(...).reduce(...).filter(...).concat(...) // not a violation of LOD

Hope this helps!

Most likely there are many better solutions for this, but here it is my quick attempt.

Usage: md.extract-js.js markdown-file.md.

Extracted .js files will be created in the markdown-file.md.extracted-js directory next to markdown-file.md. Each file has this name template: numberOfFragment.numberOfFirstLine-numberOfLastLine.js.

A contributor can run a linter on the whole directory or run each file with Node.js. There will be many linter errors or runtime errors due to many undeclared (or unused) variables, so some filter work will be needed.

In our case, the odd filenames are bad examples, the even filenames are good ones.

Here is a new PR from this script's work.

Use String literals instead:

Bad:

console.log(
  "string text line 1\n"+
  "string text line 2"
);

Good:

console.log(`
string text line 1
string text line 2
`);

The current code samples are not consistent, mix a half var and a half let (and a few const).

We may need a unified rule for the usage of const/let and apply it to the code samples, as you mentioned in the second principle Use ES6 constants when variable values do not change, or your code is not clean. 😉

I feel confused, should I use semicolon or not? what's is different between them.

Reference: dont-over-optimize

Since all examples are using ES6+, arguing about old browsers don't looks significant IMHO.
Maybe another argument should be better. What do you think?

Hi @ryanmcdermott ,

This is not an issue, How to we given the filenames is lowerCamelCase ,UpperCamelCase, file_name.js , file-name.js like that and standard format is there ?

If we request api call means how we mention the url ?

Request URL:https://serviceprovider.com/api/power-details
Request URL:https://serviceprovider.com/api/power_details
Request URL:https://serviceprovider.com/api/powerDetails
Request URL:https://serviceprovider.com/api/Powerdetails

like the above any standard ?

If we create an MVC architecture means how we use this file name

Model
- loginModel.html
- login-model.html
- login_model.html
Views
- loginView.js
- login-iew.js
- login_view.js
Controller
- loginController.js
- login-Controller.js
- login_Controller.js

If we create url means how we use ?

Please suggest standard format

Hey, thanks for the work on this - very nice!

The suggestion of using a config object to reduce the number of function arguments does not solve the underlying problem and is really just cheating - the function still takes four inputs and you still have to test every permutation, it just pretends to only take one input :)

In my opinion it's actually worse because rather than declaring the parameters explicitly it presents the consumer with an opaque method signature:

function createMenu(menuConfig)

If I'm writing code that calls this function I now have to read the full implementation of the function to find out which magic property names are expected within the config object, rather than just read the signature. Or you could add jsdoc for the function, but then you've got a maintenance problem.

There must be a better way to reduce the number of parameters, maybe currying?

From the guide: "If Haskell were an IPA then JavaScript would be an O'Douls."

I have no idea what you're talking about. JS programmers aren't required to know beer brands, let alone country-specific beer brands. A user that doesn't know where Haskell and JS stood on the functional spectrum would be pretty lost here.

In general, a programming guide should require zero knowledge outside the field of programming.

If the comparison just has to be there, here's a suggestion:
"If Haskell were a good steak, then JS would be a sandwich with slices of meat.

const [, city, zipCode] = address.match(cityZipCodeRegex);

This will error if the address doesn't match, but can be fixed with:

const [, city, zipCode] = address.match(cityZipCodeRegex) || [];

The time log shows us that maybe it's better to calculate the length and store it .

From 0.065ms to 0.035ms.

later: Sorry that I did't see In modern browsers, this is optimized.;

As the rule from title tells us, we should favor composition when we code.
So maybe it's worth to replace all examples using class with proper ones based on composition?

Let's take the "Avoid conditionals" section for instance. It shows the example of inheritance while the goal was to encourage the developer to use composition. Below you can see how the definition of different airplanes could look like (using composition):

const Airplane = {
  getName() {
    return `${this.name}`
  }
}

const AirbusA380 = { 
  name: 'Airbus A380',
  getName() {
    return  `${Airplane.getName.call(this)} of ${this.airline}`
  }
}

const Cessna = { 
  name: 'Cessna'
}


// define factory functions (airbusA380, cessna)
const airbusA380 = (state) => Object.assign(
  {},
  Airplane,
  AirbusA380,
  state
)

const cessna = (state) => Object.assign(
  {},
  Airplane,
  Cessna,
  state
)

// create instances of airplanes
const emiratesAirbus = airbusA380({
  airline: 'Emirates'
})

const privateCessnaJet = cessna()

console.log(emiratesAirbus.getName()) // prints "Airbus A380 of Emirates"
console.log(privateCessnaJet.getName()) // prints "Cessna"

What do you think about replacing all class examples with composition-based ones?

Nice work, very useful! In the section on 'method chaining', the setMake functions:

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

should it be:

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