Sunday, November 27, 2011

About Felix's Style Guide

Style guides are good as long as these are meaningful and a bit "open minded" because if one style is recognized as anti pattern then it must be possible to update/change it.

The Felix's Node.js Style Guide is surely a meaningful one, but I hope it's open minded too.

Why This Post

Because I write node.js code as well sometimes and I would like to write code accepted by the community. Since developers may go too religious about guides I don't want explain all these points per each present or future node.js module I gonna write ... I post it once, I will eventually update it, but it's here and I can simply point at this any time.

Style Guide Side Effect

As happened before with the older one or the linter, a style guide may mislead developers and if they are too religious and unable to think about some point, the quality of the result code may be worse rather than better.
I have talked about JSLint already too much in this blog so ... let's bring the topic in track and see what Felix may re-consider.

Quotes

Current status: Use single quotes, unless you are writing JSON
A common mistake for JavaScript developers, specially those with PHP, Ruby, or other scripting languages background, is to think that single quotes are different from double quotes ... sorry to destroy this myth guys but in JavaScript there is no difference between single and double quotes.
Once you get this, you must think that if by standard definition JSON uses double quotes there is no reason to prefer single quotes over double.
What should this difference tell us, that one is JSON and one is JavaScript for node.js? Such assumption is a bit ridiculous to me since JSON is JavaScript, nothing more, nothing less.
More over, I am not a big fan of HTML strings inside JS files because of separation of concerns, and I'll come back on it, but in English, as well as in many other languages, the single quote inside a string is quite common, isn't it?
Accordingly, are you really planning to prefer 'it\'s OK' over "it's OK"?
And if the suggested editor, in the guide itself, is one with supported syntax highlight, shouldn't we simply trust the highlighted string in order to recognize it, rather than distinguish between JSON and non JSON strings?
Shouldn't we write comfortably without silly rules, as Felix says in another point, so that we can chose whatever string delimiter we want accordingly with the content of the string?
Last, but not least, for those convinced that in HTML there is any sort of difference between single and double quotes, I am sorry to, once again, destroy this myth since there is no difference in HTML between quotes ... so, once again, chose what you want and focus on something else than distinction between JSON and non JSON strings ...

Variable declarations

Current status: Declare one variable per var statement, it makes it easier to re-order the lines. Ignore Crockford on this, and put those declarations wherever they make sense.
If there is one thing that I always agreed with Crockford is this one: variable declared at the beginning of the function, with the exception for function declarations that should be before variable declarations, and not after.
The reason is quite simple: wherever we declare a variable, this will be available at the very beginning of the scope so which place can be better than the beginning of the scope itself to instantly recognize all local scope variables rather than scrolling the whole closure 'till the end in order to find that name that completely screwed up the whole logic in the middle because is shadowing something else inside or outside the scope ?
Is that easy, I don't want to read potentially thousands of lines of code to understand which variable has been defined where, I want a single bloody place to understand what is local and what is not.
The only exception to this rule may be some temporary variable, as is the classic i used in for loops, or some tmp reference still common in for loops

// the exception ... in the middle of the code

for (var i = 0, length = something.length, tmp; i < length; ++i) {
tmp = something[i];
// do something meaningful
}

Chances that above loop will destroy our logic because of a shadowed i or tmp reference are really rare and we should use meaningful variables name.
As summary, there are only advantages on declaring variables on top, and until we have no let statements it is only risky, and dangerous for our business, to define variables in the wild.

// how you may write code
(function () {
return true;
var inTheWild = 123;
function checkTheWild() {
return inTheWild === 123;
}
}());

// how JS works in any case
(function () {

// function declarations instantly available in the scope
function checkTheWild() {
return inTheWild === 123;
}

// any var declaration instantly available as undefined
var inTheWild;

return true;

// no matters where/if we assign the value
// the inTheWild is already available everywhere
// with undefined as default value
// and before this assignment
inTheWild = 123;

// shadows should be easy to track
// top of the function is the right place to track them
}());


Constants

