Les pires bouts de code que j’ai jamais vu

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.



bonnes pratiques


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
})
}
oof


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.



bonnes pratiques


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.



ha ok


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
bonnes pratiques


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.



leave


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.

Qui me parle ?

jesuisundev
Je suis un dev. En ce moment, je suis développeur backend senior / DevOps à Montréal pour un géant du jeux vidéo. Le dev est l'une de mes passions et j'écris comme je parle. Je continue à te parler quotidiennement sur mon Twitter. Tu peux m'insulter à cet e-mail ou le faire directement dans les commentaires juste en dessous. Y'a même une newsletter !

Pour me soutenir, la boutique officielle est disponible ! Sinon désactiver le bloqueur de pub et/ou utiliser les liens affiliés dans les articles, ça m'aide aussi.

11 commentaires sur “Les pires bouts de code que j’ai jamais vus”

  1. Ça me rappelle un projet client sur lequel j’ai travaillé il y a quelques années. Déjà la base du projet était du code PHP procedural, pourquoi pas j’ai rien contre (le projet datait de 2009). Une des tâches de ma mission consistait en l’ajout de fonctionnalités à un module écrit avec AngularJS 1.x. Sauf que… Sauf que le dev qui était passé dessus avant moi et qui avait mis en place le module n’avait visiblement ni les compétences en JS ni les connaissances suffisantes sur AngularJS (je me demande même s’il avait les compétences suffisantes en dev…). Le résultat a été un code INFÂME à maintenir : tu modifie un truc, BIM t’as tout cassé. Le code était inconsistent, spaghetti, tantôt en français tantôt en anglais, monobloc (les directives, services, factory? Quesaco?). Ajouter à ça le référent métier qui n’y connaissait rien et qui changeait toutes les 5 minutes d’avis sur une fonctionnalité (ce qui a joué dans l’horreur du code je pense), on avait la palme. Ce qui m’a marqué c’est par exemple les appels serveur async sans await et sans loader. Du coup tu ne savais jamais quand ça chargeait. Ou les méthodes écrires dans un controller AngularJS… Et appelées dans un autre (!!). Enfin que des trucs comme ça.
    Et l’autre projet n’était pas mieux : ils l’ont commencé en 2018. Techno front choisie ? AngularJS bien sûr !.. Et quand j’ai demandé au lead Dev la raison, il a pas su me répondre… Bref des moments bien sympa (ironie ironie…).

    Merci pour tes articles en tout cas 😉

  2. Je pense que la majorité des codes spaghettis qu’on trouve sont là par manque d’expérience dans une technologie ou la programmation en général. Les gens essayent de se débrouiller comme ils peuvent en bidouillant jusqu’à que ça marche, et comme ils n’ont pas l’expérience et donc la vision de faire du mauvais code, on se retrouve avec des lignes comme ça non commentées, des variables dont le nom n’explique rien, et aucun formalisme constant dans le code.
    Dès qu’on acquiert de l’expérience je pense qu’on voit ces choses là et qu’on fait de plus en plus attention à ça naturellement!

  3. Si tu veux j’ai un fichier de 532ko sauf que là c’est le fichier le plus important de l’application. Genre c’est le truc le plus utilisé de l’application.
    Là j’ai négocié du temps pour refaire ça étape par étape car le watch du build webpack il prend 5 sec et mon ide il est en pls dès que j’ouvre le fichier…

  4. Billet intéressant.
    Je fais une petit aparté sur le «Scurm Master» que tu décris. Il me semble qu’il ressemble plus à un chef de projet déguisé qu’a un vrai SM. Si son but est de tenir les planning et délai court-termiste sans voir que consacré un peu de temps sur le sujet pour un gain a long terme, c’est qu’il n’a pas compris son rôle. Après ça je ne m’étonne plus que beaucoup de dev voie l’Agilité et tout ce qui l’entoure d’un mauvais œil.

    Merci pour le partage !

    1. C’est malheureusement le cas dans beaucoup d’endroits j’ai l’impression, on suit le Scrum Guide sans vraiment comprendre pourquoi et on se retrouve avec des process complêtement à l’opposé du manifeste agile de départ !

      Pour rappel “Les individus et leurs interactions plus que les processus et les outils”, là où la plupart des équipes Scrum sont complètement sous le contrôle des process Scrum (sans se poser de question) et où le board Jira est presque plus important que l’équipe Scrum !

      Chez nous, on a été formé par une “certification Scrum” (on a eu le joli diplôme et tout !), résultat :
      – On a réussi à augmenter le “time to market” en passant au Scrum, car on ne livre plus qu’à la fin de chaque sprint (ce n’est pas une limite technique, c’est une limite organisationnelle volontaire…)
      – On se retrouve à ne faire quasiment plus de tickets “techniques” car c’est le PO (orientation métier sans background technique) qui a le dernier mot et c’est pas toujours évident de négocier, donc l’état global du produit ne s’améliore pas avec le temps…

      Je ne suis pas membre de l’équipe Scrum, mais vu de l’extérieur, c’est assez flippant, et je ne pense pas qu’on soit la seule entreprise dans ce cas, loin de là !

  5. petit joueur. tu n’as jamais vu un code C avec des fonctions de 2000 lignes (oui 2000, pas 200) qui gère ses données avec des listes chainées gérer inline (avec malloc et gestion de pointeur dans le corps de la fonction) et le tout avec une dizaine de niveau d’indentation.

  6. Niveau bol de spaghetti, je vous propose un fichier C++ de 49149 lignes pour 1740Ko … Celui là rien que de le voir dans le projet il me fait pleurer 😀 , heureusement que j’interviens pas souvent dessus.

    Pour moi il y’a plusieurs cause à ce genre de problème :
    – L’inexpérience du développeur
    – L’absence de review qui pourrait mitigé le point précédent
    – Des mauvaises estimations de temps et/ou l’absence de process de travail qui conduisent les dév à être dans l’urgence en permanence.

    Perso je sais généralement pas écrire un code propre du premier coup. Ca passe par une ou plusieurs phase de refactoring avant de pousser. Sauf que, quand on te donne pas le temps pour ces phases la dettes s’accumule et c’est le drame qu’on connais tous.

  7. Ah tes illustrations me rappellent un concept d’émission à faire : “Cauchemar en SS2I”.
    Encore un side project tombé aux oubliettes, ça aurait pas fait rêver la ménagère mais on aurait pu bien se marrer.

  8. C’était du code très court, mais il était tellement présent, partout dans cette vieille application !

    If (ma_condition)

    Then (!ma_condition)

    Then
    // Ne devrait jamais se produire…

    “Ne devrait jamais se produire” : ah bah tu m’étonnes, John. Le commentaire qui tue !

    Mon premier (et dernier, ouf) booléen à 3 états, je ne savais même pas jusqu’alors que ça pouvait existait… ou pas…

    Code trop ancien pour en connaître l’auteur, j’aurais bien aimé discuter avec lui. Je le verrais bien bosser sur l’informatique quantique… 😉

  9. petite typo dans la phrase “Et justement, après la mise à jour du code, la vraie erreur à commencer à montrer son visage. ” -> “a commencé” 😉

T'en penses quoi ?

Your email address will not be published. Required fields are marked *