Code propre: conséquences de méthodes courtes avec peu de paramètres

Récemment, lors d'une revue de code, je suis tombé sur du code, écrit par un nouveau collègue, qui contient un motif avec une odeur. Je soupçonne que les décisions de mon collègue sont basées sur des règles proposées par le célèbre livre Clean Code (et peut-être aussi par d'autres livres similaires).

Je crois comprendre que le constructeur de classe est entièrement responsable de la création d'un objet valide et que sa tâche principale est l'attribution des propriétés (privées) d'un objet. Il peut bien sûr arriver que des valeurs de propriété facultatives puissent être définies par des méthodes autres que le constructeur de classe, mais de telles situations sont plutôt rares (bien que pas nécessairement fausses, à condition que le reste de la classe prenne en compte le caractère facultatif d'une telle propriété). C'est important, car cela permet de s'assurer que l'objet est toujours dans un état valide.

Cependant, dans le code que j'ai rencontré, la plupart des valeurs de propriété sont réellement définies par d'autres méthodes que le constructeur. Les valeurs qui résultent des calculs sont affectées aux propriétés à utiliser dans plusieurs méthodes privées de la classe. L'auteur utilise apparemment les propriétés de classe comme s'il s'agissait de variables globales qui devraient être accessibles dans toute la classe, au lieu de paramétrer ces valeurs pour les fonctions qui en ont besoin. De plus, les méthodes de la classe doivent être appelées dans un ordre spécifique, car la classe ne fera pas grand-chose autrement.

Je soupçonne que ce code a été inspiré par les conseils pour garder les méthodes courtes (<= 5 lignes de code), pour éviter les grandes listes de paramètres (<3 paramètres) et que les constructeurs ne doivent pas faire de travail (comme effectuer un calcul quelconque) c'est essentiel pour la validité de l'objet).

Maintenant, bien sûr, je pourrais plaider contre ce modèle si je peux prouver que toutes sortes d'erreurs non définies peuvent survenir lorsque les méthodes ne sont pas appelées dans un ordre spécifique. Cependant, je prédis que la réponse à cela va être l'ajout de validations qui vérifient que les propriétés doivent être définies une fois les méthodes appelées qui nécessitent que ces propriétés soient définies.

Je préférerais cependant proposer de changer complètement le code, de sorte que la classe devienne un imprimé bleu vers un objet réel, plutôt qu'une série de méthodes qui devraient être appelées (procéduralement) dans un ordre spécifique.

Je sens que le code que j'ai rencontré sent. En fait, je crois qu'il existe une distinction assez claire quant au moment de sauvegarder une valeur dans une propriété de classe et quand la mettre dans un paramètre pour une méthode différente à utiliser - je ne crois pas vraiment qu'ils puissent être des alternatives les uns aux autres . Je cherche les mots pour cette distinction.

En tant que personne qui a lu Clean Code et regardé la série Clean Coders, plusieurs fois, et qui enseigne et encadre souvent d'autres personnes à écrire du code plus propre, je peux en effet garantir que vos observations sont correctes - les mesures que vous signalez sont toutes mentionnées dans le livre .

Cependant, le livre continue en faisant d'autres points qui devraient également être appliqués parallèlement aux lignes directrices que vous avez indiquées. Celles-ci ont apparemment été ignorées dans le code avec lequel vous traitez. Cela peut être arrivé parce que votre collègue est encore en phase d'apprentissage, auquel cas, autant qu'il est nécessaire de signaler les odeurs de leur code, il est bon de se rappeler qu'ils le font de bonne volonté, apprenant et essayant pour écrire un meilleur code.

Clean Code propose que les méthodes soient courtes, avec le moins d'arguments possibles. Mais le long de ces lignes directrices, il propose que nous devons suivre les principes S OLID, augmenter la cohésion et réduire le couplage .

Le S dans SOLID représente le principe de responsabilité unique, qui stipule qu'un objet ne doit être responsable que d'une seule chose. "Chose" n'est pas un terme très précis, donc les descriptions de ce principe varient énormément. Cependant, l'oncle Bob, l'auteur de Clean Code, est également la personne qui a inventé ce principe, le décrivant comme: "Rassemblez les choses qui changent pour les mêmes raisons. Séparez ces choses qui changent pour différentes raisons." Il continue en disant ce qu'il veut dire avec des raisons de changer ici et ici(une explication plus longue ici serait trop). Si ce principe était appliqué à la classe à laquelle vous avez affaire, il est très probable que les éléments qui traitent des calculs soient séparés de ceux qui traitent de l'état de maintien, en divisant la classe en deux ou plus, selon le nombre de raisons. pour modifier ces calculs ont.

De plus, les classes Clean doivent être cohérentes , ce qui signifie que la plupart de ses méthodes utilisent la plupart de ses attributs. En tant que telle, une classe à cohésion maximale est une classe où toutes les méthodes utilisent tous ses attributs; à titre d'exemple, dans une application graphique, vous pouvez avoir une Vectorclasse avec des attributs Point aet Point b, où les seules méthodes sont scaleBy(double factor)et printTo(Canvas canvas), les deux fonctionnant sur les deux attributs. En revanche, une classe à cohésion minimale est une classe où chaque attribut est utilisé dans une seule méthode, et jamais plus d'un attribut n'est utilisé par chaque méthode. En moyenne, une classe présente de « groupes » non cohérents de pièces de cohésion - à savoir quelques méthodes utilisent les attributs a, bet c, tandis que l'utilisation de repos cetd - ce qui signifie que si nous divisons la classe en deux, nous nous retrouvons avec deux objets cohésifs.

