This page is a helper guideline for all project developers who request code review sessions.

The developer is responsible for inherited code. He is responsible for the code he commits regardless its author. He must provide a fair quality code with graceful degradation (disabling javascript will not affect the basic functionality)

Efficiency review

Avoid repetitive code

Examples of how not to do. Rather rely on associative arrays.

WRONG

...      
case 'be': country_subsite = 'belgium'; break;
      case 'bg': country_subsite = 'bulgaria'; break;
      case 'hr': country_subsite = 'croatia'; break;
      case 'cy': country_subsite = 'cyprus'; break;
      case 'cz': country_subsite = 'czech-republic'; break
...

Use an efficient for/while loop

WRONG

jQuery('#panel_0').toggler({toggler_element: '.item-0 > a.link', toggled_element: '.item-0b', delay: 100});
jQuery('#panel_1').toggler({toggler_element: '.item-1 > a.link', toggled_element: '.item-1b', delay: 100});
jQuery('#panel_2').toggler({toggler_element: '.item-2 > a.link', toggled_element: '.item-2b', delay: 100});
jQuery('#panel_3').toggler({toggler_element: '.item-3 > a.link', toggled_element: '.item-3b', delay: 100});
jQuery('#panel_4').toggler({toggler_element: '.item-4 > a.link', toggled_element: '.item-4b', delay: 100});
jQuery('#panel_5').toggler({toggler_element: '.item-5 > a.link', toggled_element: '.item-5b', delay: 100});

