Les pires bouts de code que j’ai jamais vus

Aujourd’hui, je vais te montrer les pires bouts de code que j’ai jamais vus. Des sataneries qu’il ne faut surtout pas produire ! À part si tu veux te faire haïr par tes collègues et tes utilisateurs. On va voir qu’avec quelques bonnes pratiques, c’est facile de les éviter.

Satanerie

Il y a une différence entre du code à améliorer et ce que j’appelle une satanerie.

Peu importe le langage, une satanerie c’est un bout de code si sale qu’il met en danger la stabilité et la maintenabilité du projet. Et je peux te dire que j’en ai vu des sataneries.

Quand ça s’accumule, ton projet devient vite comparable aux sous-sols des enfers. Et si c’est toi qui fais des sataneries partout, le lead tech en cuisine va commencer à plus te regarder pareil.

T’as deviné, aujourd’hui on va parler des pires sataneries que j’ai jamais vues. Mais on va aussi discuter de comment ne pas les faire.

Ambiguïté et inconsistance

Il y a bien longtemps, dans une galaxie lointaine, très lointaine, je suis arrivé un matin et on m’a sauté dessus comme si c’était la fin du monde.

Un énorme bug en prod. Tous les tickets du système en production renvoient « null » sans raison. Le chaos total. Tout le monde courait partout comme des poulets sans tête.

Je tape un sprint jusqu’à mon poste et premier réflexe, je regarde Kibana. Aucune erreur. Putain, ça commence mal.

Je décide donc de retracer le cheminement de création du ticket.

Pour ce faire, je dois aller dans les tréfonds des librairies internes qui sont là depuis l’ère jurassique. Au bout de mon travail d’archéologie, j’arrive dans un des fichiers responsables.

Je tombe alors là-dessus.

// use id and expire to get ticket
async function get_ticket(i, expire) {
  return CheckisNotExp(expire).then(async function() {
    var t = await GetTicketModel(i)
    if (t) {
      return t
    } else {
      logger.error(JSON.stringify(t))
      return null
    }
  }).catch(async function(e)  {
      logger.error(JSON.stringify(e))
      return null
})
}

Y’a vraiment une variable i comme ça au calme ? On est où là ? C’est un id c’est ça ? Integer ou UUID ? Et l’expire c’est quoi exactement ? C’est une date ou un timestamp ? Pourquoi y’a du camelCase, PascalCase et snake_case ? Notation async avec des promesses et re notation async ? Si quelque chose fail on retourne null ? SATANERIE !

---

À ce moment-là, j’ai la moitié de la boite qui me Skype toutes les 5 minutes pour savoir ou en est le fix.

Git blame immédiatement, quelqu’un doit avoir des infos sur ce bout de code.

Je me rends compte que c’est pas une seule personne qui est responsable, mais trois. Deux personnes ont quitté l’entreprise depuis longtemps, la troisième n’est pas encore arrivée ce matin. Scénario classique.

D’après Git, ces trois personnes ont touché ce fichier à des périodes très éloignées. D’où l’inconsistante, les différents styles, les différentes versions ECMAScript et les différentes façons de gérer les promesses.

Tiens d’ailleurs j’en profite pour placer la meilleure conférence sur les promesses que j’ai jamais vues.

J’étais dans la salle lors de cette conférence ! T’en as rien à foutre de cette info et tu as bien raison.

Bref, dans ce bout de code, tout est ambigüe, tout est inconsistant. C’est l’exemple parfait de ce que tu dois à tout prix éviter. Inutile de te dire que la code review n’était pas passé là-dessus.

Bon maintenant, il faut dégager ça vite.

Je change le nom des variables et des fonctions. Aucune ambiguïté permisse. Async / Await partout et de la même façon.

Je fais également en sorte de ne cacher aucune erreur sous le tapis avec un return null. Ces erreurs doivent péter la fonction si quelque chose ne fonctionne pas. Les exceptions devraient être gérées par la couche plus haut. Ce n’est pas le travail de cette fonction de gérer ça.