Enfin, les classes Clean devraient réduire le couplage autant que possible. Bien qu'il existe de nombreux types de couplage qui méritent d'être discutés ici, il semble que le code à portée de main souffre principalement de couplage temporel , où, comme vous l'avez souligné, les méthodes de l'objet ne fonctionneront comme prévu que lorsqu'elles sont appelées dans le bon ordre. Et comme les deux directives mentionnées ci-dessus, les solutions à cela impliquent généralement de diviser la classe en deux ou plusieurs objets cohérents . La stratégie de fractionnement dans ce cas implique généralement des modèles comme Builder ou Factory, et dans des cas très complexes, State-Machines.

Le TL; DR: Les directives Clean Code que votre collègue a suivies sont bonnes, mais seulement en suivant également les principes, pratiques et modèles restants mentionnés dans le livre. La version propre de la «classe» que vous voyez serait divisée en plusieurs classes, chacune avec une seule responsabilité, des méthodes cohérentes et aucun couplage temporel. C'est le contexte où de petites méthodes et des arguments peu ou pas de sens ont un sens.

Je crois comprendre que le constructeur de classe est entièrement responsable de la création d'un objet valide et que sa tâche principale est l'attribution des propriétés (privées) d'un objet.

Il est généralement responsable de mettre l'objet dans un état initial valide, oui; d'autres propriétés ou méthodes peuvent alors changer l'état en un autre état valide.

Cependant, dans le code que j'ai rencontré, la plupart des valeurs de propriété sont réellement définies par d'autres méthodes que le constructeur. Les valeurs qui résultent des calculs sont affectées aux propriétés à utiliser dans plusieurs méthodes privées de la classe. L'auteur utilise apparemment les propriétés de classe comme s'il s'agissait de variables globales qui devraient être accessibles dans toute la classe, au lieu de paramétrer ces valeurs pour les fonctions qui en ont besoin. De plus, les méthodes de la classe doivent être appelées dans un ordre spécifique, car la classe ne fera pas grand-chose autrement.

En plus des problèmes de lisibilité et de maintenabilité auxquels vous faites allusion, il semble qu'il y ait plusieurs étapes de flux / transformation de données en cours au sein de la classe elle-même, ce qui peut indiquer que la classe ne respecte pas le principe de responsabilité unique.

soupçonne que ce code a été inspiré par les conseils pour garder les méthodes courtes (<= 5 lignes de code), pour éviter de grandes listes de paramètres (<3 paramètres) et que les constructeurs ne doivent pas faire de travail (comme effectuer un calcul d'une sorte qui est essentiel pour la validité de l'objet).

Suivre certaines directives de codage tout en en ignorant d'autres conduit souvent à un code stupide. Si nous voulons éviter qu'un constructeur fasse du travail, par exemple, le moyen le plus sensé serait généralement de faire le travail avant la construction et de transmettre le résultat de ce travail au constructeur. (Un argument pour cette approche pourrait être que vous évitez de donner à votre classe deux responsabilités: le travail de son initialisation et son `` travail principal '', quel qu'il soit.)

D'après mon expérience, rendre les classes et les méthodes petites est rarement quelque chose que je dois garder à l'esprit en tant que considération distincte - plutôt, cela découle quelque peu naturellement d'une seule responsabilité.

Je préférerais cependant proposer de changer complètement le code, de sorte que la classe devienne un imprimé bleu vers un objet réel, plutôt qu'une série de méthodes qui devraient être appelées (procéduralement) dans un ordre spécifique.

Vous auriez probablement raison de le faire. Il n'y a rien de mal à écrire du code procédural simple; il y a un problème à abuser du paradigme OO pour écrire du code procédural obscurci.

Je crois qu'il existe une distinction assez claire quant au moment de sauvegarder une valeur dans une propriété de classe et quand la mettre dans un paramètre pour une méthode différente à utiliser - je ne crois pas vraiment qu'ils puissent être des alternatives les uns aux autres. Je cherche les mots pour cette distinction.

Habituellement , vous ne devriez pas mettre une valeur dans un champ comme moyen de la passer d'une méthode à une autre; une valeur dans un champ doit être une partie significative de l'état d'un objet à un moment donné. (Je peux penser à quelques exceptions valides, mais pas à celles où de telles méthodes sont publiques ou où il existe une dépendance à l'ordre dont un utilisateur de la classe devrait être conscient)

Vous êtes très probablement contre le mauvais modèle ici. Les petites fonctions et la faible arité sont rarement problématiques en soi. Le vrai problème ici est le couplage qui provoque la dépendance à l'ordre entre les fonctions, alors cherchez des moyens de résoudre ce problème sans perdre les avantages des petites fonctions.

Le code parle plus fort que les mots. Les gens reçoivent ce genre de corrections beaucoup mieux si vous pouvez réellement faire une partie du refactoring et montrer l'amélioration, peut-être comme un exercice de programmation en binôme. Lorsque je fais cela, je trouve souvent qu'il est plus difficile que je ne le pensais de bien concevoir, d'équilibrer tous les critères.