Code Guidelines

[Garrett Smith]

I'm putting together a couple of documents:

1) code guidelines
2) code review guidelines

The goals are to help make for better code reviews here and to help
debate on assessing javascript code quality.

I'd like to start with my outline of code guidelines and get some
feedback on it.

Rich Internet Application Development Code Guildelines (Draft)

Problems:

Markup:
* Invalid HTML
* sending XHTML as text/html
* xml prolog (cause problems in IE)
* javascript: pseudo protocol
* Not escaping ETAGO.
Replace: "</" + "script>"
with: "<\/script>";
Replace: "</td>";
with: "<\/td>";

CSS:
* invalid css
* classNames that do not have semantic meaning

Script:
Variables:
* Global variables
* Undeclared variables
* non-descriptive name

Methods:
* initialization routines that loop through the DOM
* methods that do too much or have side effects
* long parameter lists
* non-localized strings
* inconsistent return types
Methods should return one type; returning null instead of an Object
may be a fair consideration.

Strategies:
* Modifying built-in or Host object in ways that are either error
prone or confusing (LOD).
* Use of non-standard or inconsistent ecmascript methods (substr,
escape, parseInt with no radix).
* Useless methods, e.g.
goog.isDef = function(val) {
return val !== undefined;
};
Instead, replace useless method with typeof check:
if(typeof val === "undefined"){ }

Strings:
* use of concatenation to repeatedly (repeatedly create and
discard temporary strings)
Instead use String.prototype.concat(a, b, c) or htmlBuf.push(a, b, c);

Loops:
* Loop body uses a long chain of identifiers
Replace long chain of identifiers with variable.
* Loop body traverses over elements to modify the style or event of
each element.
Solution:
Styles: Replace a loop that applies styles to descendants by adding
a class token to the nearest common ancestor and add a style rule to
the style sheet with the selector text of #ancestor-id .descendant-class.
Events: Use event delegation.

Statements:
* Use of == where === should be used
* Boolean conversion of Host object (sometimes Error-prone)
* Boolean conversion of value that may be falsish, e.g. if(e.pageX)
* useless statements (e.g. typeof it == "array")

RegularExpressions
Be simple, but do not match the wrong thing.

Formatting:
* Tabs used instead of spaces.
* Long lines

DOM:
* Use of poor inferences; browser detection
* Use of non-standard approach or reliance on hacks
* Branching for multiple conditions when common approach works in more
browsers.
* If an approach requires several conditions to branch, look for a
different approach that works in all tested browsers.
* Traversing the dom on page load (slow) especially traversing DOM
using hand-rolled query selector.

Instead of traversing the DOM, use Event delegation.
Comments:

* Inaccurate statement in comment
* Comment doesn't match what code does

 

[Erwin Moller]

Garrett Smith schreef:

I'm putting together a couple of documents:

1) code guidelines
2) code review guidelines

The goals are to help make for better code reviews here and to help
debate on assessing javascript code quality.

I'd like to start with my outline of code guidelines and get some
feedback on it.

<snipped>

Hello Garrett,

Sounds very interesting.
I have read through it and I hope you will also give the reason for the
do's and don'ts.
Maybe with good examples and poor examples and some background information.

That would be a great document for starters and JavaScript programmers
with moderate experience.

