G
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
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
And from this point of view == would be enough; === in thiscase