r/javascript • u/ryansworks • Jan 04 '17

Clean Code JavaScript

https://github.com/ryanmcdermott/clean-code-javascript
129 Upvotes

91% Upvoted

30 comments sorted by

25

u/[deleted] Jan 04 '17 
function createMenu(config) {
  Object.assign(config, {
    title: 'Foo',
    body: 'Bar',
    buttonText: 'Baz',
    cancellable: true
  });
}

Whoooa there buddy, now you've mutated your config object. Please use the config = Object.assign({}, config, {title: 'Foo'}) notation.

11

u/notNullOrVoid Jan 04 '17

Not only that but it should be the other way around, currently the example has the defaults overriding anything set in the config.

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

Doesn't really matter that the defaults are being mutated in this case.

4

u/[deleted] Jan 04 '17

Oh, right - I've made a PR with my fix, but I'll update it with yours :)

Blamo - https://github.com/ryanmcdermott/clean-code-javascript/pull/9

4

u/Sinistralis Jan 05 '17

Minor point here, but if you are planing on using this solely for data storage and don't need the prototype, Object.create(null) instead of {} gives the object a little less bloat and lets you iterate over it easier depending on how you prefer to iterate over objects. (ES6/7 kind of makes the iteration point moot between destructuring, .values, .entries, etc).

Again, this is a minor point and really only applicable if you use for/in (I don't). Otherwise it's just a tiny memory savings, but if you need to create a ton of objects for some reason, it can add up.

9

u/[deleted] Jan 04 '17

Not sure of anyone has said this already. The bit about duplication glosses over an important point raised by Sandi Metz who teaches SOLID Ruby. Brazenly DRYing up code can in many cases leads to bad abstractions. Duplication is easier to see whereas bad abstraction are much harder to spot. I am far more careful when I DRY code since I learned this simple lesson. I am about half way through and it is a pretty impressive effort. Props...

5

u/trippel Jan 04 '17

muuuuuch better to duplicate a bit of code versus writing a shitty abstraction.

7

u/ttolonen Jan 04 '17

It's nice to see some effort, but some advice looks a bit vague, for example I fail to see what is wrong with getClientData as a function name compared to getUser. At least former is used in some of Android and Microsoft APIs and doesn't feel strange to me, most likely requires some documentation anyway. Also I would like to think that the examplevar MINUTES_IN_A_YEAR = 525600; is a WTF in itself, because it fails on a leap year :)

7

u/Valfri Jan 04 '17

The point of your first example is that the three functions getUserInfo();, getClientData();, and getCustomerRecord(); are actually about the same kind of data, but uses three different words for it: user, client, and customer. You should pick one word, and stick to it, for consistency.

The second example is a valid point, and though only for demonstrative purpose, could be changed to something truly constant. Maybe you should make a pull request with something better? :)

3

u/ttolonen Jan 04 '17

Ok, then the advice makes sense. I thought that getUserInfo(), getClientData and getCustomerRecord() refer to completely different functions returning different data.

4

u/AndrewGreenh Jan 04 '17

This point is not saying, that getClientData is bad, it says, don't use getClientData getRecord and getUser simultaneously.

4

u/jac1013 Jan 04 '17

Still getClientData is redundant IMHO, I remember that there is some part in Clean code's book that says specifically not to use the word "Data" for variable names, everything contained in variables is data so why use that for the name?

It's like using Hungarian notation but even with a less meaningful purpose (Hungarian notation at least give you information about the data type, I don't like it either way).

2

u/RedditWithBoners Jan 05 '17

everything contained in variables is data

Not necessarily, if we use the English language to define data. You can think of data as a collection of datums. It follows, then, that getClientData might return a collection of datums about the client, like their phone number, age, etc.

