14 trucs !@# dans votre JS

Par Christophe Porteneuve • Publié le 26 décembre 2014 • 10 min

Globales en pagaille

Faute d’enrobage dans un module (qu’il s’agisse d’un module formel, type CommonJD, AMD ou ES6, ou d’une simple IIFE), votre script pollue l’espace de noms global avec tout plein d’identifiants, et pas que ceux que vous aviez volontairement exposés.

Il y a entre 500 et 1 000 identifiants globaux dans votre navigateur, suivant le type et la version de celui-ci, inutile d’en rajouter, les gens…

Pour éviter ça, utilisez des modules, ou a minima enrobez votre code dans une Immediately-Invoked Function Expression (IIFE) et n’exposez que le strict nécessaire (souvent rien) :

;(function () {
  // Votre code ici.
  // Et tant qu'à faire, collez-y un 'use strict' au début ;-)
})()

Et surtout, n’oubliez pas les var pour déclarer vos variables. Sans quoi, fuite de global (mais le mode strict, tout comme les linters de type JSHint, vont vous gronder si vous en oubliez).

Identifiants flemmards

Ce n’est pas parce qu’on n’a pas toujours une complétion haut de gamme pour JavaScript qu’il faut retomber sur des identifiants pourris genre i, j, k, xs, gpco et que sais-je encore…

Même avec un éditeur lambda, on a au moins la complétion sur les identifiants du fichier. Et la plupart des éditeurs ont du multi-curseur ou du Local Rename. Préférez donc, par exemple, index, column, row, positions, getParentContainerOffset…

Votre code sera lu bien plus souvent qu’il sera écrit. La lisibilité des identifiants est indispensable pour une maintenabilité correcte.

Blocs / callbacks imbriqués trop profond

Trop de développeurs oublient l’intérêt des court-circuits de type return, break et continue, et imbriquent les blocs jusqu’à plus soif :

function processCredentials() {
  if (userLoggedIn) {
    var cred
    while ((cred = getNextCredentials())) {
      if (verifyCredentials(cred)) {
        if ('oauth' === cred.type) {
          checkOAuthExpiry(cred)
          return
        } else {
          extendCredentials(cred)
        }
      }
    }
  }
}

Je vous conseille de configurer votre JSHint avec "maxdepth": 3 pour vous faire asticoter quand vous tombez dans ce genre de travers.

Par exemple, le code précédent peut être réécrit comme ceci :

function processCredentials() {
  if (!userLoggedIn) {
    return
  }

  var cred
  while ((cred = getNextCredentials())) {
    if (!verifyCredentials(cred)) {
      continue
    }

    if ('oauth' === cred.type) {
      checkOAuthExpiry(cred)
      return
    }

    extendCredentials(cred)
  }
}

Si malgré tous vos efforts, vous avez encore plus de 3 niveaux d’imbrication, c’est sans doute un signe que votre fonction fait trop de choses, et qu’elle devrait être découpée en sous-fonctions.

On retrouve un principe similaire avec le callback hell, également connu sous le nom de Pyramid of Doom :

function postComment(req, res) {
  req.getUser(function (user) {
    user.getPost(req.params.id, function (post) {
      post.comments.create(req.body, function (comment) {
        res.status(201).json({ id: comment.id })
      })
    })
  })
}

Il existe de nombreuses manières de résoudre ce genre de problèmes. Suivant le contexte et les usages souhaités, on peut passer par une bibliothèque comme Async, recourir aux promesses, ou simplement découper en sous-fonctions déclarées localement, ce qui a l’avantage supplémentaire de les nommer, améliorant ainsi les stack traces au débogage :

function postComment(req, res) {
  req.getUser(fetchUserPost)

  function fetchUserPost(user) {
    user.getPost(req.params.id, createComment)
  }

  function createComment(post) {
    post.comments.create(req.body, respond)
  }

  function respond(comment) {
    res.status(201).json({ id: comment.id })
  }
}

