Pourquoi préférer les variables locales aux variables d'instance?

La base de code sur laquelle je travaille utilise fréquemment des variables d'instance pour partager des données entre diverses méthodes triviales. Le développeur d'origine est catégorique sur le fait que cela respecte les meilleures pratiques énoncées dans le livre de l'oncle Bob / Robert Martin sur le code propre : "La première règle en matière de fonctions est qu'elles doivent être petites." et "Le nombre idéal d'arguments pour une fonction est zéro (niladique). (...) Les arguments sont difficiles. Ils prennent beaucoup de puissance conceptuelle."

Un exemple:

public class SomeBusinessProcess {
  @Inject private Router router;
  @Inject private ServiceClient serviceClient;
  @Inject private CryptoService cryptoService;

  private byte[] encodedData;
  private EncryptionInfo encryptionInfo;
  private EncryptedObject payloadOfResponse;
  private URI destinationURI;

  public EncryptedResponse process(EncryptedRequest encryptedRequest) {
    checkNotNull(encryptedRequest);

    getEncodedData(encryptedRequest);
    getEncryptionInfo();
    getDestinationURI();
    passRequestToServiceClient();

    return cryptoService.encryptResponse(payloadOfResponse);
  }

  private void getEncodedData(EncryptedRequest encryptedRequest) {
    encodedData = cryptoService.decryptRequest(encryptedRequest, byte[].class);
  }

  private void getEncryptionInfo() {
    encryptionInfo = cryptoService.getEncryptionInfoForDefaultClient();
  }

  private void getDestinationURI() {
    destinationURI = router.getDestination().getUri();
  }

  private void passRequestToServiceClient() {
    payloadOfResponse = serviceClient.handle(destinationURI, encodedData, encryptionInfo);
  }
}

Je reformulerais cela en utilisant les variables locales suivantes:

public class SomeBusinessProcess {
  @Inject private Router router;
  @Inject private ServiceClient serviceClient;
  @Inject private CryptoService cryptoService;

  public EncryptedResponse process(EncryptedRequest encryptedRequest) {
    checkNotNull(encryptedRequest);

    byte[] encodedData = cryptoService.decryptRequest(encryptedRequest, byte[].class);
    EncryptionInfo encryptionInfo = cryptoService.getEncryptionInfoForDefaultClient();
    URI destinationURI = router.getDestination().getUri();
    EncryptedObject payloadOfResponse = serviceClient.handle(destinationURI, encodedData,
      encryptionInfo);

    return cryptoService.encryptResponse(payloadOfResponse);
  }
}

Ceci est plus court, élimine le couplage de données implicite entre les différentes méthodes triviales et limite les portées variables au minimum requis. Pourtant, malgré ces avantages, je n'arrive toujours pas à convaincre le développeur initial que cette refactorisation est justifiée, car elle semble contredire les pratiques d'Oncle Bob mentionnées ci-dessus.

D'où mes questions: Quelle est la raison objective, scientifique, de privilégier les variables locales aux variables d'instance? Je n'arrive pas à mettre le doigt dessus. Mon intuition me dit que les couplages cachés sont mauvais et qu’une portée étroite vaut mieux qu’une large. Mais quelle est la science pour sauvegarder cela?

Et inversement, cette refactorisation a-t-elle des inconvénients que j'ai peut-être négligés?

Quel est l'objectif, la justification scientifique, de privilégier les variables locales aux variables d'instance?

La portée n'est pas un état binaire, c'est un dégradé. Vous pouvez les classer du plus grand au plus petit:

Global > Class > Local (method) > Local (code block, e.g. if, for, ...)

Edit: ce que j'appelle une "portée de classe" est ce que vous entendez par "variable d'instance". À ma connaissance, il en va de même, mais je suis un développeur C #, pas un développeur Java. Par souci de brièveté, j'ai regroupé toutes les statiques dans la catégorie globale car les statiques ne sont pas le sujet de la question.