There is so much (poor) advice on the net when it comes to JavaScript,
that is is often hard to find the right approach if you are not a
specialist (in which case you don't need to search the web).

Good luck!

Regards,
Erwin Moller


--
"There are two ways of constructing a software design: One way is to
make it so simple that there are obviously no deficiencies, and the
other way is to make it so complicated that there are no obvious
deficiencies. The first method is far more difficult."
-- C.A.R. Hoare

 

[Dmitry A. Soshnikov]

Good parts, thanks.

  * inconsistent return types
Methods should return one type; returning null instead of an Object
may be a fair consideration.

Not necessarily. Method can be sort of factory, which returns
different type values. Or, e.g. "analysis returns" when check is
negative so simply exit from the function - there's no need to analyze
this code more; in this case nor value neither its type is so
essential (simple "return;" can be used without explicit value -
implicit undefined), but maybe "return null;" will be better. From the
other hand - why "return null" if the return statement at the end
returns e.g. number (will you use "return -1" then)?

// just exit, no need to analyze
// code bellow if first check is not pass
if (!someCheck)
return;

// other calculations

return 100;

goog.isDef = function(val) {
   return val !== undefined;};

Instead, replace useless method with typeof check:
if(typeof val === "undefined"){ }

From the speed point of view, calling function (establishing the new
context and so on) sure is less effective. From the abstraction point
of view, shorter wrapper can be better - with the same check inside -
everyone chooses. I use typeof val === "undefined", but not against
simple short wrappers.

/ds

 

[John G Harris]

Formatting:
* Tabs used instead of spaces.

Spaces are preferred, I hope.

Comments:

* Inaccurate statement in comment
* Comment doesn't match what code does

Avoid too many comments. (Let the code speak for itself).

Remember to update comments when the code is changed.

John

 

[Asen Bozhilov]

  * Boolean conversion of Host object (sometimes Error-prone)

//Boolean conversation host object in JScript
var xhr = new ActiveXObject('Microsoft.XMLHTTP');
window.alert(Object.prototype.hasOwnProperty.call(xhr, 'open')); //
true
try {
Boolean(xhr.open);
}catch(e)
{
window.alert(e instanceof TypeError); //true
window.alert(e); //Object doesn't support this property or method
}
window.alert(typeof xhr.open); //unknown

 

[Dmitry A. Soshnikov]

Statements:
  * Use of == where === should be used

if(typeof val === "undefined"){ }

typeof returns always string, so there's no any difference using == or
=== in this case (algorithms are equivalent: word for word); == is
less on one symbol, you can use typeof val == 'undefined'.

/ds

 

[Asen Bozhilov]

typeof returns always string, so there's no any difference using == or
=== in this case (algorithms are equivalent: word for word); ==is
less on one symbol, you can use typeof val == 'undefined'.

Yes, here you are absolutely right. But `==' can be harmful especially
when `EqualityExpression` and `RelationalExpression` ares from
different type, because one of them will be converted to primitive
value.

| 11.9.3 The Abstract Equality Comparison Algorithm

| 1. If Type(x) is different from Type(y), go to step 14.
| [...]
| 21. If Type(x) is Object and Type(y) is either String or Number,
| return the result of the comparison ToPrimitive(x) == y.

ToPrimitive for `object` call internal [[DefaultValue]] of passed
`object'. If `object' properties `toString' and `valueOf' is not a
objects [[DefaultValue]] throw TypeError.

e.g.

var o = {toString : null, valueOf : null};
try {
o == '';
}catch(e)
{
window.alert(e instanceof TypeError);
}

 

[Dmitry A. Soshnikov]

typeof returns always string, so there's no any difference using ==or
=== in this case (algorithms are equivalent: word for word); == is
less on one symbol, you can use typeof val == 'undefined'.

Yes, here you are absolutely right. But `==' can be harmful especially
when `EqualityExpression` and `RelationalExpression` ares from
different type, because one of them will be converted to primitive
value.

| 11.9.3 The Abstract Equality Comparison Algorithm

| 1. If Type(x) is different from Type(y), go to step 14.
| [...]
| 21. If Type(x) is Object and Type(y) is either String or Number,
| return the result of the comparison ToPrimitive(x) == y.

ToPrimitive for `object` call internal [[DefaultValue]] of passed
`object'. If `object' properties `toString' and `valueOf' is not a
objects [[DefaultValue]] throw TypeError.

e.g.

var o = {toString : null, valueOf : null};
try {
        o == '';}catch(e)

{
        window.alert(e instanceof TypeError);

}

Thanks, I know about that. But I mentioned about exactly typeof
operator and string 'undefined' (of whatevery) but not about o ==
'' And from this point of view == would be enough; === in thiscase
is obsolete.

/ds

 

[Asen Bozhilov]

//Boolean conversation host object in JScript
var xhr = new ActiveXObject('Microsoft.XMLHTTP');
Boolean(xhr.open);

Garrett do you have any others example where Boolean conversion throw
Error?

In my example error will be throwing from internal [[Get]] method of
`object' referred from `xhr'.

var xhr = new ActiveXObject('Microsoft.XMLHTTP');
try {
var a = xhr.open;
}catch(e)
{
window.alert(e); //Object doesn't support this property or method
}

Here isn't the problem in type conversion. The problem is before that.
That example can show it own implementation of [[Get]] and [[Put]]
methods from that host object.

var xhr = new ActiveXObject('Microsoft.XMLHTTP');
try {
xhr.open = null;
}catch(e)
{
if (e instanceof ReferenceError)
{
window.alert('8.7.2 PutValue(V) throw ReferenceError');
}
window.alert(e); //TypeError
}

Can you show better example, because ECMA3 9.2 ToBoolean explicit
say:

| The operator ToBoolean converts its argument to a value of
| type Boolean according to the following table:
| [...]
| Object true

Abstract for me that mean:

function ToBoolean(value)
{
if (Type(value) === Object)
{
return true;
}
}

So at the moment i can't see where explicit or implicit conversion of
any object Native or Host can produce error.

 

[Garrett Smith]

Yes, here you are absolutely right. But `==' can be harmful especially
when `EqualityExpression` and `RelationalExpression` ares from
different type, because one of them will be converted to primitive
value.

Yes, Dmitry is correct, you're both right. I wasn't explicit enough, or
used terminology that was open to interpretation.

Changed to:
* Use of == where strict equality is required
Where strict equality is required, the strict equality operator must be
used. The strict equality operator should always be used to compare
identity.

Example:-
// Do not use == to compare identity.
if (element == target) {
return true;
}


In the case of checking to see if a value can be null or undefined, I
don't have any problem with:-

if(v == null) {

}

Others do not using == to loose check. JSLint will error on that, I
believe.

[...]

 

[Garrett Smith]

Garrett do you have any others example where Boolean conversion throw
Error?

No, you're right, I was mistaken. That should be replaced with something
appropriate.

In my example error will be throwing from internal [[Get]] method of
`object' referred from `xhr'.

var xhr = new ActiveXObject('Microsoft.XMLHTTP');
try {
var a = xhr.open;
}catch(e)
{
window.alert(e); //Object doesn't support this property or method
}

Here isn't the problem in type conversion. The problem is before that.
That example can show it own implementation of [[Get]] and [[Put]]
methods from that host object.

You're right about it not being ToBoolean. The error is from accessing
the property. I had believe the error was caused by type conversion.

Without doing anything with xhr.open method:-

javascript: new ActiveXObject('Microsoft.XMLHTTP').open;
Error: Object doesn't support this property or method.

Thanks for pointing that out.

| * Error-prone handling of Host objects.
| Potentially disconnected nodes behave differently
| javascript: void document.createElement("div").offsetParent;
| Unspecified Error in IE.
| Instead, use typeof to avoid undefined values or IE "unknown" types:
| var div = document.createElement("div");
| isOffsetParentSafe = !/^un/.test(typeof div.offsetParent);

 

[Thomas 'PointedEars' Lahn]

Changed to:
* Use of == where strict equality is required
Where strict equality is required, the strict equality operator must be
used.

That goes without saying.

The strict equality operator should always be used to compare
identity.

Example:-
// Do not use == to compare identity.
if (element == target) {
return true;
}

Define `identity'. It makes no difference whether `==' or `===' is used if
both values are object references (as in "objects have identity"). To be
exact, it makes no difference whether `==' or `===' is used if both values
are of the same internal type, e.g., Object. That is, it makes only a
little difference with regard to compatibility as `==' is slightly more
compatible than `==='.

In the case of checking to see if a value can be null or undefined, I
don't have any problem with:-

if(v == null) {

}

ACK. Then again, one seldom needs that because with a function one wants to
differentiate whether an argument was not passed, whereas the value of the
argument were `undefined', or whether `null' was passed. In all other cases
either host objects are involved and a `typeof' test is safer, or native
objects are involved and a type-converting test is more efficient.

Others do not using == to loose check. [...]

Parse error.


PointedEars

 

[RobG]

I'm putting together a couple of documents:

1) code guidelines
2) code review guidelines

The goals are to help make for better code reviews here and to help
debate on assessing javascript code quality.

I'd like to start with my outline of code guidelines and get some
feedback on it.

Rich Internet Application Development Code Guildelines (Draft)

Problems:

Markup:
* Invalid HTML
* sending XHTML as text/html
* xml prolog (cause problems in IE)
* javascript: pseudo protocol
* Not escaping ETAGO.
Replace: "</" + "script>"
with: "<\/script>";
Replace: "</td>";
with: "<\/td>";

I don't understand the issue here, is it specifically with the TD end
tag? Or is it more generally with end tags in HTML sent as an XHR
response?

A suitable alternative may be to omit the closing TD tag altogether
(valid in HTML 4.01 and draft HTML 5).

CSS:
* invalid css
* classNames that do not have semantic meaning

While that comment is OK for class values used for CSS, it should be
noted that class names are not intended for CSS only.


[...]

RegularExpressions
Be simple, but do not match the wrong thing.

Not sure what that means.

 

[Garrett Smith]

That goes without saying.

It is a true statement, but not obvious to all. The recent discussion on
Qooxdoo had an example of using == to compare two objects. I commented
that that comparison should use strict equality. That avoids cases
where, say, comparing a and b, a is null and b is empty string.

Same goes with unqualified assignment.

Define `identity'.

The same unique object. As in, "if source is the target..."

if(source === target) {

}

and not:-

if(source == target) {

}

The strict equality tests leaves no ambiguity to the reader of the code
that the intent is to check identity. There is no room for considering
what happens when one operand is a different type.

It makes no difference whether `==' or `===' is used if

both values are object references (as in "objects have identity"). To be
exact, it makes no difference whether `==' or `===' is used if both values
are of the same internal type, e.g., Object. That is, it makes only a
little difference with regard to compatibility as `==' is slightly more
compatible than `==='.

I think you meant "built-in" where you wrote "internal type". While it
is true that comparing two native ES objects using == does not result in
conversion, it still allows the possibility where when one side of the
operation is not an object, then conversion will happen. It may seem
unlikely to occur, but the added insurance of using === doesn't hurt. It
can actually result in nanosecond performance increase in IE.

For host object, == can be true while === is false.

javascript: alert( self === window );// false
javascript: alert( self == window ); // true

[...]

 

[David Mark]

It is a true statement, but not obvious to all. The recent discussion on
Qooxdoo had an example of using == to compare two objects. I commented
that that comparison should use strict equality. That avoids cases
where, say, comparing a and b, a is null and b is empty string.

Do you mean b is undefined?

null != ''

 

[David Mark]

I don't understand the issue here, is it specifically with the TD end
tag? Or is it more generally with end tags in HTML sent as an XHR
response?

No, it refers to markup in script (e.g. document.write calls).
Nothing to do with the tag type.

A suitable alternative may be to omit the closing TD tag altogether
(valid in HTML 4.01 and draft HTML 5).



While that comment is OK for class values used for CSS, it should be
noted that class names are not intended for CSS only.

I try to avoid that.

[...]

RegularExpressions
   Be simple, but do not match the wrong thing.

Not sure what that means.

Seems overly simplified.

 

[Garrett Smith]

[...]

I don't understand the issue here, is it specifically with the TD end
tag? Or is it more generally with end tags in HTML sent as an XHR
response?

* Not escaping ETAGO, or using concatenation to escape ETAGO.
Inline script must not contain the character sequence "</"

http://www.w3.org/TR/WD-script-970314

When concatenating strings of HTML in an inline script, a backslash
should appear before the solidus symbol in the sequence "</", resulting
in "<\/", and not - "<" + "/" -.

A suitable alternative may be to omit the closing TD tag altogether
(valid in HTML 4.01 and draft HTML 5).



While that comment is OK for class values used for CSS, it should be
noted that class names are not intended for CSS only.

True. Non-semantic class or ID lacks meaning (HTML and CSS). The same
applies to ID.

Instead, class and id should be meaningful, so the code is easier to
understand.

Bad:
..errorButton, #warningMessage

Good:

[...]

RegularExpressions
Be simple, but do not match the wrong thing.

Not sure what that means.

The context of a Regular Expression is is as important as what it can
match. A complex regular expression is harder to understand than a
simple one and so simple regular expressions are preferred. It is OK the
expression can match the wrong thing, just so long as the context in
which it is used precludes or handles that.

// Wrong: Matches 137, 138...
ARROW_KEY_EXP : /37|38|39|40/;

// Right: matches only arrow keys.
ARROW_KEY_EXP : /^(?:37|38|39|40)$/;

// Simple: Can match the wrong thing, but that can be handled.
var iso8601Exp = /^\s*([\d]{4})-(\d\d)-(\d\d)\s*$/;

Trying to match leap years would be excessively complex.
Instead, the date validation can be addressed where the expression is used.

 

[Garrett Smith]

Do you mean b is undefined?

Yes, of course!

null != ''

Of course, I know that. Thanks for pointing it out though.

 

[Garrett Smith]

It is a true statement, but not obvious to all. The recent discussion on
Qooxdoo had an example of using == to compare two objects. I commented
that that comparison should use strict equality. That avoids cases
where, say, comparing a and b, a is null and b is empty string.

Correction:
where, say, comparing - a - is null and - b - is undefined.

 

[Asen Bozhilov]

RegularExpressions
Be simple, but do not match the wrong thing.

I ever wondering about using RegExp for type checking.

/^\s*\bfunction\b/.test("" + f);

That will be check implementation depended string returned from
f.toString(); for as function, and that checking can be falsish.

e.g.

var o = {
toString : function()
{
return 'function function';
}
};
window.alert(/^\s*\bfunction\b/.test("" + o)); //true