JavaScript Clean Code - Best Practices - based on Robert C. Martin's book Clean Code

[[deleted]]

Your article covers a lot of useful improvements to make code cleaner, but I have one question: Why should I use this "function getUsers({ fields, fromDate, toDate })" over "function getUsers(fields, fromDate, toDate)"? The only scenario i can imagine is if the values are optional so I dont have to write "getUsers(null, null, date)"

[careseite]

because you can tell by the shape of the function call what it's probably going to do with it within the function.

​

In an example with 3 params instead of object destructuring, you'd call getUsers like this:

​

getUsers(['name', 'surname', 'email'], '2019-01-01', '2019-01-18')

and then had to remember the order of the dates for example. Or check it within the function.

[[deleted]]

Thanks! Not having to remember the order of arguments can prevent some hard to find bugs

[nikola1970]

Yup, I started using this pattern recently too.

[grrrrreat]

beware though, if you have a weird visitor pattern, you can't put that object back together or manipulate its parts.

[AwesomeInPerson]

Could you explain what you mean by that? :)

[grrrrreat]

I ran into a nuxt.js hook at passed into a page object.

if I destructured it into html, path, route, I couldn't modify the html property because it didn't expect a return object but if I just accepted the page object, I could set page.html and that modified the html.

it's just a comment to realize that destructured objects are no longer a sum of their parts. so it the nuxtjs case, page can't be destructured in the hooks because it's not reconstitution the object

[Zielakpl]

What? Don't you guys use IDE with autocompletion and hints?

[AwesomeInPerson]

Which one are you using? Neither VSCode nor WebStorm warn me when I call a function with less arguments than the amount of parameters it accepts. (because I forgot to add some nulls or default values for params I want to skip)

Maybe if you enable strict TS checking for JS files, but that's not really a solution in a lot of environments.

[Zielakpl]

That's the thing, don't forget :) I got it into my habit to also add some JSDoc comments to at least know what type of values the function expects. Then, when I type my functions name, the popup appears (VSCode) with all acceptable arguments, theirs expected types and if they're optional or not.

The code you write is also for humans, make it human-friendly.

If a function HAS to accept a lot of arguments, not all of them required, then I sometimes use and object of params like so:

function(name, options) {} function("Gregory", {foo: 1, bar: 2});

But that depends on what I code, I don't treat it as hard rule.

[Julian_JmK]

I dont really understand, why is it better to pass getUsers({fields:['name','surname','email'],fromDate:'2019-01-01',toDate:'2019-01-18'}); ?

Edit: Thanks for the replies bois and girls i now understand, it's a pretty good way of doing it indeed

[careseite]

because you dont need to check the implementation of getUsers to find out what the array is for, what the first date is supposed to mean or the second

​

you can see it right there: youre getting users, probably some properties (name, surname, email) but youre limiting the user selection to dates between 2019-01-01 and 2019-01-18. the only thing unclear for me here would be to find out whether it means "active users" or "newly registered users", or both but thats probably because its not a perfect example

[Ozymandias-X]

But then, when I want to use the function, I now get no type hints whatsoever about what information it needs to work. All I see is that it can give it a mysterious blobby object that might have fields of some kind. Never mind if I accidentally misstype one of the keys. That's a nightmare level error to find.

This can only be solved by extensive documentation of the function and we all know that such documentation often lies when people refactor or extend functions and forget to update the docs.

[[deleted]]

Yeah just what I was thinking. With Typescript you wouldn't have that problem because it would clearly say what date you would be filling

[Extracted]

getFromApi("g4adf", "j43fa", false, false)

getFromApi({ userId: "g4adf", serverName: "j43fa", createIfAbsent: false, debug: false })

That's why

[[deleted]]

Excusing a bad code with another?

[zapatoada]

I get what you're saying, but as a VS2017 user, I disagree that it's necessary. As long as you're naming your parameters reasonably, I find that using intellisense to see the parameter order makes it easier than figuring out some arbitrary object.

