[Bonnes pratiques] Questions sur lisibilité

Salut à tous,

J’ai écrit un petit code FizzBuzz pour mettre en pratique le TDD sous la forme du kata que voici : https://tddmanifesto.com/exercises/ (premier exercice).

J’aurais quelques questions concernant la lisibilité de ce code, pourtant très simple bien sûr.

<?php

namespace App\BusinessClasses;

class FizzBuzzClass
{

	public function fizzBuzz(int $number)
	{
		$shown_string = $this->isMultipleOf3($number) ? 'Fizz' : '';
		$shown_string .= $this->isMultipleOf5($number) ? 'Buzz' : '';

		if($this->isNotMultiple($shown_string)) {
			$shown_string = $number;
		}

		return $shown_string;
	}

	private function isMultipleOf3($number)
	{
		return $number % 3 == 0;
	}

	private function isMultipleOf5($number)
	{
		return $number % 5 == 0;
	}

	private function isNotMultiple($shown_string)
	{
		return empty($shown_string);
	}

}

1. Par souci de cohérence avec les lignes précédentes, devrais-je utiliser une ternaire au lieu du if, quand bien même je mettrais ceci : $shown_string = $this->isNotMultiple($shown_string) ? $number : $shown_string; (l’identificateur $shown_string apparaît donc trois fois).

2. Puis-je utiliser return empty($shown_string); dans isNotMultiple, parce que la chaîne vide est conséquence du fait que les appels à isMultipleOf3 et isMultipleOf5 ont tous deux retourné FALSE ? Ou bien dois-je absolument utiliser return !$this->isMultipleOf3($number) && !$this->isMultipleOf5($number); ? (voire même, pour éviter de recalculer ces deux fonctions, stocker leur résultat dans deux variables temporaires dès leurs premiers appels dans la fonction fizzBuzz ?)

3. Est-il préférable d’utiliser le nom de fonction isNotMultiple plutôt que de directement utiliser !$this->isMultipleOf3($number) && !$this->isMultipleOf5($number) ?

Merci d’avance !

Bonne soirée !

Mon avis n’engage que moi, mais si je devais améliorer la lisibilité de ce code, la première chose que je ferais c’est supprimer les 3 fonctions privées qui l’alourdissent inutilement.

D’ailleurs, l’intitulé est plutôt d’accord avec moi :

keep the three rules in mind and always write just sufficient enough code

Je pense que c’est assez complexe à lire. Je m’attendrais à quelque chose qu’un débutant en programmation écrirait naturellement après avoir appris les if/else.

La fonction isNotMultiple doit se baser sur l’accumulateur interne qui est construit au fil de l’exécution de ta méthode. Ça ne serait pas plus simple de juste gérer le cas dans un else (branche dans laquelle aucun multiple n’a été pris) ? Ou au moins d’en faire une méthode qui porte mieux son nom : prend en entrée un entier et renvoie un booléen. Ça réglerait d’un coup tes question 2 et 3 qui seraient alors caduques.

P.-S. : Tu parles de TDD et c’est ce que tu veux travailler. Il serait peut-être pertinent de nous montrer alors aussi tes tests, je pense.

Je ne code pas en PHP mais j’aurais codé un truc du style :

  public function fizzBuzz(int $number)
  {
    if($number % 15 == 0) {       // Multiple of 15
      return "FizzBuzz";
    } else if($number % 5 == 0) {  // Multiple of 5
      return "Buzz";
    } else if($number % 3 == 0) {  // Multiple of 3
      return "Fizz";
    } else {                     // Not a multiple
      return strval($number);
    }
  }

Je m’intéroge d’ailleurs sur cette ligne :

$shown_string = $number;

Ici, tu changes le type de $show_string ?

Les méthodes isMultipleOf3 et isMultipleOf3 ont une utilité : faire entrer des éléments du langage du métier (i.e. l’énoncé) dans le code.

Par contre, tu peux les remplacer par une seule méthode du style isMultipleOf(3, value) pour simplifier un peu

J’éviterais les ternaires, ici : ils rendent le code plus difficile à lire

Je ne garderais pas isNotMultiple qui est un peu étrange par construction (son paramètre est une string).
Je ferais le test directement dans la fonction. Même si on pourra arguer que ça ne respecte pas le principe de responsabilité unique (shown_string servirait à construire la chaine et à tester si aucune règle ne s’applique)