Current status: Constants should be declared as regular variables or static class properties, using all uppercase letters.

Node.js / V8 actually supports mozilla's const extension, but unfortunately that cannot be applied to class members, nor is it part of any ECMA standard.

I could not disagree more here ... the meaning of constant is something immutable and the reason we use constant is to believe these are immutable.
Why on earth avoid a language feature when it provides exactly what we need? Don't we want more reliable code? And what about security?
Neither in PHP we can define an object property and this does not mean we should avoid the usage of define function, isn't it?
Use const as much as you want unless the code does not have to run cross platform ( browsers included ) and if you want to define constants with objects, use the JavaScript equivalent to do that.

function define(object, property, value) {
Object.defineProperty(object, property, {
get: function () {
return value;
}
// if you really want to throw an error
// , set: function () {throw "unable to redefine " + property;}
});
}

var o = {};
define(o, "TEST", 123);
o.TEST = 456;
o.TEST; // 123

I have personally proposed cross platform ways to do the same without or within objects themselves.
As we can see there are many ways to define constants and if the argument is that const is not standard it's a weak one because nobody is planning to remove this keyword from V8 or SpiderMonkey, most likely every other browser will adopt this keyword in the future but if that won't happen, to find and replace it with var only when necessary is a better way to go: security and features, we should encourage these things rather than ignore them, imho.

Equality operator

Current status: Programming is not about remembering stupid rules. Use the triple equality operator as it will work just as expected.
No it doesn't because coercion is not about equality only.

alert("2" < 3 && 1 < "2");

Programming IS about remembering rules, that's the reason we need to know the syntax in order to code something ... right?
Coercion must be understood rather than being ignored and rules are pretty much simple and widely explained in the specs, even more simplified in this old post of mine.
If any of you developers still do this idiotic check you are doing it wrong.

// WRONG!!!!!!!!!!!!!
arg === null || arg === undefined

// THE SAFER EXACT EQUIVALENT
arg == null

You should rather avoid undefined equality in any piece of code you wrote, unless you did not create an undefined reference by yourself.
As summary, a good developer does not ignore problems, a good developer understands them and only after decide which way is the best one.
Felix is a good developer and he should promote deeper knowledge ... this specific point was really bad because it's like saying "avoid regular expressions because you don't understand them and you don't want to learn them".

Extending prototypes

Current status: Do not extend the prototypes of any objects, especially native ones
Read this article and stick an "it depends" in your screen. Nobody wants to limit JS potentials and Object.defineProperty/ies is a robust way to do things properly.
Sure we must understand that even a method like Array#empty could have different meanings ... e.g.

var arr = Array(3);
// is arr empty ?
// length is 3 but no index has been addressed

If we pollute native prototype we must agree on the meaning or we must prefix our extension so that nobody can complain if that empty was good enough or just a wrong concept.

// a meaningful Array#empty
Object.defineProperty(Array.prototype, "empty", {
value: function empty() {
for (var i in this) return false;
return true;
}
});

alert([
[].empty(), // true
Array(3).empty(), // true
[null].empty() // false
]);


Conditions

Current status: Any non-trivial conditions should be assigned to a descriptive variable
I often write empty condition on purpose and to both avoid superflous re-assignment and speed up the code.

// WRONG
object.property = object.property || defaultValue;

// better
object.property || (object.property = defaultValue):

// RIGHT
"property" in object || (object.property = defaultValue);

Why on earth would I assign that ? Same I would not do it if the condition is not reused at least twice.

if (done.something() && done.succeed) {
// done.something() && done.succeed useful
// here and nowhere else in the code
}

Accordingly with the fact spread variable declarations are a bad practice, I cannot think about creating flags all over the place for these kind of one shot checks.

Function length

Current status: Keep your functions short. A good function fits on a slide that the people in the last row of a big room can comfortably read. So don't count on them having perfect vision and limit yourself to ~10 lines of code per function.
I agree on this point but JavaScript is all about functions.
I understand the require approach does not need a closure to surround the whole module, being this already somehow sandboxed, but cross platform code and private functions or variables are really common in JS so that a module may be entirely written inside a closure.
A closure is a function so as long as this point is flexible is fine to me, also as long as the code is still readable and elegant ... I can already imagine developers writing 10 lines of functions using all 80 characters per line in order to respect this point ...



