Organiser son code est tout un art. Ça se peaufine au fur et à mesure d’une carrière. J’ai découvert au fil du temps des règles qui formalisent quelque chose que je faisais sans vraiment m’en rendre compte. Vous savez comme ces règles de français à la noix que vous utilisez quotidiennement sans savoir qu’il y a règle.
DRY au bout de 3
Je commence par la plus facile.
Le célèbre Don’t Repeat Yourself n’a pas besoin d’être poussée à l’extrême. Copier/coller du code une fois est tout à fait acceptable, et souvent bien plus productif que de refactoriser son code. On commence donc à faire une fonction si on a 3 copies de son code.
Dans ce code par exemple, j’utilise 3 fois mon trigger, il est mur pour la cueillette :
async def setup(self, cwd=None): if cwd is None: cwd = get_project_dir() self.project_dir = Path(cwd) # Tell all component to hook them self to the events they want to react # to futures = [ensure_awaitable(c.setup) for c in self.components.values()] await asyncio.gather(*futures) self.state = "init" await self.trigger('init') self.state = "ready" await self.trigger('ready') self.state = "running" return self.trigger('running') |
Pouf:
def change_state(self, value): """ Change the inner state of the app, and call all state observers. """ self.state = value return self.trigger(value) async def setup(self, cwd=None): if cwd is None: cwd = get_project_dir() self.project_dir = Path(cwd) # Tell all component to hook them self to the events they want to react # to futures = [ensure_awaitable(c.setup) for c in self.components.values()] await asyncio.gather(*futures) await self.change_state('init') await self.change_state('ready') return self.change_state('running') |
Transformez vos commentaires en fonctions
C’est un conseil qui a apparemment été rendu populaire pas le courant Extreme Programming. Leur idée c’est que les noms de fonctions sont une forme de documentation. Chaque fonction devant être testée et documentée, si vous avez un commentaire qui se balade tout seul au milieu du code, c’est le signe qu’il faut en faire une fonction.
Dans le code qu’on vient de voir, j’ai un gros commentaire en plein milieu expliquant un bout de code un peu compliqué. Hop, on coupe :
async def setup(self, cwd=None): if cwd is None: cwd = get_project_dir() self.project_dir = Path(cwd) # Tell all component to hook them self to the events they want to react # to futures = [ensure_awaitable(c.setup) for c in self.components.values()] await asyncio.gather(*futures) await self.change_state('init') await self.change_state('ready') return self.change_state('running') |
Pouf:
async def setup_components(self): """ Give all components a chance to hook to life cycle events """ futures = [ensure_awaitable(c.setup) for c in self.components.values()] return await asyncio.gather(*futures) async def setup(self, cwd=None): if cwd is None: cwd = get_project_dir() self.project_dir = Path(cwd) await self.setup_components() await self.change_state('init') await self.change_state('ready') return self.change_state('running') |
On voit ici que la présence du commentaire signalait que le code était assez complexe, et donc méritait d’être isolé. Cela permet de :
- Rendre la fonction
setup()plus facile à comprendre. - Isoler la complexité en une unité simple, plus facile à lire et à expliquer donc les limites et dépendances sont claires.
- Et donc permettre un beau test unittaire juste pour cette petite fonction.
Une partie qui n’a rien à voir avec la choucroute mérite sa propre choucroute
Dans de grosses fonctions, on a souvent des bouts qui sont des étapes, mais qui ne sont pas liés sémantiquement aux autres étapes. Elles doivent juste arriver chronologiquement avant ou après celle-ci.
Toujours dans le même code, on a un bout qui initialise le project_dir de l’app. Ce snippet, c’est comme l’inspecteur Karamazov, fils unique, aucun lien. En tout cas aucun avec le setup de components ou le changement du state. Il est là car il doit arriver en premier. Aucune, aucune, aucune hésitation ! Amputation, SO-LU-TION !
async def setup(self, cwd=None): if cwd is None: cwd = get_project_dir() self.project_dir = Path(cwd) await self.setup_components() await self.change_state('init') await self.change_state('ready') return self.change_state('running') |
Pouf :
def setup_environment(self, cwd=None): """ Set project dir for the current app """ if cwd is None: cwd = get_project_dir() self.project_dir = Path(cwd) async def setup(self, cwd=None): self.setup_environment(cwd) await self.setup_components() await self.change_state('init') await self.change_state('ready') return self.change_state('running') |
On gagne en testabilité.
Et maintenant notre méthode setup() est très, très facile à comprendre. Son rôle est d’organiser 5 étapes. Pas de faire les étapes elle-même, ça c’est délégué. Le cycle de vie de l’application saute maintenant aux yeux. S’il y a un bug, on cherchera uniquement dans la fonction qui est responsable.
Sans le savoir, et sans vraiment le chercher, nous avons mis en oeuvre le principe de séparation des responsabilités. Seulement au lieu d’avoir cette notion floue et abstraite de responsabilité, on a maintenant quelques règles qui peuvent être facilement appliquées.
Diantre, je faisais donc de la prose, depuis toujours dans le savoir…
Pas de paramètre on/off
Le code de l’article d’hier était un cas d’école
Ma première version ressemblait à :
def resource_stream(resource, locations, encoding="utf8", ..., binary=False): """ Return a resource from this path or package """ # du code, du code... puis: if binary: return stream return io.TextIOWrapper(stream, encoding, errors, newline, line_buffering) |
On voit clairement ici que binary est un paramètre on/off, un truc qui déclenche un mode auquel on va passer un booléen.
Ça va s’utiliser comme ça:
txtstream = resource_stream(...) bstream = resource_stream(..., True) |
Quand on a des constantes magiques, il faut sortir la hache :
def binary_resource_stream(resource, locations): """ Return a resource from this path or package """ ... return stream def text_resource_stream(path, locations, encoding="utf8", ...): stream = binary_resource_stream(path, locations) return io.TextIOWrapper(stream, encoding, errors, newline, line_buffering) |
La seconde fonction utilise la première. A l’usage le code est beaucoup plus clair :
txtstream = text_resource_stream(...) bstream = binary_resource_stream(...) |
Encore une fois, c’est plus facile à tester, plus facile à comprendre, et on diminue le nombre de branches dans le code.
Pour les gens particulièrement intéressés par la lisibilité du code, il existe l’indicateur de McCabe qui se base notamment sur le nombre de branches logiques. Le max recommandé est en général de 10, personnellement mes codes dépassent rarement 5. flake8 a l’option –max-complexity pour vérifier que votre code s’y tient.
En gros, c’est l’inverse des one-liners
AAAAAAAAAAMen to that. je fais tourner aux collègues.
Bon article comme trop souvent, et du coup je vais en prendre un peu le contrepied (c’est sans douleur). L’idée est tirée d’un commentaire à un article que je ne retrouve pas donc je vais faire comme si j’étais intelligent.
L’idée d’extraire des fonctions construisant ainsi son propre language pour gérer le problème avec des couches plus faciles à étudier et tester c’est une montée en abstraction. Et les abstractions c’est bien mais parfois ça foire, parfois on se rend compte plus tard qu’on a choisi la mauvaise: les différentes fonctions similaires s’avèrent au fil des changements pas si similaires en soit et difficiles à rassembler sous un même étendard, l’argument que l’on croyait binaire commence à prendre un peu trop de valeurs différentes et les parties commentées du code sont trop liées au code d’origine pour permettre une extraction aisée.
Bon, c’est pas toujours le cas heureusement, mais ça arrive. Et bien il se trouve que le processus s’inverse en nous donnant plein d’outils pour gérer de mauvaises abstractions:
Si une fonction a du mal à fédérer différents comportements dupliquer le code peut rendre les choses plus claires
Si une fonction est compliquée mais perd son sens hors d’une autre fonction alors les rassembler avec un commentaire est la chose à faire
Si on commence à avoir beaucoup de fonctions similaires avec des noms à peine différents on devrait sans doute n’avoir qu’une fonction et passer un paramètre à la place
Bon, je ne dis pas qu’il faut faire ça tout le temps, mais on tape toujours sur la duplication de code alors je pense intéressant de la voir comme un outil à notre disposition pour résoudre des problèmes pour changer :)
Pas mal ! Vraiment à prendre en considération ! Si on est pas en embarqué avec 1024 bytes de programme et une stack de 128 octet ! Harggg !
En ce moment je développe un peu d’AVR, de l’embarqué comme ils disent, en fait c’est de l’arduino. Mais quand on commence à descendre sur des cachou comme l’ATtiny 25 on est bien obligé d’arriver à des truc comme ça pour économiser un peu de jus :
if( millis()-timer >= (digitalRead(output)? 1200 : 750)){...}C’est pas bien compliqué mais ça nécessite son petit commentaire ! Bourré de la veille, je prends pas les paris que je m’y retrouve !
Bref, le but de ce message c’était ; oui c’est très bien, mais je pense qu’il faut aussi prendre en considération le matériel. Sur un 64Bits 32go de ram avec garbage collector pas de soucis… Quand on est à un UINT prêt… ça ce discute… même une condition IF peut faire kaboum entre la stack et les variables statiques ! Et alors pour debuger ça au secours !
Merci pour l’article néanmoins :-)
Perso je serai un peu “normand” dans cette façon de faire :
je serai à mi chemin entre Cym13 et Sam. Le principe du jusqueboutisme en refactorant jusqu’à ce qu’on ne puisse plus faire de fonctions “à la poupée russe” (qui en appelle une, qui en appelle une, qui en appelle une), ne me sied guère, car à force de devoir aller à la fonction appelante pour m’y retrouver/debugger un probleme, me perdra en compréhension.
Le code ne va pas bien marcher, car la fonction “change_state” est devenue “change_setate” entre la définition et les appels :-)
self.setup_environnement(cmd)Le paramètre devrait être “cwd” j’ai l’impression.
Je crois que la variable “value” utilisée à trois reprises dans le premier bout de code n’existe pas. Je subodore un copier/coller foireux.
Tant de monde pour relire un article, c’est beau.
Article intéressant. Je m’étais jamais vraiment rendu compte de la mauvaise idée des paramètres on/off. Pourtant, j’en fous pas mal partout, en y repensant.
Excellent. Ton article vient à point après mon visionnage ce matin de la vidéo que tu as partagé sur Twitter. Merci.
Je ne suis pas encore familier avec la syntaxe
async/awaitdonc je me trompe peut être mais il me semble que la fonctionsetup_environnementne doit pas être déclaréeasync.Tant qu’on y est avec cette fonction, le nom
setup_environmentparait plus judicieux.Merci pour l’article qui va dans le bon sens comme d’hab.
Je pinaille (ce n’est pas sale) :
s/pas le courant extrem programming/par le courant Extreme Programming/
@Cym13 : je pense que tu fais un amalgame entre le fait d’extraire une fonction (ou une méthode) et le fait d’exposer cette fonction à l’extérieur du module, ce qui est loin d’être systématique.
@Foxmask : si tu te retrouves avec une chiée de fonctions ou de méthodes après avoir tout extrait, c’est peut-être que tu as besoin d’extraire un module, ou une classe. C’est l’étape d’après ^^
Et sinon, j’aurais rajouté comme indicateurs :
La fonction est longue (plusieurs dizaines de lignes)
La fonction contient plusieurs blocs imbriqués (for -> opportunité de faire un générateur, if else -> opportunité d’extraire une fonction avec des early returns)
L’approche extreme serait : http://williamdurand.fr/2013/06/03/object-calisthenics/
+1 pour la vidéo de Brandon Rhodes.
@Romain: non non, je vois très bien la distinction, ce que je ne vois pas par contre c’est comment tu en viens à cette conclusion. La problèmatique d’exposition des interfaces et de couche d’abstraction est liée mais orthogonale à mon sens.
Perso, l’article a l’air bien, mais je ne vois pas pourquoi on a incrusté des “asyncio” dans le tas, du coup ça m’a perdu et n’ai pas pu profiter des exemples cités, des exemples un peu plus basiques (si possible sans import par ex, juste avec des fonctionnalités de base) aurait très bien pu convenir pour illustrer l’idée derrière =/
Merci à tous pour les corrections. Effectivement, j’ai extrait ça d’un bout de code récent que j’ai refactoré, ce qui explique quelques copier/coller hasardeux et les bouts de asyncio qui trainent.
Un livre sympa et que je recommande à chaque développeur http://www.amazon.fr/Coder-proprement-Robert-C-Martin/dp/2744023272
Ce livre aborde les préceptes liés à l’écriture d’un code propre, maintenable, et évolutif, à lire absolument.
Les exemples sont en JAVA mais la philosophie est applicable à tout les langages.
Comment refactoriser son code, ne plus mettre de commentaires et donner des noms exhaustifs à ses variables, méthodes et fonctions, comment gérer les exceptions proprement sans polluer la logique métier qu’il traite… etc..
Attention, je ne recommande pas de retirer tous les commentaires. Les commentaires sont toujours utiles, simplement certains peuvent être remplacer avantageusement, maintenant, ou dans le future quand on aura la ressource pour le faire.
Je ne l’ai pas vérifié, mais j’ai l’impression qu’une abondance de commentaire est plutôt un signe de code mal découpé, qu’un signe de code parfait. (Du coup je suis d’accord avec le conseil pour le découpage en cas de commentaire).
Après c’est toujours pareil, au début tu fais des blocs normaux, et puis tu rajoutes des trucs, et tu les commente, et c’est quand tu reviens plus tard que tu t’apperçois que wow, ça en fait du code, faut découper tout ça.