Tu peux également pousser un peu plus l’exercice pour te rapprocher de la façon dont on pourrait exprimer les règles à l’oral (même si c’est vraiment poussé pour un fizzbuzz…) et chercher à faire en sorte qu’une personne qui ne code pas puisse comprendre en te relisant

Je pense que c’est une mésinterprétation de l’énoncé, dûe au fait celui-ci n’exprime pas assez clairement l’idée sous-jacente.

Dans le contexte du TDD, "write just sufficient enough code" sous-entend "… to go from RED to GREEN", c’est une invitation à écrire le moins de code possible pour passer un test, quitte à être de très mauvaise foi (if number == 3 { return "Fizz" }) pendant les premières itérations.

De plus, l’énoncé précise juste en-dessous :

Do not forget to refactor after each passing test.

… Et je suppose que c’est exactement à cette étape que ces fonctions sont apparues, avec en tête, probablement, les recommandations de Clean Code de Robert C. Martin, à savoir qu’une fonction helper suffit bien souvent à rendre le code parfaitement explicite en utilisant le vocabulaire du métier, tout en s’affranchissant d’un commentaire qui ne risquera pas de pourrir/mentir.

Et l’on retombe alors sur les conseils de @Mzungu, auxquels je n’ai pas grand chose à rajouter. Je trouve juste personnellement un peu dommage d’écrire une classe pour ça, mais c’est peut-être idiomatique en PHP… d’ailleurs quitte à écrire une classe, je ne suffixerais surtout pas son nom avec Class. Ça reviendrait à la même chose que "Jean-Pierre-Humain" ou "Dominique-Humaine", ou "Félix-Chat" ou "Dolce-Gusto-Cafetière".

Juste pour ajouter un grain de sel.

Définir des fonctions pour les multiples peut être un peu overkill dans des cas simples. Par contre, on peut très bien nommer les choses avec une variable plutôt qu’une fonction. Ça fournit une solution intermédiaire entre rien et des fonctions. On n’a pas la lourdeur d’une fonction, mais on a un apport de sens en nommant la quantité.

$isMultipleOf3 = $number % 3 == 0
$isMultipleOf5 = $number % 5 == 0

if($isMultipleOf3 && $isMultipleOf5) {
   // etc
}

La forme x % y == 0 est tellement courante pour la divisibilité que c’est compris comme un chunk par beaucoup de développeurs. Le mettre à part dans une fonction rajoute une indirection sans forcément rajouter beaucoup de signification.

Et aussi, c’est dur de dire des choses intéressantes sur des exemples aussi simples ou presque tout se vaut…

Dans l’idée, pourquoi pas, mais à condition de faire ça d’une façon à ce que ça ait un sens d’un point de vue métier. Donc ici, plutôt de nommer les méthodes isFizz ou isBuzz, ce qui a plusieurs avantages :