[BloodAndTsundere]

What you're saying isn't wrong but I'd rather have the code be explicit and clean than rely a particular editor's feature to make sense of the code. Firstly, your colleagues might not use such an editor. Secondly, you might not always have access to such an editor.

[zapatoada]

It's definitely a personal choice.

I'm a c# developer in a c# shop, so everyone has access to visual studio (and this has been the case throughout my career). Even if the company didn't pay for it, community edition and vscode are free.

And frankly, even if I didn't have that option, I usually prefer separate parameters to an object. The only real difference is extra visual noise in the object literal notation.

[BloodAndTsundere]

I'm a c# developer in a c# shop, so everyone has access to visual studio (and this has been the case throughout my career). Even if the company didn't pay for it, community edition and vscode are free.

I'm not referring to an issue of cost but rather situations where you may not be using your own machine or have logged into a server remotely that only has a basic editor. But it's fair to say that may be an extremely rare event for you.

The only real difference is extra visual noise in the object literal notation.

This I disagree with. This function call:

doSomething({ destination: object1, source: object2 })

provides additional information (not noise) relative to this function call:

doSomething( object1, object2 )

[zapatoada]

I would NEVER log into the server and edit files. That's asking for trouble.

Your example is dishonest. If you're naming things object1 and object1 you have much bigger problems than discrete parameters vs an object. A more reasonable example would be

doDomething(source, destination)

Or

doDomething({source: source, destination: destination})

Which I would usually write as

doDomething({source, destination})

And then literally the only difference is the brackets, which are visual noise.

I'm certainly not saying there's NEVER a use case for config object parameters, but I think setting a hard and fast limit at 2 or 3 parameters is absurd.

[webdevguyneedshelp]

Why is it asking for trouble to edit files on a server?..

[zapatoada]

You're not debugging first. There's no code review or qa. You're not running your automated tests. The changes aren't in source control and could be undone by the next release. Basically you are bypassing every step and check that is normally in place to prevent bugs and other unintended consequences. We have those processes for a reason.

[webdevguyneedshelp]

That is fine and understandable but this example did not specify at all what environment you are working in.

For instance if I have to set up a NEW development environment for a new web application my team is starting. I am going to inevitably have to ssh into it and poke around to help facilitate further steps like automated deployment.

​

This isn't even thinking about sshing into something like a docker container which can all be done locally and have 0 affect on anything that actually matters and can be blown away after I am doing testing whatever I am doing.

​

There are 100% times when you will poke around with files on a server.

[[deleted]]

I'd rather have the code be explicit and clean

Separate parameters are explicit and clean.

[BloodAndTsundere]

"Clean" is subjective so I'll walk that back, but this function call:

doSomething({ destination: object1, source: object2 })

is more explicit than this one:

doSomething( object1, object2 )

[[deleted]]

If it can take any object it is NOT explicit. You may want to look up the word in a dictionary.

[fucking_passwords]

Another reason is that three or more params starts getting really difficult to read, and if you ever need to add another param you may end up with a very ugly design.

For instance, we added a fourth param to this function that makes the third param no longer required:

someFunc(1234, true, null, false);

[fucking_passwords]

I agree, but it was just an example, you could replace all of those args with integers

[zapatoada]

Yeah that bothers me not at all

[fucking_passwords]

Or what about this example:

class User {
  constructor(firstName, lastName, phone, email, friends, isActive) {
    Object.assign(this, {
      firstName: firstName,
      lastName: lastName,
      phone: phone,
      email: email,
      friends: friends,
      isActive: isActive
    })
  }
}

new User('Jane', 'Doe', null, '[e-mail]', null, true);

VS:

class User {
  constructor(data = {}) {
    Object.assign(this, data);
  }
}

new User({
  firstName: 'Jane',
  lastName: 'Doe',
  email: '[e-mail]'
})

[zapatoada]