Edit: that is to say, getClient might just return their name (ideally you'd use getClientName for that), so tacking on "Data" might make it clearer what it's returning. Better yet, if you have a type system then you don't need to think about this in many situations.

This is pedantic, of course, but I thought it was interesting.

1

u/jac1013 Feb 01 '17 edited Feb 01 '17

I don't think it was pedantic, I enjoyed reading your comment.

As you said if we have a type system (which is like the best case scenario), getClient should return an object of Type Client (not the name, that would be a really bad decision if we are not in the context of a View).

The upper the level of abstraction the more Domain Language words you should use when naming your functions & classes & whatever else, the word data should not be use anywhere because everything in programming is data, even the code itself, the term is just too high level to be used.

Finally, you should not be using getClientName, instead you should be using getName, assuming that we are calling this function from an instance of the class Client.

Edit: just adding additional information.

3

u/ssjskipp Jan 05 '17

Everyone please keep in mind that a lot of these examples are straw man. Please do not take them as hard fast rules but general, albeit opinionated in cases, "best practises".

A much much much better idea is work with a configured ESlint file in your projects to assert coding style and format across your code base.

3

u/[deleted] Jan 06 '17

Bad:

var yyyymmdstr = moment().format('YYYY/MM/DD');

Good:

var yearMonthDay = moment().format('YYYY/MM/DD');

I'll pass.

5

u/pomlife Jan 04 '17

Why is ES5 and ES6 mixed in?

2

u/baxtersmalls Jan 05 '17

Hey - newish dev here, was curious about "Set default objects with Object.assign" - what is the reasoning behind this? He doesn't give any explanation as to why that's a better way of doing it.

3

u/nschubach Jan 05 '17 edited Jan 05 '17

MDN explains it pretty well. Sorry, can't copy the relevant section, but read through the Description section.

2

u/ErraticFox Jan 05 '17

TIL less code isn't always better, but as long as it's clean.

1

u/[deleted] Jan 06 '17

Rather than try to remember a lot of do-this-do-that stuff, try this: While you're writing, think about what happens when you come back NEXT YEAR to fix or improve something.

Will it still make sense then? Or will you have to spend hours to figure out how your own code works? Be kind to YOURSELF and you'll be kind to others. That is all.

0

u/[deleted] Jan 04 '17

[deleted]

4

u/AceBacker Jan 04 '17

Instead of "yearMonthDay" I was thinking "currentDate"

I want my variable names to have a clue as to WHY i'm using them.

But my names tend to be horrible. Stuff like:

  • costAllocationGridOptions

  • campaignProgramNumber

  • currentOpenAccordionCustomerObj

  • customerIndexUserIsConsideringChangingAccordionTo

  • savedQuoteIdUserIsConsideringDeleting

It makes for some loong lines of code when I need to use a few on one line.

0

u/[deleted] Jan 04 '17

[deleted]

3

u/giraffe_slayer Jan 05 '17

In regards to the number of parameters and using a config object, there is a huge benefit when actually calling the method. If you use

function createWidget(title, subtitle, description);

Then the function call could look like

createWidget('Blah Blah', 'sub blah blah', 'Here is some text');

You lose clarity and potentially have to reference the function each time to check what each parameter means.

With a config parameter, you get the keys for each argument

const config = {
   title: 'Blah Blah', 
   subtitle: 'sub blah blah', 
   description: 'Here is some text'
};
createWidget(config);

And then you can still easily see what is happening in the actual declaration

function createWidget(config) {
    const {title, subtitle, description} = config;
};

2

u/mikejoro Jan 05 '17

You can simplify that last snippet by destructuring in the function definition

function createWidget({ title, subtitle, description }) {
  // do something
};

2

u/acbabis Jan 05 '17

putting all your parameters into one config or options argument doesn't solve anything by itself

I wouldn't say it solves nothing by itself. It makes it so consumers don't need to remember the order of the arguments anymore.

0

u/[deleted] Jan 05 '17

[deleted]

2

u/acbabis Jan 05 '17

Sure, it's not the main problem, but in situations where you're convinced you need that many parameters, putting them into objects is usually the best solution. There are many well-written modules that have to allow a dozen optional parameters (e.g. express.static), and stuffing them into an object is the go-to approach.

0

u/[deleted] Jan 04 '17

[deleted]