... now, that would be lame.

Object.freeze, Object.preventExtensions, Object.seal, with, eval

Current status: Crazy shit that you will probably never need. Stay away from it.
Felix here put language features together with "language mistakes".
With "use strict" directive, the one Felix should have put at the very beginning of his guideline, with statement is not even allowed.
eval has nothing to do with Object.freeze and others and these methods are part of the ES5 specification.
It is true you may not need them, to define them as something to stay away from, specially from somebody that said there are no constants for objects, is kinda limitative and a sort of non sense.
Learn these new methods, and use them if you think/want/need.

And that's all folks

13 comments:

LCID Fire said...

I am not a big fun to I am not a big fan

Andrea Giammarchi said...

ahahah ... typo fixed, thanks

trenc said...

To be correct:

»it's OK« !== »it’OK«

And only single quotes are correctly highlited by Kate (KDE Editor) - but ok, this could be their faults.

Angus Croll said...

Excellent! Amazingly I agree with almost every word. As Felix himself said: "Programming is not about remembering stupid rules".

Crockford did not become Crockford by slavishly following his own rules. He made mistakes, he experimented, he learned, he understood.

Andrea Giammarchi said...

trenc "it's OK" === 'it\'s OK' since ever in JavaScript

Unknown said...

I haven't read the original style guide, but all of the things you're calling out as 'wrong' seem to be common sense.

Quotes: It doesn't actually matter which you always use, as long as you always use one. It makes debugging easier, as you're always escaping the same things. As you'll find yourself escaping HTML more often than sentences, it makes sense to use single quotes. (as double quotes for attributes is the defacto standard)

Variable Declarations: I don't understand your point on this, your previous section on quotes talks about the 'silly rules', but then you're suggesting a silly rule of declaring variables at the top of a function. This isn't C, you can declare your variables wherever you want. Either you want conventions or you don't... don't switch between the two just so you can critique someone's style guide.

Constants: this is absurd. Just stick with the convention that every language uses... you don't need to create actual constants if you always treat constants as constants. Your define() function fixes a problem that doesn't exist.

Equality Operator: Again, the triple equality helps with the readability and debugging of code. So what if there are hacks that let you do strange things, the maintainability of the code is drastically reduced for every 'hack' you use.

I do agree that you should avoid undefined, and declare variables as null... but if you're writing code that interfaces with external dependencies, you still have to test for undefined.

'Enhancing' built-in objects: Utterly, completely, irrefutably wrong. There is no time, EVER, that you should EVER, EVER modify the prototype of a built-in object. If you want additional functionality, create a new 'class' of object to use. Your example is EXACTLY why this should never be done... .empty() should be an action, while .empty or .isEmpty() would be the accessors to determine if the array is empty.


"property" in object || (object.property = defaultValue);
Really? You're going to suggest code that ONLY works when a property is undefined, two paragraphs after you just say that you should define everything as 'null'.
Again, this is utterly wrong, view my examples to see why:
http://jsfiddle.net/3qafR/17/

Function lengths: This is industry standard. Sure, there are exceptions, it's not a hard-and-fast rule, but if you really have an issue with this recommendation, you need to reconsider writing code for a living.

Andrea Giammarchi said...

Nothing you said makes sense but feel free to use your rules.

Quotes ... no debugging is made easier with quotes

Vars on top: I explained why it makes sense to find vsrs on top. var is not let, var at the end of a function is available at the beginning so the beginning is the right place to find vriables, not in the wild

Constants: are not absurd, if you want to ensure no code can change them it's about security and reliability. const is a de facto standard too onserver side JS plus there are ways to define them with objects.
Which part is absurd

Equality: triple does not help anyone ... understanding the difference between == and === does help developers.
You apparently did not get it since you said I said to set everything as null ... I never even thought about it, improve your knowledge 'cause == null does not meen you need to set everything as null.
There's nothing to test against undefined but if you have to, == null is what you need as check.