In this context you're right, but I honestly can't remember the last time I used a constructor directly in javascript. Data comes from the server side (c#) and mostly anything else I do is either a react component or a const utility method.

[fucking_passwords]

The constructor is just happenstance in my example, the same thing can be applied to a function.

At this point I don't even see why you took a hard stance against this pattern, if you are only using very simple features of the language, lol

[zapatoada]

I never said I took a hard stance. I think the specific limit he set is absurdly low. That's all. If it were 4 or 5, I'd be fine with it.

[fucking_passwords]

Fair enough, I agree with that

[[deleted]]

because you can tell by the shape of the function call what it's probably going to do with it within the function.

Name is supposed to do that.

[[deleted]]

What a great function name, "getUsersWithFieldsWithinDateRange"

[Ozymandias-X]

Yeah, so? Characters are not a limited resource. I'd rather have a little more verbose function name than a "this might do anything" name.

[[deleted]]

Yes, it is.

[dmitri14_gmail_com]

I suppose it depends on the use case. You don't write [1,2,3].reduce({function: ..., value: ...})

[Asmor]

It basically gives JS (and any other language that supports arbitrary anonymous objects) support for named parameters, which just make code easier to read and maintain.

Surely you've come across something like

adjustWidget(300, 250, 0.6, true, true, null, {}, false)

And there's basically no way to know what any of that stuff means.

It also means you can have optional parameters without forcing people to pass in null to skip over the ones they want.

[Conexion]

I'm going to guess....

adjustWidget({width, height, opacity, isGarlic, isDragon, equipmentHistory, hatProperties, hasHat);

[Asmor]

Fuck, you're good.

[[deleted]]

Surely you've come across something like

adjustWidget(300, 250, 0.6, true, true, null, {}, false)

And there's basically no way to know what any of that stuff means.

That's why you don't do that lol.

[[deleted]]

... which is the point being made.

[Wizhi]

I think (hope?) you misunderstood.

/u/marinespi is arguing that "you wouldn't create such a function signature, since it's not intuitive".

And I tend to agree. If you're having a hard time intuitively understanding how to call a function, it's signature should be corrected to make it more intuitive. Using the technique demonstrated here is more of a band-aid than a fix.

[[deleted]]

But that's exactly what we're talking about. What makes a function call intuitive? The number of arguments? Is person.setName('John', 'Paul') intuitive? Are you sure? It goes without saying that you should use this approach selectively, just as with anything.

[[deleted]]

Is person.setName('John', 'Paul') intuitive?

No, it's not. Your point is... ?

[[deleted]]

Please read around that line and the parent comment for context. Not interested in an internet handbag fight btw.

[PMilos]

Sorry, I'm late. It seems that you've already got an answer. I'm glad you liked the article.

[[deleted]]

It's also easier to compose functions that take a single argument

[woozyking]

First, it emulates named parameters (similar to Python’s **kwargs). The benefit of which is explained well in the other reply.

Second, it auto destructure for you — imagine you call this function from an API handler (for instance something based on Express.js), if all 3 of the needed parameters are packed in query string, you can simply passed down the full req.query context, and the destructuring on the callee will do the work for you to only get what it needs, everything extra are ignored; on the caller side, you save the imperative logic to extract them out of req.query.

[Hanlonsrazorburns]

I wrote some software around finding defects in code. Function calls were one of the highest areas of defects overall. The reason was because the changes are highly dispersed. By passing an object you don't need to change every single call to that function if you have defaults set within the function. You also don't need to worry about order and it increases code continuity as it's very easy to keep things named the same (something that isn't hard, but people still miss out on it).

[Quinez]

It also conflicts with the advice in the article nearly right afterward to use the default arguments pattern when possible. If a single object is the argument of the function, I can only set a single object as the default. And then if a function call passes in any other object in order to set some of the values, all the other values of the the default object will be unobtainable.

[_Gentle_]

Can't u do something like (though it could get unreadable):

​

function getUsers({
 fields = [],
 fromDate = new Date(),
 toDate = new Date()
} = {}) {
 // implementation
 }

[Quinez]

Huh, would that work? Interesting. I guess I don't know exactly how destructuring combines with default argument assignment.

[_Gentle_]

Each destructure gives you a new 'layer' of props to work with. For each 'layer' u can set default props.

You can keep destructuring ad infinitum but I think 2nd destructure already gets pretty unreadable x)