Les déclarations de fonctions étant hoisted (interprétées avant tout le reste de la portée), on peut appeler une fonction « avant » sa déclaration locale, ce qui permet de dérouler l’algo principal avant les sous-fonctions, ou simplement de refléter le déroulé du code « de haut en bas ».

Si certaines fonctions ont besoin d’infos acquises aux niveaux d’appel précédents, il suffit par exemple de stocker ces infos dans la closure principale. Par exemple :

function postComment(req, res) {
  req.getUser(fetchUserPost)

  var gUser

  function fetchUserPost(user) {
    gUser = user
    user.getPost(req.params.id, createComment)
  }

  function createComment(post) {
    post.comments.create(_.extend(req.body, { author: gUser }), respond)
  }

  function respond(comment) {
    res.status(201).json({ id: comment.id })
  }
}

Fonctions anonymes non triviales

La plupart des développeurs ne nomment pas leurs fonctions de rappel (gestionnaires d’événements, timers, etc.), y compris quand celles-ci contiennent du code non trivial (ou appellent des fonctions non triviales).

Pour rappel, le nom d’une fonction, accessible via sa propriété name, est ce qui apparaît entre function et (. Du coup, function( crée une fonction anonyme.

Les dernières versions d’ECMAScript font évoluer la définition automatique du nom pour prendre en compte toute une série de contextes lexicaux (propriété d’objet, affectation à une variable, etc.) et réduire le nombre d’anonymes de façon organique. Les navigateurs modernes tentent des approches similaires. Mais outre que ce n’est pas garanti sur tous les navigateurs à prendre en charge, ça ne couvre pas le cas classique de la fonction anonyme de rappel :

setTimeout(function () {
  // Peu importe la runtime, cette fonction est anonyme
}, 0)

$(document).on('keydown', function (e) {
  // Celle-ci aussi
})

localStore.batch(function (entries) {
  // Celle-là pareil
})

casper
  .start('http://www.paris-web.fr/', function () {
    // Et bim, anonyme
  })
  .run(function () {
    // Blam, pareil
  })

Du coup, prenez l’habitude de nommer intelligemment vos expressions de fonctions.

C’est également pratique pour contextualiser immédiatement des fonctions associées à des propriétés standardisées, notamment dans la surcharge de fonctions héritées, par exemple dans un contexte MVC. Dans le code suivant :

module.exports = Backbone.View.extend({
  template: require('./templates/check_in'),

  afterRender:   function() { … },
  getRenderData: function() { … }
});

…même dans Chrome, dont la runtime V8 tente de nommer automatiquement les fonctions que vous avez définies, ça donnera au mieux des noms générés du genre Backbone.View.extend.afterRender. Pas terrible, car ça sera pareil pour toutes nos sous-classes de Backbone.View (et puis ce segment .extend, c’est du bruit).

Il nous suffit de les nommer correctement pour avoir directement un nom pertinent dans les piles d’appel :

module.exports = Backbone.View.extend({
  template: require('./templates/check_in'),

  afterRender:   function afterCheckInRender() { … },
  getRenderData: function getCheckInRenderData() { … }
});

Signatures à rallonge

Je pleure toujours des larmes de sang en regardant la signature de la méthode initMouseEvent définie par la spec W3C DOM Level 2 Events :

function initMouseEvent(type, canBubble, cancelable, view,
  detail, screenX, screenY, clientX, clientY,
  ctrlKey, altKey, shiftKey, metaKey,
  button, relatedTarget)

Juré.

Quand on voit une aberration pareille, on réalise à quel point, pendant longtemps, le W3C vivait dans une tour d’ivoire, totalement déconnecté de la réalité des développeurs. Ça donne quand mêmes des appels imbitables du genre :

event.initMouseEvent(
  'click',
  true,
  true,
  view,
  1,
  214,
  121,
  17,
  43,
  false,
  false,
  false,
  false,
  1,
  null
)

Lisible ? Cristallin ? Maintenable ? Non : impardonnable.

D’une façon générale, je recommande de se limiter à 3 paramètres ; au-delà, préférez un hash d’options. N’hésitez pas à ajouter un "maxparams": 3 à votre configuration JSHint pour vous rappeler à l’ordre.

Cette partie d’un article précédent vous montrera comment créer des signatures proportionnées qui tirent correctement parti de ce principe.

Expressions au lieu de déclarations (de fonctions)

À force de voir des expressions de fonctions (FE) un peu partout, de nombreux développeurs oublient que les déclarations existent, et ont l’avantage d’un nom explicite et du hoisting (elles sont analysées et mises à disposition avant tout le reste de leur portée). Il y a trop de ça :

var doStuff = function () {
  // …
}

var runTask = function () {
  // …
}

var processJob = function () {
  // …
}

Même si les navigateurs récents vont suivre les recos des dernières specs d’ECMAScript et auto-affecter un nom (en tout cas dans leurs piles d’appels) sur base des variables auxquelles la fonction est affectée, ça reste inutilement verbeux et, qui plus est, non hoisted :

// KO : ne fonctionne pas faute de hoisting (et la fonction sera anonyme
// sauf sur des navigateurs récents)
function processQueue(queue) {
  queue.forEach(processJob)

  var processJob = function (job) {
    // …
  }
}

Alors que le code suivant fonctionne et, à mon sens, est plus simple :

// OK : processJob est hoisted (et instrinsèquement nommée, qui plus est)
function processQueue(queue) {
  queue.forEach(processJob)

  function processJob(job) {
    // …
  }
}

Ma recommandation est stable depuis des années : si vous ne pouvez pas prouver par A+B que vous avez besoin d’une expression de fonction, utilisez une déclaration.

Boucles for inefficaces

La grande majorité des boucles for sont sur des tableaux, ou équivalents tableaux (Arguments, NodeList, etc.) :

// KO : inutilement lent
for (var index = 0; index < items.length; index++) {
  // …
}

Lors d’une boucle for, il est rarissime qu’on modifie la collection à la volée : ça nous obligerait à compenser l’incrémentation systématique, c’est source de bugs et vite pénible. Dans un tel cas, on aurait plutôt recours à une boucle while avec une gestion manuelle de l’incrément.

Du coup, il est possible de ne lire .length qu’une fois, au moment de l’initialisation du for. Et c’est particulièrement souhaitable. En effet, la majorité des implémentations de tableaux en JavaScript sont en fait des listes liées dont l’accesseur length n’est pas mis en cache[^1] et doit reparcourir la collection à chaque lecture. Du coup, une boucle for qui consulte length à chaque tour (comme ci-dessus) a une complexité cachée quadratique : O(n^2).

Pour éviter ce problème, il suffit de mettre length en cache :

// OK : length mis en cache à l'initialisation
for (var index = 0, len = items.length; index < len; index++) {
  // …
}

Nettement mieux. D’ailleurs, la plupart des transpilers vers JS vont produire ce type de boucle pour leurs syntaxes d’itérations : voyez par exemple ce que fait CoffeeScript.

Nombres magiques

Ah, le bonheur des nombres non évidents sortis de nulle part…

document.cookie = 'foo=bar;path=/;max-age=2592000'

if (nextTask.occursAt - Date.now() < 1800000) {
  // …
}

while (taskCount < 896) {
  // …
}

Par pitié, découpez le calcul avec des nombres compréhensibles, et si possible utilisez des variables/constantes pour les stocker. Aucune pénalité de performance, mais nettement plus lisible :

var COOKIE_MAX_AGE = 30 * 24 * 60 * 60
// …
document.cookie = 'foo=bar;path=/;max-age=' + COOKIE_MAX_AGE

if (nextTask.occursAt - Date.now() < 30 * 60 * 1000) {
  // …
}

var RETENTION_DAYS = 7
var BUCKETS_PER_DAY = 4
var BUCKET_SIZE = 32
var MAX_TASKS = BUCKET_SIZE * BUCKETS_PER_DAY * RETENTION_DAYS
// …
while (taskCount < MAX_TASKS) {
  // …
}

Lookups jQuery répétés

Ce n’est pas par hasard que les jQueryistes ont une réputation de copier-colleurs ; combien de fois suis-je tombé sur ce genre de code :

// KO: Lookups jQuery répétés inutilement
$('.jq-slider a').addClass('active')
$.each(processSteps, function (i, step) {
  setTimeout(function () {
    $('.jq-slider a').css(styleForStep(step))
  }, i * 16)
})
setTimeout(function () {
  $('.jq-slider a').removeClass('active')
}, processSteps.length * 16)

Et allez, pas de pitié, on ré-examine le DOM (et tout le document, qui plus est) à chaque fois ! Peu importe qu’en fait, la sélection soit la même à chaque fois. Et peu importe aussi qu’en l’espèce, notre sélecteur soit assez peu performant (balise courante non qualifiée sur la droite).

Alors je vous en supplie, factorisez-moi ça et ne faites le lookup qu’une fois :

var links = $('.jq-slider a')
links.addClass('active')
$.each(processSteps, function (i, step) {
  setTimeout(function () {
    links.css(styleForStep(step))
  }, i * 16)
})
setTimeout(function () {
  links.removeClass('active')
}, processSteps.length * 16)

Sous-utilisation des prototypes

C’est sans doute la faute à de nombreux tutos mal avisés, mais on trouve encore beaucoup trop de monde qui définit les méthodes d’instance dans le constructeur :

// KO: méthodes singletons redéfinies à chaque instance
function Person(first, last) {
  this.first = first;
  this.last = last;

  this.greet = function greet() { … };
  this.speak = function speak() { … };
  this.walk  = function walk()  { … };
}

C’est une méconnaissance coupable du fonctionnement des prototypes. Les méthodes d’instance ont une implémentation unique à travers toutes les instances, elles devraient donc n’être définies qu’une fois dans le prototype :

function Person(first, last) {
  this.first = first;
  this.last = last;
}
$.extend(Person.prototype, {
  greet: function greet() { … },
  speak: function speak() { … },
  walk:  function walk()  { … }
});

Plus rapide, moins gourmand en mémoire, bref : faites plutôt ça.

Appels tantôt synchrones, tantôt asynchrones

« Ah ouais mais cette fois j’ai l’info en cache, je peux appeler le callback directement ! » Genre comme ça :

// KO : rappel tantôt synchrone, tantôt asynchrone
function fetchDetails(id, cb) {
  if (cache[id]) {
    return cb(cache[id])
  }

  $.getJSON('/api/v1/users/' + id, function (data) {
    cache[id] = data
    cb(data)
  })
}

Oui mais non™. Quand tu fais un truc pareil tu relâches Zalgo (billet d’origine d’izs), et c’est le mal.

Ce genre de méthode exige de son code appelant qu’il gère tant le cas synchrone que le cas asynchrone, les deux n’ayant pas du tout le même workflow ni les mêmes mécanismes techniques de capture d’erreur.

La règle est simple : si ton code a parfois besoin d’être asynchrone, il doit toujours appeler son callback en asynchrone. La bonne version du code ci-dessus est la suivante :

// OK : rappel toujours asynchrone
function fetchDetails(id, cb) {
  if (cache[id]) {
    setTimeout(function () {
      cb(cache[id])
    }, 0)
    // IE9+ et autres browsers : setTimeout(cb, 0, cache[id]);
    // Underscore / Lo-Dash :    _.defer(cb, cache[id]);
    return
  }

  $.getJSON('/api/v1/users/' + id, function (data) {
    cache[id] = data
    cb(data)
  })
}

Comparaisons booléennes mal avisées

Un des pièges fréquents dans lequel tombent les gens en JavaScript est la différence entre une comparaison booléenne laxiste (== true ou == false) et la truthiness (valeur booléenne autonome) d’une expression. J’en parlais déjà il y a plus de deux ans sur JS Attitude.

En gros, on a deux cas :

1. Opérateur binaire (= 2 opérandes) entre un booléen et autre chose : conversion des deux opérandes en Number, jamais en Boolean.
2. Expression seule dans un contexte booléen (intégralité de la condition d’un if, while, for…) : conversion en Boolean.

Notez que si vous recourez toujours à des comparaisons strictes (=== et !==), vous ne risquez pas d’incident : dès lors qu’un des opérandes ne serait pas booléen, ça donnerait forcément false, au moins on est tranquilles…

Mais prenons par exemple ce code-ci :

var x = 42

// OK: truthiness de l'expression, donc ToBoolean
if (x) {
  console.log('ça roule')
}

// KO: comparaison laxiste Boolean/autre : ToNumber
if (x == true) {
  console.log('et mince… car en fait ça teste 42 == 1')
}

// OK: comparaison stricte : on échoue, x n'étant pas Boolean
if (x === true) {
  console.log('bin non…  Boolean ≠ Number')
}

Bref, évitez les x == true et x == false, préférez x et !x, qui garantissent une interprétation booléenne quel que soit le type de x.

parseInt et parseFloat

Je vous le disais déjà il y a deux ans : parseInt et parseFloat sont des enfoirés, pas fiables, fourbes, vils et vicieux. N’ayez jamais recours à leurs services.

Outre la bonne blague du « je devine la base numérique si tu ne la précises pas » avant ES5 (sur IE8 par exemple, parseInt('08') renvoie 0), ils ignorent silencieusement toute saisie invalide (parseInt('42foobar') vaudra 42 et parseInt('42foo@bar.baz') vaudra 0, et non NaN).

Règle 1 : là où vous utiliseriez parseFloat, utilisez Number en tant que fonction : c’est beaucoup plus fiable sans rien perdre de sa lisibilité (c’est même plus lisible, je trouve). Il est possible de raccourcir ça en utilisant l’opérateur unaire préfixe + (genre +x au lieu de Number(x)), mais c’est moins lisible pour le profane…

Règle 2 : là où vous utiliseriez parseInt, utilisez Number en fonction puis arrondissez explicitement (Math.floor sur un positif, Math.ceil sur un négatif, ou Math.trunc dans tous les cas à partir d’ES6). Notez que si vous voulez juste des entiers pas trop grands (qui tiennent en 32 bits) et, surtout, qu’avoir zéro par défaut (au lieu d’un NaN en cas de souci) ne vous dérange pas, y’a x | 0 qui marche bien… mais ça reste illisible pour les non experts.

Gestionnaires en return false

Encore beaucoup trop de développeurs front terminent leurs gestionnaires d’événement par un antique return false, au lieu d’utiliser la (ou les) méthode(s) appropriée(s) sur l’événement (parmi preventDefault(), stopPropagation() et stopImmediatePropagation()).

Ça fait déjà très longtemps qu’on vous dit d’être plus spécifiques, d’apprendre comment tout ça marche, et de ne pas vous reposer sur ce raccourci qui n’a d’ailleurs rien de standard : il est spécifique à jQuery, et servait de « couche de compatibilité » avec les vieux gestionnaires inline dans les attributs HTML genre onclick.

La plupart du temps, vous souhaitez en effet juste preventDefault(), par bloquer la propagation. Et être « forcés » au return peut gêner quand d’autres traitements ont besoin d’être faits dans le gestionnaire, ce qui ne serait pas un problème avec un appel de méthode sur l’événement. Alors prenez un moment pour apprendre comment mieux gérer vos gestionnaires.

Envie d’en savoir plus ?

On fait des formations JS magnifiques, qui font briller les yeux et frétiller les claviers :

• JS Total, pour apprendre tout ce qu’il faut pour faire du dev web front moderne, efficace, rapide et haut de gamme.
• Node.js, pour découvrir, apprivoiser puis maîtriser le nouveau chouchou de la couche serveur, qui envoie du gros pâté !

[^1]: Même si les runtimes très récentes, notamment dans les dernières releases de V8, optimisent enfin cet aspect ; mais votre code doit sans doute tourner sur moins récent que ça…