Plus la portée est petite, mieux c'est. La raison en est que les variables doivent vivre dans la plus petite portée possible . Cela présente de nombreux avantages:

• Cela vous oblige à réfléchir à la responsabilité de la classe actuelle et vous aide à respecter le PÉR.
• Il vous permet de ne pas avoir d'éviter les conflits de noms globaux, par exemple si deux ou plusieurs classes ont une Namepropriété, vous n'êtes pas obligé d'eux comme préfixe FooName, BarName... Ainsi en gardant les noms des variables aussi propre et laconique que possible.
• Il décline le code en limitant les variables disponibles (par exemple, pour Intellisense) à celles qui sont pertinentes sur le plan contextuel.
• Il permet une forme de contrôle d'accès afin que vos données ne puissent pas être manipulées par un acteur inconnu (par exemple, une classe différente développée par un collègue).
• Cela rend le code plus lisible lorsque vous vous assurez que la déclaration de ces variables tente de rester aussi proche que possible de l'utilisation réelle de ces variables.
• Déclarer librement des variables dans un champ trop large est souvent révélateur d'un développeur qui ne comprend pas tout à fait la POO ou la manière de le mettre en œuvre. Le fait de voir des variables trop étendues a pour effet de signaler qu'il y a probablement un problème avec l'approche POO (avec le développeur en général ou la base de code en particulier).
• (Commentaire de Kevin) L'utilisation de la population locale vous oblige à faire les choses dans le bon ordre. Dans le code d'origine (variable de classe), vous pourriez vous déplacer incorrectement passRequestToServiceClient()en haut de la méthode et la compiler. Avec les sections locales, vous ne pouvez commettre cette erreur que si vous transmettez une variable non initialisée, ce qui est suffisamment évident pour que vous ne le fassiez pas.

Pourtant, malgré ces avantages, je n'arrive toujours pas à convaincre le développeur initial que cette refactorisation est justifiée, car elle semble contredire les pratiques d'Oncle Bob mentionnées ci-dessus.

Et inversement, cette refactorisation a-t-elle des inconvénients que j'ai peut-être négligés?

Le problème ici est que votre argument pour les variables locales est valide, mais vous avez également apporté des modifications supplémentaires qui ne sont pas correctes et qui entraînent l'échec du test d'odeur dans le correctif suggéré.

Bien que je comprenne votre suggestion "pas de variable de classe" et que cela ait du mérite, vous avez également supprimé les méthodes elles-mêmes, ce qui est un jeu totalement différent. Les méthodes auraient dû rester, et vous devriez plutôt les modifier pour renvoyer leur valeur plutôt que de la stocker dans une variable de classe:

private byte[] getEncodedData() {
    return cryptoService.decryptRequest(encryptedRequest, byte[].class);
}

private EncryptionInfo getEncryptionInfo() {
    return cryptoService.getEncryptionInfoForDefaultClient();
}

// and so on...

Je suis d'accord avec ce que vous avez fait dans la processméthode, mais vous auriez dû appeler les sous-méthodes privées plutôt que d'exécuter leurs corps directement.

public EncryptedResponse process(EncryptedRequest encryptedRequest) {
    checkNotNull(encryptedRequest);

    byte[] encodedData = getEncodedData();
    EncryptionInfo encryptionInfo = getEncryptionInfo();

    //and so on...

    return cryptoService.encryptResponse(payloadOfResponse);
}

Vous voudriez cette couche supplémentaire d'abstraction, en particulier lorsque vous rencontrez des méthodes qui doivent être réutilisées plusieurs fois. Même si vous ne réutilisez pas actuellement vos méthodes , il est néanmoins judicieux de déjà créer des sous-méthodes, le cas échéant, même si cela ne sert qu'à améliorer la lisibilité du code.