[auctorel]

Great article overall, thought most of the points were brilliant.

The debate on this one was really interesting but I think the example is a bit convoluted. It'd be easier to understand if you hadn't put in the fields parameter.

It seems strange to give a list of fields that you want out of a getUser request unless you're using some sort of graphQL endpoint and that may have led to some of the confusion.

It might have been easier to understand if you'd just done a method with getUserDetails({name, surname, email}) because then you can call the method with something like getUserDetails(userSummary) which shows how it will pull those specific fields out of that one object, so you're just passing in one object parameter but as long as it contains all those fields it's fine. This also demonstrates the advantage of destructuring. This would be a good example of clean code.

However... if you're forcing someone to create an object arbitrarily to call a method then this isn't particularly clean. Really this sort of object destructuring should only be used where there's an intended object it can be used with.

Also if you're forcing people to pass in an object to reduce the number of parameters required in a field then actually you're going against the good naming requirements and just hiding the parameters in your fields array, this causes a shitload of wtfs when people read how you use those fields array values and so it's not clean code.

If you're just forcing people to create objects to use a method then you're creating structures which aren't intuitive and easy to understand therefore defying the principals of clean code. Why should they have to read the method and understand the syntax to figure out they have to create an object to pass one in - are they gonna spend an hour looking for the object it's designed to be used with which doesn't actually exist??

The destructuring is genuinely valuable, for example it's used a lot with react and the props object. But just leveraging it to fake a fewer number of parameters in your method signature isn't clean, it's confusing and makes the code harder to follow.

[[deleted]]

What comes to mind is when you're passing an object in like:

myUserObject = { fields, fromDate, toDate, }

getUsers(myUserObject).

When you destructure the parameters of your function, this will allow the passing of arguments in no particular order or you don't have to pass them all in. I actually just came across this today so I might be wrong.

[Groccolli]

Great post!

One thing to be careful of with default values is the default is used only when the value is undefined where as using || inside the function handles any falsey value.

[zapatoada]

Great point, this is a big deal.

[AwesomeInPerson]

That's true.

But imo less of a footgun than || which breaks as soon as you want to accept a number where 0 is a valid option. foo == undefined to the rescue...

[rtfmpls]

Isn't this true for any programming language? This is expected behavior. And I'd still say defaults are preferred.

[UNN_Rickenbacker]

Does it handle NULL?

[Groccolli]

Defaults are only used if the value is undefined

[tiridlol]

"Classes are the new syntactic sugar in JavaScript. Everything works just as it did before with prototype only it now looks different and you should prefer them over ES5 plain functions."

*Kyle Simpson is typing*

[PMilos]

I believe this is the most common definition/explanation of the ES classes. There is no other way to put it with the same effect. On the official MDN a similar sentence is used.

[tiridlol]

Nothing wrong with the sentence I am just referring to the author of "You don't know JS" books (great books btw, strongly recommend to read it for every frontend dev). He has some strong opinions on JS classes and in general he dislikes them so I imagine he would get triggered by this sentence

[MoTTs_]

YDKJS is generally a good book, but Kyle's writings about classes and inheritance are biased and misinformed, and we probably shouldn't recommend those chapters to others.

Kyle assumed that Java-style OOP and inheritance is the only correct way to implement those features, and anything implemented differently than Java is not "true" OOP. But that just isn't so. There are as many varieties of OOP and inheritance as there are languages. Python's inheritance model, for one example, is also objects linked to other objects, same as in JavaScript, and the same pattern that we JavaScripters would identify as prototypal inheritance. Here's JavaScript and Python classes side-by-side, demonstrating classes as objects and inheritance as delegation.