// hotfix
// @todo rewrite the ticket module entirely
async function getTicket(ticketUuid, ticketExpirationTimestamp) {
  await validTicketExpiration(ticketUuid, ticketExpirationTimestamp)
  const ticket = await getTicketByUuid(ticketUuid)

  return ticket
}

La vraie bonne solution serait surtout de réécrire une partie du module. La logique de validation de ticket est super bizarre ici. Mais c’est pas l’urgence.

L’urgence est de repérer et régler l’erreur.

Et justement, après la mise à jour du code, la vraie erreur à commencer à montrer son visage. Un changement de configuration fait le jour d’avant avait changé le comportement des tickets. Le retour à la configuration d’avant régla tout de suite le problème.

La semaine qui suivit, le module en question fut totalement réécrit.

Spaghettis bolognaise

Il y a bien longtemps, dans une galaxie lointaine, très lointaine, je bossais sur un produit au code propre et bien carré.

Comme tout bon produit, tout était optimisé à l’intérieur. Des features faites avec le moins de code possible. Une forte attention à la lisibilité. Un suivi strict de toutes les bonnes pratiques assuré par une code review géré par des nazis du clean code.

Du SOLID, DRY, KISS, YAGNI et tous les autres acronymes auxquels tu peux penser.

C’était tellement propre, on aurait pu bouffer par terre.

Malgré tout ça, une partie bien précise du produit cassait de façon intermittente.

Lors d’un sprint, je finis par réussir à négocier du temps pour investiguer cette affaire.

Très rapidement, je me rends compte que le problème ne vient pas du produit. Les erreurs ont un seul point commun : une dépendance. Une dépendance interne résolue via un artifactory interne.

C’est géré par une autre équipe et -chose très étonnante- le code n’est pas en accès libre. Je demande donc l’accès au code pour comprendre ce qui se passe.

Je reçois alors un message slack où on me demande pourquoi je veux l’accès.

– Salut! Pourquoi tu veux l’accès à ce repo ?
– Comment ça pourquoi ? T’es au courant que je travaille ici ? Attends, j’arrive

Après une clef de bras surprise sur la personne, je finis par avoir accès au projet.

Je vois un seul fichier dedans. Un fichier qui fait presque 300KO. 300KO de texte, c’est gigantesque ! Ça n’a pas été touché depuis plusieurs années. Le nom de la personne qui la touché en dernier m’est complètement inconnue.

J’ouvre alors ce fichier, et là, l’absolue horreur. La pire des sataneries.

Le plus gros spaghetti code que j’ai jamais vu de toute ma vie. Par souci de longueur, et pour préserver ta santé mentale, je te mets pas tout. Juste un très court extrait de ce que j’ai vu à l’intérieur.

Attention, ça pique les yeux.

// Des milliers de lignes de codes spaghettis

if (global.Builder)
{
	module.exports.buildPgs = function(pgs, options, limitNodes = 0)
	{
		var config = options.config || {};
		var builded = [];

		pgs.each(function(pg)
		{
			var supported = pg.prop('tagName') == "INPUT"
							&& pr.attr['name'] == "file"
							&& options.pgs.rel.active == ""
							&& global.FileReader;

			if (!supported || !pg.f || pg.f.length == 0)
				return;
			
			for (var i = 0; i < pg.f.length; i++)
			{
				builded.push({
					file: pg.f[i],
					instanceConfig: _.extend({}, config)
				});
				
				if (isFunction(options.before))
				{
					var returned = options.before(pg.f.path);

					if (typeof returned === 'object' && global.status.in_progress)
					{
						if (returned.action == "skip")
						{
							var needsSkip = (typeof global.status.in_progress === 'boolean' && global.status.in_progress)
											|| _.hasAny("cancel", Global._quotes.BAD_DELIMITERS)
											|| str.indexOf(Global._delimiter) > -1;
							
							if(needsSkip) return;
						}
						else if (typeof returned.config === 'object')
						{
							var LOCAL_BUILDER = new global.Builder("/builder/" + options.module + "/" + options.module + );
							
							for(var s=p,a=p.matchIndex(o),shift=0,i=0;i<a.length;i++){
								var deepcopyfile = JSON.parse(JSON.stringify(pg.f[i].getRawValue()));
								LOCAL_BUILDER.build(deepcopyfile)
								LOCAL_BUILDER.onmessage = global.Notification("buildPg", deepcopyfile);
							}
						}
					}
				}
			}
		});
	}
}

