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.