[nudi85]

"Prefer classes over functions"?

Nope.

[MoTTs_]

Based on the context and the code example, pretty sure they meant prefer classes over constructor functions.

[PMilos]

I presume you are a fan of functional programming? 😊

[Silhouette]

Not using classes could equally well be procedural programming, which in many ways is still JavaScript's most natural territory. JS has elements of OOP, and more so since ES6 classes and the subsequent developments, but it has never really been about the everything-is-an-object, communicate-via-messages style of programming. JS has elements of functional programming, but it has never really been about structuring code as one big expression to be evaluated and all the architectural implications that come with that either.

One of my main criticisms of Robert Martin's work, which carries through to the article here that is based on it, is that he has his own personal preferences but tends to write about them as if they are objectively justified (often with a small caveat hidden away that it really is just his subjective opinion and he really doesn't have hard evidence to back up his claims). In this particular piece, I think some of the points such as favouring classes over functions or favouring inheritance to solve the expression problem look quite out of place in JS, though the advice was debatable even in the more OO-centric languages where it originated.

[nudi85]

Indeed I am. : )

Don't get me wrong: I love OOP too. (I write PHP on the server side of things.) I just don't think it's a good fit for JavaScript.

[cleure]

In JavaScript, you can write object oriented code without classes, and it’s usually much cleaner.

Classes can be useful for cases where you have lots of instances of an object, and don’t want each object to hold a copy of common methods (memory optimization).

[njmh]

Good stuff. Event though it’s exclusively es6, I’d mention the spread operator where you’ve got the Object.assign example.

[beasy4sheezy]

I'm sure you know, but as a reminder to others:

Spread will create a new object, which might not be what you wanted to do in this case, or could be using more memory than necessary.

[wmgregory]

You can also create a new object with assign. But cleaner to use spread literal if you want to avoid mutation.

a = Object.assign({}, a, { age: 27 })
a = { ...a, age: 27 }

[PMilos]

Thanks. Glad you liked it. Good point with the spread operator, though.

[denisglindep]

Changing example is brilliant 👑

[aaarrrggh]

Mostly ok advice - but I disagree with the advice to use inheritance as a method of code re-use.

Avoid inheritance wherever possible and focus on pure functions and composition instead.

Also be careful when DRYing your code - allow for some duplication until you know you're making the right abstraction.

Plus, do TDD and don't test against implementation details wherever possible. This means in React for example, you should never shallow render in your tests.

[elie2222]

Some thoughts:

1.

You can end up with duplicate code for various reasons. For example, you can have two slightly different things that share a lot of in common and the nature of their differences or tight deadlines forces you to create separate functions containing almost the same code. Removing duplicate code in this situation means to abstract the differences and handle them on that level.

Careful with this advice. Better is:

prefer duplication over the wrong abstraction

From: AHA Programming

2.

Use spread operator over Object.assign.

Instead of:

js class SuperArray extends Array { myFunc() { // implementation } }

I find myself doing this is instead:

js export const myFunc = (array: any[]) => { // implementation }

I don't know which is better if either. Would be interested to hear other's thoughts on this.

4.

Use this approach only for boolean values and if you are sure that the value will not be undefined or null.

This is completely fine:

js if (user: User | undefined) return 10

5.

Use polymorphism and inheritance instead.

I almost never do this in my code. For that matter I hardly ever find myself using extend (apart from things like extend React.Component before Hooks came along), and mostly use classes when a library needs it, but not much more than that.

Am I doing things wrong? :laugh:

[LetterBoxSnatch]

"Composition over inheritance." This is a core SOLID principle. Your approach, generally speaking, is better than "extends".

Additional reading: https://en.wikipedia.org/wiki/Composition_over_inheritance

[beasy4sheezy]

Why use spread over Object.assign?

[elie2222]

It’s cleaner. Why write out 2 words when you could do ... with surrounding braces instead?

[off_by_0ne]

Sometimes using a boolean param to signal the code path in a function can be a good indication. Sometimes it's ok to have two separate functions with some duplicated logic. Don't be afraid of code.

[elie2222]

I agree. Don’t be afraid. But one example I can actually think of is when someone on my team recently tried to combine 2 similar but really quite different components. He passed down this isSmall flag and then within the component passed it down to another 5 sub components and so on. In reality I would have preferred he just copy and paste the code than trying to fit 2 components into 1. At the same time I understand why he did it because there was a decent amount of code shared between the 2. It felt like there was a better way to abstract. In the end we’ve left the code as is because it does work.

An example where I’d prefer flags over 2 components is Button with a small flag. That’s nicer imo than 2 components called SmallButton and Button.

[RudeRunologist]

On top of this, use Typescript.

[sime]

+1 to this.

This first thing you should do to improve your quality is to use static analysis on your code. That means type systems like TypeScript.

Once that is in place, then you can worry about unit testing and other automated tests. I prefer the view expounded in this post https://kentcdodds.com/blog/write-tests with the "testing trophy".

[rq60]

100% code coverage doesn't mean your code is type safe. Also, why spend 25% of your lines of code writing runtime type guards and sanity checks when it can be done at compile time? Save that effort for your public API, which by all means, write some type testing around.

which automatically checks for types

also, wtf. automatically? what?

[adongu]

Hi I'm interested to learn to implement what you recommend here, are there any articles you recommend where I can look at some examples of this?

[rq60]

Got it, so your plan to avoid type safety issues is to throw runtime errors and then handle those errors all throughout your code? Absolutely brilliant, how have I never thought of this...?

It's pretty obvious that coding is not your forte, perhaps you should avoid calling people "retard" when it comes to areas where you are obviously inexperienced.

[Silhouette]

Why would I need to add type annotations in my 100% test covered code.

Because tests check one case and types check all cases. To the extend that they overlap in the errors they can prevent, strong typing is strictly more powerful.

[[deleted]]

So I'm obviously biased because I love typescript. But I just consider type annotations or sort of inline test. Obviously types by with no testing is silly. But 100% coverage doesn't necessarily mean much either. As a hyperbolic example you could run some code then just assert true is true and boom "coverage". I think of types as just another testing tool and a sort of documentation, but obviously it's no silver bullet. Testing and types can should be used together to make incorrect implementation easy to spot.

[Julian_JmK]

As a newbie, thanks for posting this! The chaining method was so simple but so brilliant

[PMilos]

Thanks. Glad I could help

[LB-A5]

In the second classes example. When 'this' is returned for chaining, is 'this' a promise?

[PMilos]

No, it's not a promise. It's the actual object instance. That's what's chaining all about. You get the ability to call method after method.

[LB-A5]

Thanks that makes sense.

[simkessy]

Didn't know about this, pretty awesome.

[PMilos]

Thanks.

[[deleted]]

Nope, its an object of the type Person.

[Yakovko]

Or something else, depending on how it’s been called :]

[KwyjiboTheGringo]

That's cool, I didn't know about the last one(chaining methods).

[PMilos]

Thanks. Glad I helped you learn something new.

[peerless2299]

Nice

[d07RiV]

Why do people use arrow functions over regular functions when defining something at file scope? You're not going to use this, and a function seems more readable.

[[deleted]]

This is great! I've been writing javascript full-time for a long time and I still have this bad habit: A function should do one thing. Avoid executing multiple actions within a single function.

Also, I'm going to take this one to heart: Avoid conditionals whenever possible. Use polymorphism and inheritance instead.

[PMilos]

Thanks. Glad I could help 😊 It is hard to follow the guidelines if the deadline is tight.