• Ça permet d’écrire très simplement et clairement la dernière condition comme if (!isFizz($nombre) && !isBuzz($nombre),
• Ça évite des noms de méthodes qui paraphrase le code (comme les const EMPTY_STRING = "" ou const AT = "@" qu’on trouve dans le code de personnes à qui on a dit qu’il fallait mettre les String en constantes),
• Ça évite des noms de méthodes qui deviennent faux dès qu’on change d’implémentation (et c’est un cas très courant, très pénible et extrêmement bugogène dans la vraie vie).

Ici l’exemple est peu parlant parce que, comme le dit @Aabu, la notation x % y == 0 est tellement connue et triviale que la remplacer par une méthode n’a pas grand intérêt sur cet exemple, d’où ma suggestion initiale.

Personnellement j’ai l’impression qu’on est dans de la prophétie autoréalisatrice ici : à force de dire que les ternaires sont peu lisibles, on évite d’en mettre même dans les cas où ils sont simplissimes (comme dans ce cas), donc on a pas l’habitude d’en voir, donc ils deviennent effectivement peu lisibles (mais par manque d’habitude plus que par illisibilité intrinsèque).

Dans l’idée, pourquoi pas, mais à condition de faire ça d’une façon à ce que ça ait un sens d’un point de vue métier. Donc ici, plutôt de nommer les méthodes isFizz ou isBuzz,

Pour le coup, je ne suis pas du tout d’accord : isFizz et isBuzz reviennent à créer un vocabulaire qui n’existe pas en amalgamant les choses.

L’énoncé indique :

For multiples of three return “Fizz” instead of the number

On parle clairement de "multiples of three", et "Fizz" est une constante sans signification. Appeler quelque chose isFizz reviendrait à définir implicitement le terme Fizz comme "un Fizz est un multiple de trois", quand bien même le vocabulaire employé dans la spécification ne définit pas du tout ce mot de cette façon.

Pour moi c’est un truc à éviter à tout prix… Même si ça semble ridiculement inoffensif sur un exemple aussi simple : c’est par ce genre de petits amalgames inoffensifs qu’on arrive à des équipes qui ne parlent pas du tout de la même chose en employant pourtant le même vocabulaire en apparence.

Pour le coup, le code qui traduit le mieux cette phrase spec n’est autre que

if isMultipleOfThree {
    return "Fizz"
}

Que ce soit une variable ou une fonction isMultipleOf, le fait est que c’est une traduction directe de ce que fait ce code et de la façon dont il a été conçu, et qui est même compréhensible par un non-dev.

Edit:

Tout moinssoyé que je sois, je ne suis pas le seul à le dire. Cela fait écho directement à la notion de ubiquitous language, ou "langage omniprésent" qui n’est visiblement pas importante que pour moi.

… C’est pour ça que je précise ensuite que l’exemple n’est pas parlant dans ce cas. Mon propos, c’est surtout qu’il faut que le nom de la méthode représente le besoin métier et qu’elle ne se contente pas de paraphraser l’implémentation. Ce qui est beaucoup trop souvent le cas – et qui semble être le cas ici, si j’en crois le nom de la 3ème méthode.

D’autre part, on atteint aussi ici les limites d’un énoncé trop simple et surtout hors sol (ne correspondant à rien dans la réalité). En y réfléchissant, ici pour moi les noms des constantes ici n’ont pas spécialement plus de significations métier que les multiples utilisés (d’ailleurs permettre de changer ces constantes est une variante de l’exercice au même titre que changer les valeurs 3 et 5).

Dans un cas réel de doute comme ici, il faudrait se poser la question du fonctionnel derrière la méthode (ça répond à beaucoup de ce genre d’interrogations – et c’est pour ça qu’il me semble impensable que l’on puisse développer des trucs sans comprendre ce qu’on fait, coucou l’outsourcing) et, si c’est toujours pas clair, aller chercher l’information chez qui la possède.

(PS à propos des liens : mon moinssoyage ne porte absolument pas sur la notion de langage omniprésent, mais sur le fait que l’une ou l’autre des interprétations soit « évidente » par rapport à la spécification).

Donc, à défaut d’information, la meilleure approche serait de tout coder de manière linéaire et statique, et attendre d’avoir besoin de changer les conditions ou les mots à afficher pour faire évoluer l’architecture du code ? Parce qu’au final, on peut se retrouver à faire un code générique avec un semble de règles de substitutions, et on peut gérer indépendamment les règles, les vocables à afficher, et les sets de règles à appliquer. Mais ça semble un peu compliqué à écrire et à relire si le code reste ad vitam un Fizz/Buzz/FizzBuzz pour les valeurs 3/5/15

Et donc dans tous les cas, ça reste une très mauvaise idée de créer un vocabulaire nouveau ou détourner celui de l’énoncé.

On pourrait tout justifier avec le fait que l’énoncé est trop simple ou abstrait, mais le fait est que l’on est dans le cadre d’un exercice qui vise à acquérir de bons réflexes. Celui de détourner "Fizz" et "Buzz" de leur non-signification, quel que soit le contexte, est un très mauvais réflexe de nommage, à moins qu’un expert du domaine ne soit venu confirmer qu’il l’entend de cette façon également, auquel cas ce n’est plus un détournement mais la réutilisation du langage omniprésent du domaine.

En l’absence de toute information supplémentaire, le nommage doit rendre intelligible ce qui se passe dans le code : sans autre information, si je lis isFizz dans un code, je n’ai aucune idée de ce que cela signifie et je suis obligé de suivre une indirection pour comprendre cette ligne.

Par contre si je lis isMultipleOf(3, n), alors clairement le nommage m’indique que cette expression vaut True si n est multiple de 3, je peux me passer de l’indirection et lire la ligne suivante pour savoir ce que l’on fait dans ce cas.

Le présent thread porte sur la lisibilité : entre ces deux solutions, il ne fait aucun doute à mes yeux (ni à ceux de Robert C. Martin dans Clean Code, pour ce que cela vaut) que ces deux solutions ne sont pas du tout équivalentes.

L’une se lit sans indirection (sauf à la rigueur pour vérifier après coup que la méthode privée fait bien ce qu’elle annonce), l’autre demande de sauter dans le code pour comprendre ce qui se passe.

PS : il ne faut pas non plus oublier qu’il s’agit d’un exercice pour débutant, autrement dit si l’on a besoin de marteler des termes comme "métier", "domaine" ou "spécification" dans nos justifications, alors celles-ci sont certainement hors sujet.

Mon point de vue se résume à "il ne faut pas chercher à réinventer des termes qui ne sont pas utilisés avec strictement le même sens dans l’énoncé", et "il faut que le code parle strictement le même langage que la personne pour qui on l’écrit".

Dans l’idée, pourquoi pas, mais à condition de faire ça d’une façon à ce que ça ait un sens d’un point de vue métier. Donc ici, plutôt de nommer les méthodes isFizz ou isBuzz, ce qui a plusieurs avantages :

• Ça permet d’écrire très simplement et clairement la dernière condition comme if (!isFizz($nombre) && !isBuzz($nombre),
• Ça évite des noms de méthodes qui paraphrase le code (comme les const EMPTY_STRING = "" ou const AT = "@" qu’on trouve dans le code de personnes à qui on a dit qu’il fallait mettre les String en constantes),
• Ça évite des noms de méthodes qui deviennent faux dès qu’on change d’implémentation (et c’est un cas très courant, très pénible et extrêmement bugogène dans la vraie vie).

Comme le dit @nohar, je n’aurais pas utilisé ces méthodes, du moins comme ça : j’aurais à la rigueur créé des méthodes isFizz et isBuzz pour passer les règles à mon fizzbuzz de façon à le rendre open/closed (si l’objectif est de poursuivre sur un foobarqix)

Mais elles se seraient tout de même appuyées sur une méthode isMultipleOf.

Ici l’exemple est peu parlant parce que, comme le dit @Aabu, la notation x % y == 0 est tellement connue et triviale que la remplacer par une méthode n’a pas grand intérêt sur cet exemple, d’où ma suggestion initiale.

Quand je lis du code métier, je ne veux pas avoir besoin d’interpréter plus que nécessaire.
Je trouve difficile de devoir à la fois analyser le quoi et le comment, notamment quand il s’agit des conditions : mes yeux glissent plus facilement sur un if(isMultipleOf(value, 3)) que sur un if(value % 3 == 0)
Ne serait-ce parce que je dois comprendre qu’il y a un modulo et qu’on teste le résultat avec 0 et donc extrapoler le si c’est un multiple alors.
En le sortant dans une méthode isMultipleOf, je rends l’implicite explicite et je donne une indication sur ce qui est en train de se passer pour les devs qui ne seraient pas forcément à l’aise avec l’opérateur %

En bonus, ça factorise le calcul de l’application des règles, le rendant plus facilement modifiable.

De façon générale, mes conditions sont toujours un appel à une méthode/fonction dont le nom explicite le calcul

J’aurais peut-être dû placer mon ici à la fin de ma phrase ou l’écrire autrement. Mais mon propos n’est pas de ne jamais utiliser de ternaire.

De ce code précis, le ternaire ajoute une branche sinon chaîne vide qui n’est là uniquement parce qu’on utilise un ternaire (on aurait un simple if sans else). On vient d’ajouter de la complexité accidentelle

Probablement, oui.

Pour moi tout ça rejoint pas mal les problématiques de définition et de nommage d’interfaces (au sens large du terme, dont le découpage en méthodes et leur nommage).
Est-ce que tu veux systématiquement définir des interfaces explicites – quitte à n’avoir qu’une seule implémentation pour la vie du projet – ou des interfaces implicites – qui seront explicitées si le besoin s’en fait sentir un jour ?
Est-ce qu’il vaut mieux que le nom de l’interface m’indique ce qu’elle fait d’un point de vue technique ou fonctionnel ?
À quel point faut-il rentrer dans le détail dans le nommage ?
À quel point faut-il donner des interfaces souples et évolutives ?

En fait tout ça dépend énormément des contraintes réelles du projet, de l’équipe, de la fiabilité et de la stabilité des spécifications.

Des spécifications floues et mouvantes vont demander aussi tôt que possible un code souple capable de s’adapter rapidement aux changements sans tout réusiner à chaque nouvelle demande (quitte à ce que l’implémentation initiale soit un peu plus longue). Un projet « jetable » (exemple : un site ou une application publicitaire) pourra se contenter d’un code très rigide. Un fonctionnel compliqué ou une implémentation complexe gagneront à avoir une politique de nommage qui explicite le pourquoi plutôt que le comment : en lisant le nom de cette méthode, je sais ce qu’elle fait, je ne m’intéresserai au comment elle le fait que si j’ai besoin d’en savoir plus, de toutes façons creuser un niveau de détail supplémentaire ralentirait ma compréhension. Un code au fonctionnel simple peut utiliser efficacement une politique de nommage où chaque méthode explique ce qu’elle fait techniquement, ce qui est en effet plus rapide pour relire le code en évitant d’aller lire l’implémentation exacte desdites méthodes.

Le plus important dans tout ça, c’est que les interfaces et leur nommage soit cohérent au travers de tout le projet – et ça n’est pas trivial à faire, même sur les projets où on est seul.

Personnellement (et j’insiste, c’est un avis personnel) je préfère, à défaut d’autre contrainte forte, un nommage qui explique le pourquoi que le comment. Je préfère savoir que ma méthode « fait un truc » (quitte à ce que je doive sauter à son implémentation pour savoir comment elle le fait – ce qui est souvent inutile) plutôt qu’elle me dise comment sans que je sache pourquoi.

Cette préférence ne sort pas de nulle part, mais de l’historique des projets sur lesquels j’ai travaillé. Je me suis trop posé de question du genre « OK, je comprends ce que fait ce code, mais pourquoi ce truc à cet endroit ? » – et surtout, trop souvent cette question aurait pu être répondue tout simplement en donnant un nom plus clair à une ou deux méthodes, la question des implémentations complexes sans commentaires est encore autre chose. Je me suis aussi trop souvent fait avoir par des méthodes du style ajouteUneHeure() qui en fait ajoutaient 15 minutes, parce qu’à un moment la spécification a changé sur un point sans que les méthodes n’aient été mises à jour¹ (et je précise, là encore, que souvent, dans les cas que j’ai croisé, un nommage qui ne se contentait pas de répéter le code aurait suffit car le nouveau nommage aurait été identique à travers les modifications).

Cela dit, on peut tout à fait avoir un avis différent sur le sujet.

@Herbe ce qui ressort principalement de tout ça, tu l’auras compris j’imagine, c’est que la notion de « code propre et lisible » n’est absolument pas universelle et va beaucoup dépendre du projet et des sensibilités de chacun.

Pour répondre de nouveau à ta question, dans le cas précis de ce kata, le besoin est trivial et sans demande d’évolution, donc je partirais sur le code le plus simple possible, comme je le disais dans mon premier message. Si tu tiens à garder tes méthodes privées, mon second message était trop affirmatif : c’est ce que je ferais à ta place, mais à réflexion un nommage comme le tien pour les deux premières méthodes au moins me semble tout autant défendable.

C’est un point que tu pourrais prendre en compte pour les prochains katas, qui ont plus d’étapes et sont moins triviaux : regarder avant de commencer quelles sont les fonctionnalités qui te seront demandées (dans un projet réel, tu as souvent une bonne idée d’à quel point les spécifications sont fiables et changeantes, même si tu n’as pas le détail de ce qui va changer). Et partant de là tu peux même faire le kata en deux versions : l’une où tu t’astreins à faire systématiquement le code minimum nécessaire, et une autre où tu prévois de pouvoir implémenter les évolutions futures facilement sans trop de modifications.

C’est peut-être une question d’habitude. Pour moi le modulo était assez utilisé pour qu’on ne bute plus sur sa signification (surtout avec cette construction en particulier), mais ça peut être faux. Là encore, j’imagine qu’en projet réel ça dépend beaucoup de l’équipe.

Je ne sais plus comment fonctionne PHP à ce sujet, mais il me semble que le premier ternaire a besoin de la chaine vide non ? Le second correspond bien à un if sans else, et là je suis d’accord avec toi.

L’exercice s’appelant le FizzBuzz, c’est pas vraiment déconnant de considérer que le but est de chercher des Fizz et des Buzz. En tous cas, ça ne me semble pas plus déconnant que de dire que l’exercice est de rechercher des multiples. Comme le signale nohar, Fizz et Buzz sont des expressions volontairement abstraites, mais tout l’exercice l’est, et si l’exercice devenait de classer des véhicules blindés et tout terrain, l’existence de méthodes estBlindé et estToutTerrain ne serait pas aberrante. Encore une fois, ça dépend comment on voit l’exercice.

Pour le coup, mes codes s’adressent en général à des gens qui lisent (value % 3 == 0) sans se poser de question, tandis que isMultipleOf ne fait pas partie de la norme du langage, et demande donc de faire confiance ou de vérifier le code des développeurs précédents, qui est plus susceptible de contenir des erreurs à ce niveau que le compilateur. Pour un code de 20 lignes, c’est dommage de s’imposer ça.

Let’s agree to disagree, dans ce cas.

Je pense que nos divergences sont dûes à plusieurs éléments contextuels insolubles :

Je pars personnellement du principe que le code que j’écris est avant tout à destination des humains qui devront le lire, plutôt que du compilateur (qui, dans mon cas, inlinera très certainement cet appel de toute manière).

En ce qui me concerne j’attache beaucoup de valeur à écrire un code, qui, lorsque l’on vient me voir en me demandant "qu’est-ce qui se passe dans ce cas ?", me permette de répondre à mon interlocuteur sans aucune traduction en termes de vocabulaire, et c’est ce qui justifie l’approche que j’emploie sur cet exercice.

Pour autant, il m’arrive aussi de tomber sur du code mal foutu qui n’est pas le mien et des fonctions qui se mettent à mentir quand elles sont changées à la va-vite. Mais quand c’est le cas, en ce qui me concerne, je ne gueule sur personne : je réfléchis à un nom qui aurait plus de sens, et je propose un correctif dûment justifié, voire une refactorisation, et on en discute.

Ce qui doit sûrement changer dans mon contexte n’est, a priori, ni spécifiquement le pays dans lequel on travaille (je n’ai jamais bossé en Théorie, ils payent bien là bas ?), l’âge du code ou la personne qui l’a écrite, mais simplement les modalités de communication dans mon équipe (quand on constate un problème, on ne cherche pas quelqu’un sur qui gueuler mais comment le résoudre tout de suite et l’éviter à l’avenir), et la place de premier ordre que l’on donne à la refactorisation pour que le code soit et reste en permanence l’expression la plus fidèle de la solution que l’on apporte à nos utilisateurs, ce qui inclut la façon dont on l’a pensée.

(Un soupçon d’ironie s’est glissée dans ce post, ne le prenez pas à mal, c’est juste pour détendre l’atmosphère : ce sujet ne vaut vraiment pas le coup de se prendre la tête ni de polariser la discussion)

Je tiens à préciser que je parle de m’engueuler moi-même, jamais d’engueuler un tiers… visiblement ça n’était pas clair, puisque le sujet revient deux fois. Parce que oui, je râle après moi-même quand j’ai fait une connerie qui m’impacte. Ça n’implique absolument pas que je gueule après les autres.

Quant au correctif et à la refactorisation systématique en cas de mauvais nommage, je suis tout à fait d’accord. En théorie. En pratique, ça implique d’avoir un projet qui le permette (pour tout un tas de raisons : méthode très utilisée, appelée dynamiquement, mal ou pas couverte par des tests, etc). En fait, rien que retrouver les informations pour créer un nouveau nom clair peut s’avérer difficile – heureusement j’ai fui ce genre de projet et ne devrait plus les rencontrer de si tôt.

PS : j’écris aussi du code pour les humains, hein. C’est très exactement ce qui prédispose à ma démarche.

Le contexte est probablement différent, mon contexte implique régulièrement d’intervenir sur un code dont je ne verrai jamais l’auteur, et qui sera probablement gardé en l’état jusqu’à ce qu’une autre modification soit nécessaire, possiblement après mon départ de l’équipe de développement, ou peut être jamais. Mon problème n’est donc pas non plus de trouver un coupable, mais la communication s’effectuera probablement différemment.

Mais j’ai pas l’impression qu’on soit fondamentalement en désaccord: la deuxième considération de mon message précédent indiquait clairement que je m’intéressais à la tâche de mon relecteur, et pas à celle du compilateur.

Pour autant, on restera sur un désaccord sur la solution à apporter ici, parce que, l’exercice étant abstrait, on imagine un contexte en fonction de notre expérience. En soit, ça ne me pose pas de problème, tu refuses d’intégrer le vocabulaire Fizz/Buzz dans ton code parce qu’il n’est pas dit explicitement que c’est ce qui structure le test, moi je considère que c’est le titre de l’exercice et que c’est structurant. Sur un cas réel, on aurait probablement l’occasion de demander à un expert du domaine; en attendant, j’accepterais les deux réponses dans un entretien techniques, même si j’éviterais probablement la question: je n’ai pas l’impression que la discussion de la solution apportée, comme ci-avant, me renseigne particulièrement sur la capacité à gérer un problème réel.

J’ai raté les noms de ceux qui disaient ça, mais +1 à avoir des is_fizz / is_buzz plutôt que des is_multiple_of. Cela correspond à ce qui est attendu, au métier. Et si les specs de l’un ou l’autre devait évoluer, cela résisterait mieux aux évolutions. Pourquoi pas que cela serait implémenté en termes is_multiple_of en interne si on doit considérer que les futurs mainteneurs ne sachent pas lire le langage dans lequel le code est écrit et décoder les modulos (malheureusement pas une spéc si aberrante…)(ceci dit, lire "divisible" ou "pas-divisible" d’un != 0 demande plus d’attention je trouve)

Dans tous les cas, j’avais trouvé $this->isMultipleOf3($number) difficile à lire. Est-ce à cause de la syntaxe de php qui demande $this->? Est-ce à cause du lowerCamelCase et ma vue qui évolue? Est-ce à cause du diviseur perdu au milieu du nom du booléen? Un mélange subtil entre tout ça?

Pour autant, on restera sur un désaccord sur la solution à apporter ici, parce que, l’exercice étant abstrait, on imagine un contexte en fonction de notre expérience.

Je pense que c’est la clé, oui.

Le pire, dans tout ça, c’est que je serais parfaitement d’accord avec cette interprétation ("c’est le métier") si l’on était en train de parler du Bowling Game, où, là, le métier définit explicitement ce que sont un strike et un spair, ce qui justifie largement l’écriture de fonctions isStrike ou isSpair. Sauf qu’ici ce n’est absolument pas le cas : le métier c’est justement cette fonction, qui n’a pas besoin que l’on définisse plus de vocabulaire "métier" que ce qui existe déjà pour l’exprimer. L’énoncé ne parle réellement que de multiples et de constantes à retourner, donc si le "métier" est ambigu, j’applique un adage purement pythonique : "in the face of ambiguity, refuse the temptation to guess", en l’occurrence, ici, deviner la signification de Fizz ou Buzz.

PS : imaginez, par exemple, un jeu débile de Perceval dans Kaamelott, ou en cas de "Triple caillou", il faut crier "sloubi !". Dans ce cas, vous n’appelleriez la fonction isSloubi, mais bien isTripleCaillou… Ici, pour moi, c’est pareil : "Fizz" et "Buzz" ne sont que les signaux retournés, mais rien, nulle part, n’indique que c’est l’expression du métier qui décrit la condition pour les déclencher.

En réalité, (et @nohar l’a déjà expliqué, il me semble) is_fizz et is_buzz ne correspondent pas à la description de l’énoncé (certes, celle citée par le premier message n’est clairement pas la meilleure).

C’est une interprétation qu’on fait parce que les termes fizz et buzz sont dans l’énoncé. Mais à aucun moment, on parle d’un nombre qui serait "fizz" ou "buzz"

Le métier, lui, parle de multiples de 3 et 5 (ou de nombres divisibles par 3 ou 5) qui donnent un mot contenant fizz, buzz ou les deux

De ma modeste expérience, c’est ce genre de réinterprétation de la part des devs qui conduit des projets à être difficiles à maintenir (voire au fameux projet de refonte…) : il faut se mettre à la place des devs qui arriveront sur le projet et qui auront des specs parlant de nombres multiples de 3 et 5 et un code qui parlent de nombres qui sont fizz ou buzz

De mon point de vue, le fizzbuzz parait simple sur papier mais si on n’arrive pas à refaire sortir le métier dans notre code comme énoncé par le métier, ça montre bien qu’il y a quelque chose qui coince quelque part…