WRONG jQuery(‘#search-ban’).unbind(‘click’); jQuery(‘#search-ban’).bind(‘click’, function(){

Solution

Chaining

$(selector)
  .unbind(...)
  .bind(... )
  .text(...);

Hidden Classes optimization

V8 has hidden types created internally for objects at runtime; objects with the same hidden class can then use the same optimized generated code.

    Initialize all object members in constructor functions (so the instances don't change type later)

    Always initialize object members in the same order
    Hidden class optimization
    function Point(x, y) {
      this.x = x;
      this.y = y;
    }
     
    var p1 = new Point(11, 22);
    var p2 = new Point(33, 44);
    // At this point, p1 and p2 have a shared hidden class
    p2.z = 55;
    // warning! p1 and p2 now have different hidden classes!

Numbers

V8 uses tagging to represent values efficiently when types can change. V8 infers from the values that you use what number type you are dealing with. Once V8 has made this inference, it uses tagging to represent values efficiently, because these types can change dynamically. However, there is sometimes a cost to changing these type tags, so it’s best to use number types consistently, and in general it is most optimal to use 31-bit signed integers where appropriate. Numbers ar i = 42; // this is a 31-bit signed integer var j = 4.2; // this is a double-precision floating point number

Prefer numeric values that can be represented as 31-bit signed integers.

Arrays

In order to handle large and sparse arrays, there are two types of array storage internally:

Fast Elements: linear storage for compact key sets
Dictionary Elements: hash table storage otherwise

It’s best not to cause the array storage to flip from one type to another. Therefore:

Use contiguous keys starting at 0 for Arrays
Don't pre-allocate large Arrays (e.g. > 64K elements) to their maximum size, instead grow as you go
Don't delete elements in arrays, especially numeric arrays
Don't load uninitialized or deleted elements: ```js a = new Array(); for (var b = 0; b < 10; b++) {   a[0] |= b;  // Oh no! } //vs. a = new Array(); a[0] = 0; for (var b = 0; b < 10; b++) {   a[0] |= b;  // Much better! 2x faster. } ```

Also, Arrays of doubles are faster - the array’s hidden class tracks element types, and arrays containing only doubles are unboxed (which causes a hidden class change).However, careless manipulation of Arrays can cause extra work due to boxing and unboxing - e.g.

var a = new Array();
a[0] = 77;   // Allocates
a[1] = 88;
a[2] = 0.5;   // Allocates, converts
a[3] = true; // Allocates, converts

is less efficient than:

//Better way if doing

var a = [77, 88, 0.5, true];

because in the first example the individual assignments are performed one after another, and the assignment of a[2] causes the Array to be converted to an Array of unboxed doubles, but then the assignment of a[3] causes it to be re-converted back to an Array that can contain any values (Numbers or objects). In the second case, the compiler knows the types of all of the elements in the literal, and the hidden class can be determined up front. Therefore:

Initialize using array literals for small fixed-sized arrays
Preallocate small arrays (<64k) to correct size before using them
Don't store non-numeric values (objects) in numeric arrays
Be careful not to cause re-conversion of small arrays if you do initialize without literals.

More on the resources https://www.html5rocks.com/en/tutorials/speed/v8/ Functions that should NOT be used

The use of eval() will cause a performance and cache issue, we should not use eval() in our custom code. setTimeout(), setInterval() should execute only well defined code that doesn’t depends on the user input. String translation

All strings in JavaScript files SHOULD be wrapped in Drupal.t(), which is an equivalent of the well-known t() function.

Likewise, there is an equivalent to format_plural(), named Drupal.formatPlural(). // Wrong var myCustomString = ‘My text here.’;

// Right var myCustomString = Drupal.t(‘My text here.’); File closure and strict mode

It is important that all JavaScript code be declared inside a closure wrapping the whole file and this closure MUST be in strict mode. Strict mode will prevent unexpected errors and is a Drupal Standard.

/**
 * @file
 */
(function () {
 
  "use strict";
 
  // All your custom code should be placed here.
 
})();

Variable naming

The variable name must follow lowerCameCase convention except for constants or contructors. If the variable holds a jQuery object, then we must prefix the name a dollar sign ($). Due the use of strict mode all variable must be initialized.

Bad example of naming

var my_custom_variable;
var CustomVar;
var form = $('#my-form');

Good example of naming

var myCustomVariable;
var customVar;
var $form = $('#my-form');

Reliability detection

A. Type detection

WRONG

if (typeof someVariable == 'undefined') {

GOOD

if (typeof someVariable === 'undefined') {

Performance review

Multiple identically selectors.

Problem: Expensiveness of DOM scanning

jQuery('.panel-indicator-second').addClass('_hide_');
jQuery('#panel-indicator-first').removeClass('_hide_');
jQuery('#panel-indicator-first').find('img').css('opacity', 0.5);
jQuery('#panel-indicator-first').find('a').unbind('click');
jQuery('#panel-indicator-first').find('a').css('cursor', 'default').bind('click', function(){return false;})

A performance fault. The DOM is being scanned several times due to the fact that #panel-indicator-first is read as long as you call jQuery('#panel-indicator-first').

Solution

var indicator = "#panel-indicator-first";
jQuery(indicator).

Security review

We are reviewing your code against Cross Site Scripting (XSS) and Cross Site request Forgery (CSRF).

Your server side should not depend on security provided by Javascript.

Treat all user input as completely untrusted data. HTML injections

A very common example is to inject malicious code into a form. It is as just as vulnerable as server side HTML injection.

WRONG

$('table').append('<tr title="'+row.title+'"><td>'+row.description+'</td></tr>');

Instead use available DOM methods:
$('table').append(
    $('<tr>', {title: row.title}).append(
        $('<td>', {text: row.description})
    )
);

There are html tags that can be used for executing the XSS: 

<!-- <img> tag XSS -->
<img src="javascript:alert("XSS");">
<!--  tag XSS using lesser-known attributes -->
<img dynsrc="javascript:alert('XSS')">
<img lowsrc="javascript:alert('XSS')">
 
 
<!-- <iframe> tag XSS -->
<iframe src=”http://evil.com/xss.html”>
 
 
<!-- <input> tag XSS -->
<input type="image" src="javascript:alert('XSS');">
<!-- <table> tag XSS -->
<table background="javascript:alert('XSS')">
<!-- <td> tag XSS -->
<td background="javascript:alert('XSS')">

More on the https://www.acunetix.com/websitesecurity/cross-site-scripting/ It is important to understands that values of this attributes should treated with care and with proper sanitization. In general we should avoid user input in this attributes of tags only well defined code and value should be allowed. Same applies for the locations based code like location, location.href, document.URL, location.hash. For example:

document.thedom_reference.onchange = function() { var url = document.theform.reference[id].value; window.location.href = url; }

SVG

SVG should not be treated as image but as a application that can have script and logic there. Every SVG needs to be checked if there is any code or external script that is executed. Here is the couple of examples:

<a xlink:href="javascript:alert(location)">
<?xml version="1.0" standalone="no"?><!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd"><svg onload="alert(1)" xmlns="http://www.w3.org/2000/svg"><defs><font id="x"><font-face font-family="y"/></font></defs></svg>
<set xlink:href="#x" attributeName="xlink:href" begin="6s" to="javascript:alert(3)" />

On this example we can see how the javascript code can be injected on different way. Each alert represent injected code.

More resources: https://html5sec.org/#svg

Drupal compliance

Drupal has a specific way of dealing with Javascript. It contextualizing thing based on Drupal.behaviors and pass PHP variables to Javascript via Drupal.settings object. Local storage

localStorage, WebSQL, IndexDB should not contain any sensitive data since in general it can not be see as secure even though it follows the SOP
useful links: https://developer.mozilla.org/en-US/docs/Web/API/Web_Storage_API   https://developer.mozilla.org/en-US/docs/Web/API/IndexedDB_API

Cookies

session cookies and any cookies that are used only by server should be created using HttpOnly parameter to prevent accessing cookie by JavaScript 
cookies should have correct domain and path set to avoid accessing by other websites

  https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies#Security SOP

The same-origin policy (SOP) restricts how a document or script loaded from one origin can interact with a resource from another origin. It is a critical security mechanism for isolating potentially malicious documents. It is important to understand when this policy can be softened and for what reason. The command document.domain can effect the SOP so it is important to see and understand reason why it is implemented. There are some JavaScript mechanism that don’t follow SOP. More details on next paragraph.

More on this link: https://developer.mozilla.org/en-US/docs/Web/Security/Same-origin_policy Data exchange and connections in Javascript

JavaScript allows to connect to other resources and exchange data. It is usually done with websocket, XMLHttpRequest or fetch API. All of them can avoid SOP so it is important to find those connections to other resources and examine what is the purpose of their existence. This should be applied to custom code as well to libraries. It should not be allowed to exchange data from untrusted resources in general. Possible exception can be only if data are well sanitized and use on very clear and confined way. The iframe in general follows the SOP and parent DOM is not accessible but there is function window.postMessage() that can exchange data between parent and iframe so the careful examine needs to be done if this commands is used. It should not be allowed iframe to effect the DOM of parent.

Some code examples:
// Fetch API
var myImage = document.querySelector('img');
 
fetch('flowers.jpg').then(function(response) {
  return response.blob();
}).then(function(myBlob) {
  var objectURL = URL.createObjectURL(myBlob);
  myImage.src = objectURL;
});
 
 
fetch('https://example.com', {
  credentials: 'include' 
})
 
 
//WebSocket
 var ws = new WebSocket("ws://127.0.0.1:8080/");
 ws.onopen = function() {
   alert("Opened!");
   ws.send("Hello Server");
 };
 ws.onmessage = function (evt) {
   alert("Message: " + evt.data);
 };
ws.onclose = function() {
  alert("Closed!");
};
ws.onerror = function(err) {
  alert("Error: " + err);
};
//XMLHttpRequest
function reqListener () {
  console.log(this.responseText);
}
 
var oReq = new XMLHttpRequest();
oReq.addEventListener("load", reqListener);
oReq.open("GET", "http://www.example.org/example.txt");
oReq.send();