// Des milliers de lignes de codes spaghettis

J’ai même pas essayé de toucher à cet enfant du démon.

Dans un cas pareil, la solution ne passe pas par le code. J’ai déclenché une réunion avec mon équipe autour d’une table. Mon plan était simple.

On y touche pas.

On remplace cette dépendance satanique par un module open-source déjà disponible. Évidement, c’était pas si simple que ça. Il fallait bosser un peu pour plugger correctement la nouvelle dépendance.

L’investigation qui devait prendre max une après-midi se transformait en grosse tâche de plusieurs jours. Le scrum master à l’autre bout de la table faisait la gueule.

Plus la discussion continuait, plus je sentais que ça allait être difficile d’éviter de toucher à cette satanerie. Et comme je le pressentais, la discussion a fini par un non.

« Tu touches juste au minimum pour réparer le module et on passe à autre chose »

Du coup, j’ai fait ce que les développeurs devraient plus faire pour la qualité de code et la viabilité des projets dans le temps. J’ai dit non. Je suis même allé plus loin.

Pour la seule et unique fois de ma carrière, j’ai dit que j’étais prêt à démissionner si on me forçait à le faire.

Ils ont évidemment demandé à d’autres développeurs. Tout le monde a refusé.

Devant le sérieux de tout ça, on m’a débloqué du temps. J’ai développé un petit adaptateur pour la dépendance open source. Puis j’ai supprimé la satanerie des dépendances.

Le produit n’a jamais aussi bien marché après ça.

Si les développeurs redoutent autant le spaghetti code, c’est qu’il y a une bonne raison à ça. C’est le pire type de code sur lequel tu peux travailler. Et pourtant, y’en a partout !

Le pire c’est que ça ne demande pas un énorme investissement pour ne jamais en produire.

Exorcisme

À la base, cet article devait être une liste de bonnes pratiques.

« Pourquoi et comment bien appliquer les bonnes pratiques en tant que développeur. »

Hormis le fait que ce titre a le même effet qu’un somnifère à haute dose, j’ai changé mes plans pour deux raisons.

• La première c’est que je trouve que c’est plus intéressant -pour moi et surtout pour toi- de d’abord parler des conséquences. Elles sont importantes. Ces conséquences sont la formation de sataneries comme celles d’aujourd’hui.

• La seconde c’est que les articles sur le sujet sont déjà légion sur internet. Ils ont tous un point commun. Ils puisent tous leurs infos de deux bouquins. Deux bouquins qui ont marqué plusieurs générations de développeurs.

Coder proprement de Robert Martin

Code complete de Steve McConnell

Tu veux vraiment écourter tes code review et ne jamais être responsable d’une satanerie ? Procure-toi directement la source de l’information et investis du temps dessus.

J’ai trouvé Code complete plus simple à lire et plus pragmatique dans sa démarche. Mais Coder proprement, malgré sa complexité, m’a appris autant -voir plus- que Code complete. Ce sont d’abord des règles et des concepts de programmation que tu vas apprendre. C’est indispensable.

C’est de loin la meilleure décision que j’ai prise pour être capable de donner des bonnes code review aussi.

Car oui, c’est bien de valider des codes reviews. Mais si t’es pas sûr de pourquoi c’est du bon code, ça se finira en satanerie validée par tes soins.

Épilogue

J’ai rencontré beaucoup plus que deux sataneries durant ma carrière. Mais, comme d’habitude, l’article est long et tu as peu de temps à me consacrer. Si le concept te plait, dis-le moi sur twitter. Si ça intéresse assez de monde, je te promets de faire un article partie 2 sur le même sujet.