ryanmcdermott
Great guide! My critique of a few of the principles
Not-so-well put together brain dump follows. Note that in some parts the tone may appear a bit harsh, but I stand by my criticism and believe that some of the issues I raise below will confuse the beginner/intermediate programmers that this guide is intended to help, and at worst even do more harm than good.
:duck:
Avoid Side Effects (part 1 & part 2)
https://github.com/ryanmcdermott/clean-code-javascript#avoid-side-effects-part-1 https://github.com/ryanmcdermott/clean-code-javascript#avoid-side-effects-part-2
Both of these points seem to have good advice contained within them, but I feel like they are mushing together lots of separate concepts, and could be articulated better. They are mixing together the ideas of: pure functions, side-effectful functions, centralization of side effects, global variables (evil!), global state (also evil?), and mutable vs. immutable data.
Also, I feel it is appropriate to mention here how the approach of "avoiding side effects" benefits testing.
BTW: the "good" example in part 2 calls Date.now() which is arguably a side effect.
Don't write to global functions
https://github.com/ryanmcdermott/clean-code-javascript#dont-write-to-global-functions
Very wise advice. But I don't like the example. Just write a regular function function diffArrays(array1, array2). Then you can use it with any array you get from anywhere instead of needing to craft a special type of array. Creating an Array subclass for every new function you want to perform on arrays would lead to dozens of array subclasses and madness...
Favor functional programming over imperative programming
https://github.com/ryanmcdermott/clean-code-javascript#favor-functional-programming-over-imperative-programming
Yikes.
Such a strong claim. What is "functional programming" anyway? Just like "what is OOP?" there is no single agreed-upon answer. And the authors don't explain what they mean. So the following is based upon what I personally understand "functional programming" to mean :D
JavaScript is not a good functional programming language: there is no form of "let" expression-based binding, it lacks any kind of loop fusion, there is no tail call optimization, no pattern matching, and if and switch are not expressions (although my prediction is that they might be so in a future JavaScript version).
The high order array functions (map, reduce, find, etc...) are half-baked, don't compose, and worst of all don't work with Promises nor with async/await syntax.
Let's look at the example that was given:
const programmerOutput = [
{
name: 'Uncle Bobby',
linesOfCode: 500
}, {
name: 'Suzie Q',
linesOfCode: 1500
}, {
name: 'Jimmy Gosling',
linesOfCode: 150
}, {
name: 'Gracie Hopper',
linesOfCode: 1000
}
];
Bad:
let totalOutput = 0;
for (let i = 0; i < programmerOutput.length; i++) {
totalOutput += programmerOutput[i].linesOfCode;
}
Good:
const totalOutput = programmerOutput
.map(output => output.linesOfCode)
.reduce((totalLines, lines) => totalLines + lines);
I like the "bad" code. It is just as clear, concise, readable, and understandable as the "good" code (although a "for..of" loop would be even better instead of using an index variable).
And this is a "best case" show of where "functional" programming works in JavaScript. But what if you need to do something a little more complicated? Here is a random example that I'm just pooping out of my head:
We want to find the first adjacent pair of programmers in the programmerOutput array, starting from the end, whose difference of "linesOfCode" between them is less than or equal to 1000, and return the index as well as both of their names. If no such adjacent pair of programmers exist, then return null.
The solution in "idiomatic" JavaScript using a loop is pretty simple:
function f(programmerOutput) {
for (let i = programmerOutput.length - 1; i >= 1; i--) {
const diff = programmerOutput[i - 1].linesOfCode - programmerOutput[i].linesOfCode;
if (Math.abs(diff) <= 1000) {
return {
index: i - 1,
name1: programmerOutput[i - 1].name,
name2: programmerOutput[i].name
};
}
}
return null;
}
Sure, you have to be careful with your loop index and watch out for off-by-one errors, but overall pretty readable IMHO.
Now let's look at the functional solution that I was able to come up with:
function f(programmerOutput) {
const tail = programmerOutput.slice(1);
const pairs = tail.map((v, i) => [programmerOutput[i], v]);
const reversed = [].concat(pairs).reverse();
const i = reversed.findIndex(([p1, p2]) => {
const diff = p1.linesOfCode - p2.linesOfCode;
return Math.abs(diff) <= 1000;
});
return i < 0
? null
: {
index: tail.length - i - 1,
name1: reversed[i][0].name,
name2: reversed[i][1].name
};
}
Is this truly preferable? I find it a mess, and hard to understand... and I wrote it! And although performance should usually not be a concern when writing "clean code", it's worth noting that this code will be super slow and generate tons of garbage.
I'm sure there is a better way to write it functionally, especially if you use one of the "functional" JavaScript libraries; but no matter what you come up with, I still claim that nothing will be as clear as the simple "for" loop version. And this isn't a cherry-picked example: any moderately complex "functional" code you write that uses map/filter/reduce/whatever will be cleaner if it is rewritten to use plain old loops.
(BTW I'm talking only about JavaScript here... I'm sure that the functional version of this code in ClosureScript or Haskell will look beautiful)
I'm going to impudently claim that the authors of this guide don't actually believe that you should "Favor functional programming over imperative programming" when writing JavaScript. None of the examples in the rest of this guide are in any way "functional". And I doubt that the production JavaScript code that they write is functional: in their own code do they truly strongly avoid the use of any loops or "let" variables and any and all mutation?
Functional programming does have its place in JavaScript; one place where it seems to work well is with the "React" library (especially when combined with "immutablejs" library). Also, simple usages of map and filter can often indeed make code shorter and clearer. But we shouldn't go overboard and say that functional programming should be "favored" everywhere. Plain old imperative code with loops and variables is just fine, and leads to simple and clean code.
BTW: the "good" example contained in this rule written by the authors is broken. It will crash if the programmerOutput array is empty. Note that the "bad" version is well written and will return the correct result of 0. This is a good demonstration of how JavaScript's reduce is half-baked: the JavaScript committee should have made reduce's initialValue argument required. Otherwise, you end up with code like what the authors wrote here that contains the worst kind of bug: The code appears to work correctly, then you ship it to production where it continues to work fine 99% of the time until it randomly blows up on an edge case that you didn't anticipate.
Anyway, my summary is that this principle is worded much too strongly. I would re-word it to: "Use a light sprinkle of functional programming when appropriate".
Avoid conditionals
https://github.com/ryanmcdermott/clean-code-javascript#avoid-conditionals
A good rule, but I feel that not enough support is given for the advantages of this approach. I understand that this is meant to be a concise guide, so maybe add links to external references?
For example, for this rule it may be worth referring readers who are interested in a more in-depth analysis to the Anti-If Campaign website
Also later on the principles in the "SOLID" section could use some good external references.
Use getters and setters
https://github.com/ryanmcdermott/clean-code-javascript#use-getters-and-setters
hm... not sure where i stand on this one... ugly verbose boilerplate code... but I guess if your object is highly stateful then the added robustness is worth it.
btw, how do you "lazy load your object's properties" in an async language like JavaScript? I guess there are ways to do it, but an example would be cool.
Use method chaining
https://github.com/ryanmcdermott/clean-code-javascript#use-method-chaining
Not my taste personally. I find this style way too "cute" for no real tangible benefit.
Only comment things that have business logic complexity
https://github.com/ryanmcdermott/clean-code-javascript#only-comment-things-that-have-business-logic-complexity
Here is the "good" example:
function hashIt(data) {
let hash = 0;
const length = data.length;
for (let i = 0; i < length; i++) {
const char = data.charCodeAt(i);
hash = ((hash << 5) - hash) + char;
// Convert to 32-bit integer
hash &= hash;
}
}
First of all, this fails to follow the advice in Don't over-optimize (no need to cache data.length)
Second, aren't we supposed to be using functional programming? ;) This function is actually a good candidate for the use of "map" and "reduce". (I don't seriously mean this; I continue to believe that a plain old loop is the best way to write this function in JavaScript, as is done here)
BTW: both the "good" and "bad" code forget to return "hash" at the end.
But getting back to the point of this rule: in this case a named function would be better than a comment:
function to32bitInt(num) {
return num & num;
}
function hashIt(data) {
let hash = 0;
for (let i = 0; i < data.length; i++) {
const char = data.charCodeAt(i);
hash = ((hash << 5) - hash) + char;
hash = to32bitInt(hash);
}
return hash;
}
Note that a comment that WOULD be appropriate here is one describing the hash function we are using. Does it have a well-known name? What are its characteristics? Adding a comment with a link to an in-depth article explaining this hash function would also be suitable.
The End
The rest of the principles in this guide overall provide good solid advice IMHO :+1:
2 мая 2018 г. 9:45 PM пользователь "benny-medflyt" [email protected] написал:
Not-so-well put together brain dump follows. Note that in some parts the tone may appear a bit harsh, but I stand by my criticism and believe that some of the issues I raise below will confuse the beginner/intermediate programmers that this guide is intended to help, and at worst even do more harm than good.
🦆 Avoid Side Effects (part 1 & part 2)
https://github.com/ryanmcdermott/clean-code-javascript#avoid-side-effects- part-1 https://github.com/ryanmcdermott/clean-code-javascript#avoid-side-effects- part-2
Both of these points seem to have good advice contained within them, but I feel like they are mushing together lots of separate concepts, and could be articulated better. They are mixing together the ideas of: pure functions, side-effectful functions, centralization of side effects, global variables (evil!), global state (also evil?), and mutable vs. immutable data.
Also, I feel it is appropriate to mention here how the approach of "avoiding side effects" benefits testing.
BTW: the "good" example in part 2 calls Date.now() which is arguably a side effect. Don't write to global functions
https://github.com/ryanmcdermott/clean-code-javascript#dont-write-to- global-functions
Very wise advice. But I don't like the example. Just write a regular function function diffArrays(array1, array2). Then you can use it with any array you get from anywhere instead of needing to craft a special type of array. Creating an Array subclass for every new function you want to perform on arrays would lead to dozens of array subclasses and madness... Favor functional programming over imperative programming
https://github.com/ryanmcdermott/clean-code-javascript#favor-functional- programming-over-imperative-programming
Yikes.
Such a strong claim. What is "functional programming" anyway? Just like "what is OOP?" there is no single agreed-upon answer. And the authors don't explain what they mean. So the following is based upon what I personally understand "functional programming" to mean :D
JavaScript is not a good functional programming language: there is no form of "let" expression-based binding, it lacks any kind of loop fusion, there is no tail call optimization, no pattern matching, and if and switch are not expressions (although my prediction is that they might be so in a future JavaScript version).
The high order array functions (map, reduce, find, etc...) are half-baked, don't compose, and worst of all don't work with Promises nor with async/await syntax.
Let's look at the example that was given:
const programmerOutput = [ { name: 'Uncle Bobby', linesOfCode: 500 }, { name: 'Suzie Q', linesOfCode: 1500 }, { name: 'Jimmy Gosling', linesOfCode: 150 }, { name: 'Gracie Hopper', linesOfCode: 1000 } ];
Bad:
let totalOutput = 0; for (let i = 0; i < programmerOutput.length; i++) { totalOutput += programmerOutput[i].linesOfCode; }
Good:
const totalOutput = programmerOutput .map(output => output.linesOfCode) .reduce((totalLines, lines) => totalLines + lines);
I like the "bad" code. It is just as clear, concise, readable, and understandable as the "good" code (although a "for..of" loop would be even better instead of using an index variable).
And this is a "best case" show of where "functional" programming works in JavaScript. But what if you need to do something a little more complicated? Here is a random example that I'm just pooping out of my head:
We want to find the first adjacent pair of programmers in the programmerOutput array, starting from the end, whose difference of "linesOfCode" between them is less than or equal to 1000, and return the index as well as both of their names. If no such adjacent pair of programmers exist, then return null.
The solution in "idiomatic" JavaScript using a loop is pretty simple:
function f(programmerOutput) { for (let i = programmerOutput.length - 1; i >= 1; i--) { const diff = programmerOutput[i - 1].linesOfCode - programmerOutput[i].linesOfCode; if (Math.abs(diff) <= 1000) { return { index: i - 1, name1: programmerOutput[i - 1].name, name2: programmerOutput[i].name }; } } return null; }
Sure, you have to be careful with your loop index and watch out for off-by-one errors, but overall pretty readable IMHO.
Now let's look at the functional solution that I was able to come up with:
function f(programmerOutput) { const tail = programmerOutput.slice(1); const pairs = tail.map((v, i) => [programmerOutput[i], v]); const reversed = [].concat(pairs).reverse(); const i = reversed.findIndex(([p1, p2]) => { const diff = p1.linesOfCode - p2.linesOfCode; return Math.abs(diff) <= 1000; }); return i < 0 ? null : { index: tail.length - i - 1, name1: reversed[i][0].name, name2: reversed[i][1].name }; }
Is this truly preferable? I find it a mess, and hard to understand... and I wrote it! And although performance should usually not be a concern when writing "clean code", it's worth noting that this code will be super slow and generate tons of garbage.
I'm sure there is a better way to write it functionally, especially if you use one of the "functional" JavaScript libraries; but no matter what you come up with, I still claim that nothing will be as clear as the simple "for" loop version. And this isn't a cherry-picked example: any moderately complex "functional" code you write that uses map/filter/reduce/whatever will be cleaner if it is rewritten to use plain old loops.
(BTW I'm talking only about JavaScript here... I'm sure that the functional version of this code in ClosureScript or Haskell will look beautiful)
I'm going to impudently claim that the authors of this guide don't actually believe that you should "Favor functional programming over imperative programming" when writing JavaScript. None of the examples in the rest of this guide are in any way "functional". And I doubt that the production JavaScript code that they write is functional: in their own code do they truly strongly avoid the use of any loops or "let" variables and any and all mutation?
Functional programming does have its place in JavaScript; one place where it seems to work well is with the "React" library (especially when combined with "immutablejs" library). Also, simple usages of map and filter can often indeed make code shorter and clearer. But we shouldn't go overboard and say that functional programming should be "favored" everywhere. Plain old imperative code with loops and variables is just fine, and leads to simple and clean code.
BTW: the "good" example contained in this rule written by the authors is broken. It will crash if the programmerOutput array is empty. Note that the "bad" version is well written and will return the correct result of 0. This is a good demonstration of how JavaScript's reduce is half-baked: the JavaScript committee should have made reduce's initialValue argument required. Otherwise, you end up with code like what the authors wrote here that contains the worst kind of bug: The code appears to work correctly, then you ship it to production where it continues to work fine 99% of the time until it randomly blows up on an edge case that you didn't anticipate.
Anyway, my summary is that this principle is worded much too strongly. I would re-word it to: "Use a light sprinkle of functional programming when appropriate". Avoid conditionals
https://github.com/ryanmcdermott/clean-code-javascript#avoid-conditionals
A good rule, but I feel that not enough support is given for the advantages of this approach. I understand that this is meant to be a concise guide, so maybe add links to external references?
For example, for this rule it may be worth referring readers who are interested in a more in-depth analysis to the Anti-If Campaign https://francescocirillo.com/pages/anti-if-campaign website
Also later on the principles in the "SOLID" section could use some good external references. Use getters and setters
https://github.com/ryanmcdermott/clean-code-javascript#use-getters-and- setters
hm... not sure where i stand on this one... ugly verbose boilerplate code... but I guess if your object is highly stateful then the added robustness is worth it.
btw, how do you "lazy load your object's properties" in an async language like JavaScript? I guess there are ways to do it, but an example would be cool. Use method chaining
https://github.com/ryanmcdermott/clean-code-javascript#use-method-chaining
Not my taste personally. I find this style way too "cute" for no real tangible benefit. Only comment things that have business logic complexity
https://github.com/ryanmcdermott/clean-code-javascript#only-comment- things-that-have-business-logic-complexity
Here is the "good" example:
function hashIt(data) { let hash = 0; const length = data.length;
for (let i = 0; i < length; i++) { const char = data.charCodeAt(i); hash = ((hash << 5) - hash) + char;
// Convert to 32-bit integer hash &= hash;} }
First of all, this fails to follow the advice in Don't over-optimize https://github.com/ryanmcdermott/clean-code-javascript#dont-over-optimize (no need to cache data.length)
Second, aren't we supposed to be using functional programming? ;) This function is actually a good candidate for the use of "map" and "reduce". (I don't seriously mean this; I continue to believe that a plain old loop is the best way to write this function in JavaScript, as is done here)
BTW: both the "good" and "bad" code forget to return "hash" at the end.
But getting back to the point of this rule: in this case a named function would be better than a comment:
function to32bitInt(num) { return num & num; } function hashIt(data) { let hash = 0; for (let i = 0; i < data.length; i++) { const char = data.charCodeAt(i); hash = ((hash << 5) - hash) + char;
hash = to32bitInt(hash);} return hash; }
Note that a comment that WOULD be appropriate here is one describing the hash function we are using. Does it have a well-known name? What are its characteristics? Adding a comment with a link to an in-depth article explaining this hash function would also be suitable. The End
The rest of the principles in this guide overall provide good solid advice IMHO 👍
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/ryanmcdermott/clean-code-javascript/issues/242, or mute the thread https://github.com/notifications/unsubscribe-auth/AX3CKdJENuv1GVn0c8-1HueInJWUgX92ks5tuZw-gaJpZM4TvT1E .
agree that method chaining should probably be removed. If you have to chain then that means your API isn't great. If you're doing a lot of "sets" in a row then you're already playing with mutable data. Using a functional approach will reduce the need for it. There is nothing that is presented here as to why chaining is actually good, except that it's "expressive" (how?) and "less verbose" (how much extra verbosity is it adding? if it is then that's an API issue).
probably the only place you should use chaining is in a builder pattern....
The part about "Favor functional programming over imperative programming" is highly biased. Most of the time for-loops are easily understandable (clean-code), extensible over time, and more performant.
Here is an example. Something as simple as splitting an array like this, which is also more performant.
let arr1 = []
let arr2 = []
for (let i = 0; i < size ; i++) {
const elm = arr[i]
if (elm % 2 === 0) {
arr1.push(elm)
} else {
arr2.push(elm)
}
}
can become a hard puzzle with functional programming, which is both slower and hard to understand.
const [arr1, arr2] = arr.reduce((result, elm) => {
result[elm % 2 === 0 ? 0 : 1].push(elm)
return result
}, [[], []])
Functional programming has good things such as preferring "well-tested/well-documented functions" (e.g. std::sort) over manually written code, but simply saying prefer functional style is highly biased advice!
aminya I'm afraid your "hard to understand" is also biased.
But I agree with your sentiment, I think. Reduce is a catch all solution for when map and filter aren't enough. Pure JS lacks the tools to be used as decent functional programming language.
This is lodash's version
partition(arr, n => n % 2)
And then one could ask "but how is partition implemented"? And I would say "does it matter?". Performance is hardly the dealbreaker.
Here is a pure JS alternative
const evens = arr.filter(n => n % 2 === 0);
const odds = arr.filter(n => n % 2 !== 0);
I just wanted to comment on this too:
Bad:
let totalOutput = 0;
for (let i = 0; i < programmerOutput.length; i++) { totalOutput += programmerOutput[i].linesOfCode; }Good:
const totalOutput = programmerOutput .map(output => output.linesOfCode) .reduce((totalLines, lines) => totalLines + lines);
I wish we had something more readable for reduce like sum, already built into the Array prototype:
Array.prototype.sum = function() {return [].reduce.call(this, (a,i) => a+i, 0);}
const totalOutput = programmerOutput
.map(output => output.linesOfCode)
.sum();
Because then a lot of debates about imperative vs. functional wouldn't exist. reduce is the lowest-level higher-order function of all, therefore the hardest to read, and should only be used when there is nothing else to use. I often find myself using lodash's functions when polyfills are not an option, because the Array prototype lacks too many useful functions. Plus lodash isn't the most fun to use because it makes you mix your style (chaining vs. function composition).
Ideally, function composition would be the better syntax (Edit: wrong, actually, piping would be it, because composition starts with the last function you would call, which is like Yodaism: forcing you to read from right to left.), but that's not the only problem with JS's functional style. reduce messes it up with 1 more problem: it uses the data-last approach for parameterization, but IMO, the lambda function should come last in the parameter list. The current way makes it awkward to use. Compare:
const sumWithInitial = array1.reduce((accumulator, currentValue) => {
return accumulator + currentValue;
}, initialValue);
vs.
const sumWithInitial = array1.reduce(initialValue, (accumulator, currentValue) => {
return accumulator + currentValue;
});
(I know this code is too simple and we could use the shorter syntax, please ignore that for now.)
Same for setTimeout, which currently reads as: set a timeout for this function of N seconds instead of set a timeout of N seconds for this function. I am simply not used to reading parameters after the function. Gradle doesn't do that either in the ad-hoc task creation syntax. I remember in the past a conversation about both syntaxes being useful depending on the language, I just don't remember where I discussed it.
I may be biased about this, but there's a nice article about the topic for more info: https://www.javierchavarri.com/data-first-and-data-last-a-comparison/
Regarding getters and setters:
ES6 introduces a much less annoying syntax for this problem: get and set. I suggest using it in the guide. Because I agree too, it's so much pain when every field is forced to be wrapped into getters and setters even when you have zero need for them. It's just as much of a ceremony as semicolons after each line, when in fact, knowing the language better is the key, plus a lot of automated tests, maybe linter tools that warn about potential problems, but I digress...
Plus there are several articles online about Why getters and setters are terrible, there I just linked one example. We may not always be able to avoid this getter-setter pattern, but when we can, I'd rather do. I know there are exceptions, like, I can imagine that Angular frameworks generally favor the get/set keywords when they actually help with the component templates.