Enhancing built in Objects is not bad in an absolute way ... the empty was an example after disambiguation.
I would not use that but that name disturbs you, set it as getter rather than value and you made my exact point I wrote there.

"property" in object once again you misunderstand accessors and the meaning of undefined.
Read this link to know more.

On Functions length you just trolled around ... what you say does not make sense neither is what I wrote.
I wonder if you actually code JS for living but it does not really matter to me.

Thanks for misunderstanding the whole post.

Unknown said...

You're right, I don't just do JavaScript for a living... I also use Java (Server-Side and Android), PHP, Python, AS3. I work on a team with other Senior SDEs that are expected to be able to read, understand and review each others' code at all times.

If JavaScripters ever want to have an ounce of respect, they need to learn how to develop maintainable software, rather than writing 'scripts'.

As it is, nearly every web dev we hire has to be taught how to write good, maintainable code. It's "Javascript Jedis" like you that give poor advice to aspiring Devs, confusing them beyond belief.

Being an engineer means understanding that convention is extremely important. Always quoting strings in the same way, never assigning a value to a variable that is IN_ALL_CAPS, keeping functions to a reasonable length... each of these practices are vital when you have to work with a team and maintain the code that you write.

And I understand that reassigning properties can be slow. Take a look at this JSFiddle Example to see how your proposed solution fails. If you're writing an API that is called by external code, you need to check for declared properties that are undefined and declared properties that are null. If it's all internal and you have tight control over what is being passed into your function, your code can work properly, but then you probably don't need to do these sorts of checks.

Andrea Giammarchi said...

You are saying constants should not be used anywhere since the practice is to do not reassign upper case variables plus you are inventing I said functions should be big plus you misunderstood the whole concept of properties access plus you used document.write ... once again, learn JavaScript, and feel free to use this blog as well to start with and read links I post or you will be yet another blind wannabe pretending to know a language because of few followed and never understood rules.

If you knew JavaScript you would not consider it "just writing script" ... I am a Senior SW Engineer since ages and I am promoting better knowledge of the JavaScript langauge for developers like you so that you will always be able to understand and maintain other developers code, inside your tiny team , or in the cloud with Open Source Softwares.

Have a nice evening/mirning

Andrea Giammarchi said...

just as extra hint

Dusty Jewett said...

We're still talking about the style guide, right? Because that's what your original article is about.

I'm sure that the things you mention are ways to optimize pieces of code, but they come at a price in readability and maintainability. These are the things that should be of primary concern in a Style Guide.

And you're right, I seem to have misread your Functions comment, and assumed you were calling it out because you disagreed with it. Your section does make it seem like you don't think it should be in the style guide, either way.

I still think your "Right" way of default assignment is a terrible suggestion to include in a style guide. There are so many ways that bad things can happen (null, undefined, false values)... It shouldn't be the 'default' way of coding it. The "Better" way is the "Right" way, and the "Right" way is the "Optimized after you've ensured that things can't possibly go wrong" way.

And please forgive my shortcuts in the jsfiddle... I just wanted a bare-bones example of how things go wrong.


My primary concern is that JS Developers fall neatly into two camps... 80% seem to be the ones that think that a 6000 line .js file with 300 some global variables is OK (an example of code I inherited from someone who called himself a "Senior Engineer"), and 20% who are so deep into the language that they have separate optimizations for the different JS Engines.

The 20% don't need a style guide. The style guide is for people who forget whether they should have underscores_in_variables or use camelCase. Its about establishing a better baseline.

Andrea Giammarchi said...

3 comments to start making sense and put a name on your sentences, you got promoted from troll to blog follower: congratulations!

trenc said...

@Andrea Giammarchi

"it's OK" === 'it\'s OK' since ever in JavaScript

Yes, but that’s not what I mean.

See my post: You write »It's OK« with a single quote. This is common but wrong. It should be an apostrophe ’. so no escaping i required.