Indépendamment de l'argument de variable locale, j'ai immédiatement remarqué que votre solution suggérée est considérablement moins lisible que l'original. Je concède que l'utilisation gratuite des variables de classe porte également atteinte à la lisibilité du code, mais pas au premier abord comparé à ce que vous avez empilé toute la logique dans une seule méthode (maintenant longue).

Le code d'origine utilise des variables membres comme des arguments. Lorsqu'il dit minimiser le nombre d'arguments, il entend en réalité minimiser la quantité de données requise par les méthodes pour fonctionner. Mettre ces données dans des variables membres n'améliore rien.

D'autres réponses ont déjà expliqué parfaitement les avantages des variables locales, il ne reste donc que cette partie de votre question:

Pourtant, malgré ces avantages, je n'arrive toujours pas à convaincre le développeur initial que cette refactorisation est justifiée, car elle semble contredire les pratiques d'Oncle Bob mentionnées ci-dessus.

Cela devrait être facile. Indiquez-lui simplement la citation suivante dans le code propre de l'oncle Bob:

N'ayez aucun effet secondaire

Les effets secondaires sont des mensonges. Votre fonction promet de faire une chose, mais elle fait aussi d’autres choses cachées. Parfois, il apportera des modifications inattendues aux variables de sa propre classe. Parfois, cela les rend aux paramètres passés dans la fonction ou aux globals du système. Dans les deux cas, ce sont des faussetés sournoises et dommageables qui aboutissent souvent à des couplages temporels étranges et à des dépendances d'ordre.

(exemple omis)

Cet effet secondaire crée un couplage temporel. Autrement dit, checkPassword ne peut être appelé qu’à certaines heures (c’est-à-dire lorsque l’initialisation de la session est sûre). Si elle est appelée en panne, les données de session peuvent être perdues par inadvertance. Les couplages temporels sont source de confusion, surtout lorsqu'ils sont cachés comme effet secondaire. Si vous devez avoir un couplage temporel, vous devez l'indiquer clairement dans le nom de la fonction. Dans ce cas, nous pourrions renommer la fonction checkPasswordAndInitializeSession, même si cela enfreint à coup sûr «Faites une chose».

Autrement dit, oncle Bob ne dit pas simplement qu'une fonction devrait prendre peu d'arguments, il dit également que les fonctions devraient éviter d'interagir avec un état non local dans la mesure du possible.

"Cela contredit ce que pense l'oncle de quelqu'un" n'est JAMAIS un bon argument. JAMAIS. Ne prenez pas la sagesse des oncles, pensez par vous-même.

Cela dit, les variables d'instance devraient être utilisées pour stocker des informations devant effectivement être stockées de manière permanente ou semi-permanente. L'information ici n'est pas. Il est très simple de vivre sans les variables d'instance afin qu'elles puissent y aller.

Test: écrivez un commentaire sur la documentation pour chaque variable d'instance. Pouvez-vous écrire quelque chose qui ne soit pas complètement inutile? Et écrivez un commentaire de documentation aux quatre accesseurs. Ils sont également inutiles.

Le pire est de supposer le moyen de déchiffrer les modifications, car vous utilisez un cryptoService différent. Au lieu de devoir modifier quatre lignes de code, vous devez remplacer quatre variables d'instance par des variables différentes, quatre accesseurs par des variables différentes et modifier quatre lignes de code.

Mais bien sûr, la première version est préférable si vous êtes payé par la ligne de code. 31 lignes au lieu de 11 lignes. Trois fois plus de lignes à écrire et à conserver pour toujours, à lire lorsque vous déboguez quelque chose, à adapter lorsque des modifications sont nécessaires, à dupliquer si vous prenez en charge un second cryptoService.

(Vous avez oublié le point important selon lequel l'utilisation de variables locales vous oblige à effectuer les appels dans le bon ordre).

Quel est l'objectif, la justification scientifique, de privilégier les variables locales aux variables d'instance? Je n'arrive pas à mettre le doigt dessus. Mon intuition me dit que les couplages cachés sont mauvais et qu’une portée étroite vaut mieux qu’une large. Mais quelle est la science pour sauvegarder cela?

Les variables d'instance servent à représenter les propriétés de leur objet hôte et non à représenter des propriétés spécifiques aux threads de calcul dont l'étendue est plus étroite que l'objet lui-même. Certaines des raisons pour lesquelles une telle distinction a été établie et qui semblent ne pas avoir déjà été abordées tournent autour de la concurrence et de la réentrance. Si les méthodes échangent des données en définissant les valeurs des variables d'instance, deux threads simultanés peuvent facilement interagir entre leurs valeurs respectives pour ces variables d'instance, générant ainsi des bogues intermittents et difficiles à trouver.

Même un seul thread peut rencontrer des problèmes dans ce sens, car il existe un risque élevé qu'un modèle d'échange de données reposant sur des variables d'instance rend les méthodes non réentrantes. De même, si les mêmes variables sont utilisées pour transporter des données entre différentes paires de méthodes, un seul thread exécutant même une chaîne non récursive d'appels de méthodes risque de se heurter à des bogues liés à des modifications inattendues des variables d'instance impliquées.

Pour obtenir des résultats corrects et fiables dans un tel scénario, vous devez utiliser des variables distinctes pour communiquer entre chaque paire de méthodes, l'une invoquant l'autre, ou bien faire en sorte que chaque implémentation de méthode prenne en compte tous les détails d'implémentation de toutes les autres. méthodes qu'il invoque, directement ou indirectement. C'est cassant, et ça pèse mal.

En discutant simplement process(...), votre exemple de collègues est beaucoup plus lisible au sens de la logique métier. Inversement, votre contre-exemple nécessite plus qu'un simple coup d'œil pour en extraire le sens.

Cela étant dit, le code épuré est à la fois lisible et de bonne qualité. Pousser l'état local vers un espace plus global, c'est simplement un assemblage de haut niveau, donc un zéro pour la qualité.

public class SomeBusinessProcess {
  @Inject private Router router;
  @Inject private ServiceClient serviceClient;
  @Inject private CryptoService cryptoService;

  public EncryptedResponse process(EncryptedRequest request) {
    checkNotNull(encryptedRequest);

    return encryptResponse
      (routeTo
         ( destination()
         , requestData(request)
         , destinationEncryption()
         )
      );
  }

  private byte[] requestData(EncryptedRequest encryptedRequest) {
    return cryptoService.decryptRequest(encryptedRequest, byte[].class);
  }

  private EncryptionInfo destinationEncryption() {
    return cryptoService.getEncryptionInfoForDefaultClient();
  }

  private URI destination() {
    return router.getDestination().getUri();
  }

  private EncryptedObject routeTo(URI destinationURI, byte[] encodedData, EncryptionInfo encryptionInfo) {
    return serviceClient.handle(destinationURI, encodedData, encryptionInfo);
  }

  private void encryptResponse(EncryptedObject payloadOfResponse) {
    return cryptoService.encryptResponse(payloadOfResponse);
  }
}

Ceci est un rendu qui supprime le besoin de variables de toute portée. Oui, le compilateur les générera, mais l’important est qu’il contrôle cela pour que le code soit efficace. Tout en étant relativement lisible.

Juste un point sur le nommage. Vous voulez le nom le plus court qui soit significatif et qui développe les informations déjà disponibles. c'est à dire. destinationURI, l'URI est déjà connu sous le type signature.

Je voudrais juste supprimer ces variables et méthodes privées complètement. Voici mon refactor:

public class SomeBusinessProcess {
  @Inject private Router router;
  @Inject private ServiceClient serviceClient;
  @Inject private CryptoService cryptoService;

  public EncryptedResponse process(EncryptedRequest encryptedRequest) {
    return cryptoService.encryptResponse(
        serviceClient.handle(router.getDestination().getUri(),
        cryptoService.decryptRequest(encryptedRequest, byte[].class),
        cryptoService.getEncryptionInfoForDefaultClient()));
  }
}

Pour les méthodes privées, par exemple, router.getDestination().getUri()est plus clair et plus lisible que getDestinationURI(). Je voudrais juste répéter que si j'utilise deux fois la même ligne dans la même classe. En d'autres termes, s'il en faut un getDestinationURI(), il appartient probablement à une autre classe, pas à la SomeBusinessProcessclasse.

Pour les variables et les propriétés, il est généralement nécessaire de conserver les valeurs à utiliser ultérieurement. Si la classe n'a pas d'interface publique pour les propriétés, elles ne devraient probablement pas être des propriétés. Le pire type de propriétés de classe utilisé est probablement celui utilisé pour transmettre des valeurs entre méthodes privées par le biais d’effets secondaires.

Quoi qu'il en soit, la classe n'a plus qu'à faire process()et l'objet sera jeté, il n'est pas nécessaire de garder un état en mémoire. Un autre potentiel de refactorisation serait de supprimer le service Crypto de cette classe.

Sur la base des commentaires, je souhaite ajouter que cette réponse est basée sur les pratiques du monde réel. En effet, lors de la révision du code, la première chose que je choisirais est de refactoriser la classe et de déplacer le travail de chiffrement / déchiffrement. Une fois que cela est fait, je demanderais si les méthodes et les variables sont nécessaires, si elles sont nommées correctement, etc. Le code final sera probablement plus proche de ceci:

public class SomeBusinessProcess {
  @Inject private Router router;
  @Inject private ServiceClient serviceClient;

  public Response process(Request request) {
    return serviceClient.handle(router.getDestination().getUri());
  }
}

Avec le code ci-dessus, je ne pense pas qu'il ait besoin d'un refactor supplémentaire. Comme pour les règles, je pense qu’il faut de l’expérience pour savoir quand et quand ne pas les appliquer. Les règles ne sont pas des théories dont l'efficacité a été prouvée dans toutes les situations.

En revanche, la révision de code a un impact réel sur le délai d’exécution d’un morceau de code. Mon astuce consiste à avoir moins de code et à le rendre facile à comprendre. Un nom de variable peut être un sujet de discussion. Si je pouvais le supprimer, les réviseurs n'auraient même pas besoin d'y penser.

La réponse de Flater couvre assez bien les problèmes de cadrage, mais je pense qu'il y a un autre problème ici aussi.

Notez qu'il existe une différence entre une fonction qui traite des données et une fonction qui accède simplement aux données .

Le premier exécute la logique métier réelle, tandis que le second économise la saisie et ajoute peut-être une sécurité en ajoutant une interface plus simple et plus réutilisable.

Dans ce cas, il semble que les fonctions d'accès aux données n'enregistrent pas la saisie et ne sont réutilisées nulle part (sinon, leur suppression poserait d'autres problèmes). Donc, ces fonctions ne devraient tout simplement pas exister.

En ne conservant que la logique métier dans des fonctions nommées, nous obtenons le meilleur des deux mondes (quelque part entre la réponse de Flater et celle de imel96 ):

public EncryptedResponse process(EncryptedRequest encryptedRequest) {

    byte[] requestData = decryptRequest(encryptedRequest);
    EncryptedObject responseData = handleRequest(router.getDestination().getUri(), requestData, cryptoService.getEncryptionInfoForDefaultClient());
    EncryptedResponse response = encryptResponse(responseData);

    return response;
}

// define: decryptRequest(), handleRequest(), encryptResponse()

La première et la plus importante chose: Oncle Bob semble parfois être comme un prédicateur, mais affirme qu'il existe des exceptions à ses règles.

L’idée de Clean Code est d’améliorer la lisibilité et d’éviter les erreurs. Il y a plusieurs règles qui se violent les unes les autres.

Son argument sur les fonctions est que les fonctions niladiques sont les meilleures, mais que trois paramètres au maximum sont acceptables. Personnellement, je pense que 4 sont également ok.

Lorsque des variables d'instance sont utilisées, elles doivent constituer une classe cohérente. Cela signifie que les variables doivent être utilisées dans beaucoup, sinon toutes les méthodes non statiques.

Les variables qui ne sont pas utilisées dans de nombreux endroits de la classe doivent être déplacées.

Je ne considérerais ni la version originale ni la version refactorisée comme optimale, et @Flater a déjà très bien expliqué ce qui peut être fait avec les valeurs de retour. Il améliore la lisibilité et réduit les erreurs d’utilisation des valeurs de retour.

Les variables locales réduisent donc la portée, ce qui limite la manière dont les variables peuvent être utilisées, ce qui permet d'éviter certaines classes d'erreur et améliore la lisibilité.

Les variables d’instance réduisent les possibilités d’appel de la fonction, ce qui contribue également à réduire certaines classes d’erreur et améliore la lisibilité.

Dire que l'un a raison et que l'autre a tort peut être une conclusion valable dans un cas particulier, mais en tant que conseil général ...

TL; DR: Je pense que la raison pour laquelle vous sentez trop de zèle est qu'il y a trop de zèle.

Malgré le fait que les méthodes commençant par get ... ne doivent pas retourner à néant, la séparation des niveaux d'abstractions au sein des méthodes est donnée dans la première solution. Bien que la deuxième solution soit plus étendue, il est encore plus difficile de raisonner sur ce qui se passe dans la méthode. Les affectations de variables locales ne sont pas nécessaires ici. Je voudrais garder les noms de méthodes et refactoriser le code à quelque chose comme ça:

public class SomeBusinessProcess {
  @Inject private Router router;
  @Inject private ServiceClient serviceClient;
  @Inject private CryptoService cryptoService;

  public EncryptedResponse process(EncryptedRequest encryptedRequest) {
    checkNotNull(encryptedRequest);

    return getEncryptedResponse(
            passRequestToServiceClient(getDestinationURI(), getEncodedData(encryptedRequest) getEncryptionInfo())
        );
  }

  private EncryptedResponse getEncryptedResponse(EncryptedObject encryptedObject) {
    return cryptoService.encryptResponse(encryptedObject);
  }

  private byte[] getEncodedData(EncryptedRequest encryptedRequest) {
    return cryptoService.decryptRequest(encryptedRequest, byte[].class);
  }

  private EncryptionInfo getEncryptionInfo() {
    return cryptoService.getEncryptionInfoForDefaultClient();
  }

  private URI getDestinationURI() {
    return router.getDestination().getUri();
  }

  private EncryptedObject passRequestToServiceClient(URI destinationURI, byte[] encodedData, EncryptionInfo encryptionInfo) {
    return serviceClient.handle(destinationURI, encodedData, encryptionInfo);
  }
}

Les deux choses font la même chose et les différences de performance sont imperceptibles, donc je ne pense pas qu'il y ait d' argument scientifique . Cela revient alors à des préférences subjectives.

Et moi aussi j'ai tendance à aimer votre chemin mieux que celui de votre collègue. Pourquoi? Parce que je pense que c'est plus facile à lire et à comprendre, malgré ce que dit un auteur de livre.

Les deux méthodes permettent d'obtenir la même chose, mais sa méthode est plus étendue. Pour lire ce code, vous devez basculer entre plusieurs fonctions et variables membres. Tout n'est pas condensé au même endroit, vous devez vous rappeler tout cela dans votre tête pour le comprendre. C'est une charge cognitive beaucoup plus grande.

En revanche, votre approche emballe tout beaucoup plus dense, mais non pour le rendre impénétrable. Vous venez de le lire ligne après ligne, et vous n'avez pas besoin de mémoriser autant pour le comprendre.

Cependant, s'il a l' habitude de coder de la sorte, j'imagine que cela pourrait être l'inverse.