Trop d'abstraction rendant le code difficile à étendre

Je suis confronté à des problèmes avec ce que je ressens comme étant trop d'abstraction dans la base de code (ou du moins à y faire face). La plupart des méthodes de la base de code ont été abstraites pour prendre le parent A le plus élevé dans la base de code, mais l'enfant B de ce parent a un nouvel attribut qui affecte la logique de certaines de ces méthodes. Le problème est que ces attributs ne peuvent pas être vérifiés dans ces méthodes car l'entrée est abstraite de A, et A bien sûr n'a pas cet attribut. Si j'essaie de créer une nouvelle méthode pour gérer B différemment, elle est appelée pour la duplication de code. La suggestion de mon responsable technique est de créer une méthode partagée qui accepte les paramètres booléens, mais le problème avec cela est que certaines personnes considèrent cela comme un "flux de contrôle caché", où la méthode partagée a une logique qui peut ne pas être évidente pour les futurs développeurs. , et cette méthode partagée deviendra trop complexe / alambiquée une fois si de futurs attributs doivent être ajoutés, même si elle est décomposée en méthodes partagées plus petites. Cela augmente également le couplage, diminue la cohésion et viole le principe de responsabilité unique, comme l'a souligné un membre de mon équipe.

Essentiellement, une grande partie de l'abstraction dans cette base de code aide à réduire la duplication de code, mais cela rend l'extension / la modification des méthodes plus difficiles lorsqu'elles sont faites pour prendre l'abstraction la plus élevée. Que dois-je faire dans une telle situation? Je suis au centre du blâme, même si tout le monde ne peut pas s'entendre sur ce qu'ils considèrent comme bon, donc ça me fait mal au final.

Si j'essaie de créer une nouvelle méthode pour gérer B différemment, elle est appelée pour la duplication de code.

Toutes les duplications de code ne sont pas créées égales.

Supposons que vous ayez une méthode qui prend deux paramètres et les ajoute ensemble appelés total(). Supposons que vous en appeliez un autre add(). Leurs implémentations semblent complètement identiques. Doivent-ils être fusionnés en une seule méthode? NON!!!

Le principe Don't-Repeat-Yourself ou DRY ne consiste pas à répéter du code. Il s'agit de diffuser une décision, une idée, de sorte que si vous changez votre idée, vous devez réécrire partout où vous répandez cette idée. Blegh. C'est terrible. Ne le fais pas. Utilisez plutôt DRY pour vous aider à prendre des décisions en un seul endroit .

Le principe SEC (ne vous répétez pas) énonce:

Chaque élément de connaissance doit avoir une représentation unique, non ambiguë et faisant autorité au sein d'un système.

wiki.c2.com - Ne vous répétez pas

Mais DRY peut être corrompu dans une habitude de scanner du code à la recherche d'une implémentation similaire qui semble être un copier-coller d'un autre endroit. Ceci est la forme morte du cerveau de SEC. Enfer, vous pouvez le faire avec un outil d'analyse statique. Cela n'aide pas car il ignore le point de DRY qui est de garder le code flexible.

Si mes besoins totaux changent, je devrai peut-être modifier ma totalmise en œuvre. Cela ne signifie pas que je dois changer ma addmise en œuvre. Si un goober les a réunis en une seule méthode, je souffre maintenant d'un peu de douleur inutile.

Quelle douleur? Je pourrais sûrement copier le code et créer une nouvelle méthode quand j'en aurais besoin. Donc ce n'est pas grave, non? Malarky! Si rien d'autre vous me coûte un bon nom! Les bons noms sont difficiles à trouver et ne répondent pas bien lorsque vous jouez avec leur sens. Les bons noms, qui rendent l'intention claire, sont plus importants que le risque que vous ayez copié un bogue qui, franchement, est plus facile à corriger lorsque votre méthode a le bon nom.

Donc, mon conseil est d'arrêter de laisser les réactions instinctives à un code similaire lier votre base de code en nœuds. Je ne dis pas que vous êtes libre d'ignorer le fait qu'il existe des méthodes et de copier et coller à la place. Non, chaque méthode doit avoir une putain de bonne réputation qui soutient la seule idée dont il s'agit. Si sa mise en œuvre correspond à la mise en œuvre d'une autre bonne idée, à l'heure actuelle, qui s'en soucie?

D'un autre côté, si vous avez une sum()méthode qui a une implémentation identique ou même différente de celle-ci total(), mais à chaque fois que vos exigences totales changent, vous devez changer, sum()il y a de fortes chances que ce soit la même idée sous deux noms différents. Non seulement le code serait plus flexible s'il était fusionné, mais il serait moins déroutant à utiliser.

Quant aux paramètres booléens, oui, c'est une odeur de code désagréable. Non seulement le contrôle du flux est un problème, mais il montre que vous avez coupé une abstraction à un mauvais moment. Les abstractions sont censées rendre les choses plus simples à utiliser, pas plus compliquées. Passer des bools à une méthode pour contrôler son comportement, c'est comme créer un langage secret qui décide quelle méthode vous appelez vraiment. Aïe! Ne me fais pas ça. Donnez à chaque méthode son propre nom, sauf si vous avez un polymorphisme honnête .

Maintenant, vous semblez épuisé par l'abstraction. C'est dommage car l'abstraction est une chose merveilleuse lorsqu'elle est bien faite. Vous l'utilisez beaucoup sans y penser. Chaque fois que vous conduisez une voiture sans avoir à comprendre le système de pignon et crémaillère, chaque fois que vous utilisez une commande d'impression sans penser aux interruptions du système d'exploitation, et chaque fois que vous vous brossez les dents sans penser à chaque poil individuel.

Non, le problème auquel vous semblez confronté est une mauvaise abstraction. Abstraction créée pour servir un objectif différent de vos besoins. Vous avez besoin d'interfaces simples dans des objets complexes qui vous permettent de demander que vos besoins soient satisfaits sans jamais avoir à comprendre ces objets.

Lorsque vous écrivez du code client qui utilise un autre objet, vous savez quels sont vos besoins et ce dont vous avez besoin de cet objet. Ce n'est pas le cas. C'est pourquoi le code client possède l'interface. Lorsque vous êtes le client, rien ne vous dit quels sont vos besoins, sauf vous. Vous mettez en place une interface qui montre quels sont vos besoins et exigez que tout ce qui vous est remis réponde à ces besoins.

Voilà l'abstraction. En tant que client, je ne sais même pas à quoi je parle. Je sais juste ce dont j'ai besoin. Si cela signifie que vous devez envelopper quelque chose pour changer son interface avant de me le remettre très bien. Je m'en fiche. Faites juste ce que j'ai besoin de faire. Arrêtez de compliquer les choses.

Si je dois regarder à l'intérieur d'une abstraction pour comprendre comment l'utiliser, l'abstraction a échoué. Je ne devrais pas avoir besoin de savoir comment cela fonctionne. Juste que ça marche. Donnez-lui un bon nom et si je regarde à l'intérieur, je ne devrais pas être surpris par ce que je trouve. Ne me faites pas continuer à regarder à l'intérieur pour vous souvenir de comment l'utiliser.

Lorsque vous insistez sur le fait que l'abstraction fonctionne de cette façon, le nombre de niveaux derrière elle n'a pas d'importance. Tant que vous ne regardez pas derrière l'abstraction. Vous insistez pour que l'abstraction soit conforme à vos besoins et ne s'adapte pas à la sienne. Pour que cela fonctionne, il doit être facile à utiliser, avoir un bon nom et ne pas fuir .

C'est l'attitude qui a engendré l'injection de dépendance (ou tout simplement faire référence si vous êtes de la vieille école comme moi). Cela fonctionne bien avec la préférence de composition et de délégation sur l'héritage . L'attitude porte de nombreux noms. Mon préféré est de dire, ne demandez pas .

Je pourrais te noyer dans les principes toute la journée. Et il semble que vos collègues le soient déjà. Mais voici le problème: contrairement à d'autres domaines de l'ingénierie, ce logiciel a moins de 100 ans. Nous sommes tous encore en train de le comprendre. Donc, ne laissez pas quelqu'un qui apprend beaucoup de livres intimidants vous intimider pour écrire du code difficile à lire. Écoutez-les, mais insistez pour qu'ils aient un sens. Ne prenez rien sur la foi. Les gens qui codent d'une certaine façon juste parce qu'on leur a dit que c'était le chemin sans savoir pourquoi faire le plus gros gâchis.

Le dicton habituel que nous lisons tous ici et là est:

Tous les problèmes peuvent être résolus en ajoutant une autre couche d'abstraction.

Eh bien, ce n'est pas vrai! Votre exemple le montre. Je proposerais donc la déclaration légèrement modifiée (n'hésitez pas à réutiliser ;-)):

Chaque problème peut être résolu en utilisant LE BON niveau d'abstraction.

Il y a deux problèmes différents dans votre cas:

• la généralisation excessive provoquée par l'ajout de chaque méthode au niveau abstrait;
• la fragmentation des comportements concrets qui donne l'impression de ne pas avoir une vue d'ensemble et de se sentir perdu. Un peu comme dans une boucle d'événements Windows.

Les deux sont corrélés:

• si vous abstenez une méthode où chaque spécialisation le fait différemment, tout va bien. Personne n'a de problème à comprendre qu'un Shapepeut le calculer surface()de manière spécialisée.

• Si vous faites abstraction d'une opération où il existe un modèle de comportement général commun, vous avez deux choix:

  • soit vous répéterez le comportement courant dans chaque spécialisation: c'est très redondant; et difficile à entretenir, notamment pour assurer que la partie commune reste en ligne dans les spécialisations:
  • vous utilisez une sorte de variante du modèle de méthode de modèle : cela vous permet de prendre en compte le comportement commun en utilisant des méthodes abstraites supplémentaires qui peuvent être facilement spécialisées. C'est moins redondant, mais les comportements supplémentaires ont tendance à devenir extrêmement divisés. Trop signifierait que c'est peut-être trop abstrait.

De plus, cette approche pourrait entraîner un effet de couplage abstrait au niveau de la conception. Chaque fois que vous souhaitez ajouter une sorte de nouveau comportement spécialisé, vous devrez l'abstraire, changer le parent abstrait et mettre à jour toutes les autres classes. Ce n'est pas le genre de propagation de changement que l'on peut souhaiter. Et ce n'est pas vraiment dans l'esprit des abstractions qui ne dépendent pas de la spécialisation (au moins dans la conception).

Je ne connais pas votre design et je ne peux pas vous aider davantage. C'est peut-être vraiment un problème très complexe et abstrait et il n'y a pas de meilleur moyen. Mais quelles sont les chances? Les symptômes d'une surgénéralisation sont là. Peut-être qu'il est temps de le revoir et de considérer la composition plutôt que la généralisation ?

Chaque fois que je vois une méthode dont le comportement active le type de son paramètre, je considère immédiatement si cette méthode appartient réellement au paramètre de la méthode. Par exemple, au lieu d'avoir une méthode comme:

public void sort(List values) {
    if (values instanceof LinkedList) {
        // do efficient linked list sort
    } else { // ArrayList
        // do efficient array list sort
    }
}

Je ferais ceci:

values.sort();

// ...

class ArrayList {
    public void sort() {
        // do efficient array list sort
    }
}

class LinkedList {
    public void sort() {
        // do efficient linked list sort
    }
}

Nous déplaçons le comportement à l'endroit qui sait quand l'utiliser. Nous créons une véritable abstraction où vous n'avez pas besoin de connaître les types ou les détails de l'implémentation. Pour votre situation, il peut être plus judicieux de déplacer cette méthode de la classe d'origine (que j'appellerai O) pour la taper Aet la remplacer dans type B. Si la méthode est appelée doItsur un objet, déplacez-vous doItvers Aet remplacez par le comportement différent dans B. S'il y a des bits de données d'où doItest appelé à l' origine, ou si la méthode est utilisée en assez de place, vous pouvez laisser la méthode originale et déléguée:

class O {
    int x;
    int y;

    public void doIt(A a) {
        a.doIt(this.x, this.y);
    }
}

Nous pouvons cependant plonger un peu plus profondément. Examinons la suggestion d'utiliser un paramètre booléen à la place et voyons ce que nous pouvons apprendre sur la façon dont votre collègue pense. Sa proposition est de faire:

public void doIt(A a, boolean isTypeB) {
    if (isTypeB) {
        // do B stuff
    } else { 
        // do A stuff
    }
}

Cela ressemble énormément à celui que instanceofj'ai utilisé dans mon premier exemple, sauf que nous extériorisons cette vérification. Cela signifie que nous devrions l'appeler de deux manières:

o.doIt(a, a instanceof B);

ou:

o.doIt(a, true); //or false

Dans le premier cas, le point d'appel n'a aucune idée de son type A. Par conséquent, devrions-nous passer des booléens tout le long? Est-ce vraiment un modèle que nous voulons partout dans la base de code? Que se passe-t-il s'il existe un troisième type dont nous devons tenir compte? Si c'est ainsi que la méthode est appelée, nous devons la déplacer vers le type et laisser le système choisir l'implémentation pour nous de manière polymorphe.

Dans le deuxième cas, nous devons déjà connaître le type de apoint d'appel. Habituellement, cela signifie que nous y créons l'instance ou prenons une instance de ce type comme paramètre. La création d'une méthode Oqui prend un Bici fonctionnerait. Le compilateur saurait quelle méthode choisir. Lorsque nous traversons des changements comme celui-ci, la duplication est préférable à la création de la mauvaise abstraction , du moins jusqu'à ce que nous sachions où nous allons vraiment. Bien sûr, je suggère que nous n'avons pas vraiment terminé, peu importe ce que nous avons changé jusqu'à présent.

Nous devons examiner de plus près la relation entre Aet B. En général, on nous dit que nous devrions privilégier la composition plutôt que l'héritage . Ce n'est pas vrai dans tous les cas, mais c'est vrai dans un nombre surprenant de cas une fois que nous creusons. BHérite de A, ce qui signifie que nous croyons Bêtre un A. Bdevrait être utilisé comme A, sauf qu'il fonctionne un peu différemment. Mais quelles sont ces différences? Pouvons-nous donner un nom plus concret aux différences? N'est-ce pas Bun A, mais a vraiment Aun Xqui pourrait être A'ou B'? À quoi ressemblerait notre code si nous faisions cela?

Si nous déplacions la méthode Acomme suggéré précédemment, nous pourrions injecter une instance de Xdans Aet déléguer cette méthode à X:

class A {
    X x;
    A(X x) {
        this.x = x;
    }

    public void doIt(int x, int y) {
        x.doIt(x, y);
    }
}

Nous pouvons mettre en œuvre A'et B', et nous en débarrasser B. Nous avons amélioré le code en donnant un nom à un concept qui aurait pu être plus implicite, et nous nous sommes permis de définir ce comportement lors de l'exécution au lieu de la compilation. Aest également devenu moins abstrait. Au lieu d'une relation d'héritage étendue, il appelle des méthodes sur un objet délégué. Cet objet est abstrait, mais plus axé uniquement sur les différences de mise en œuvre.

Il y a cependant une dernière chose à regarder. Revenons à la proposition de votre collègue. Si sur tous les sites d'appels nous connaissons explicitement le type que Anous avons, alors nous devrions faire des appels comme:

B b = new B();
o.doIt(b, true);

Nous avons supposé plus tôt lors de la composition qui Aa un Xqui est soit A'ou B'. Mais peut-être même que cette hypothèse n'est pas correcte. Est-ce le seul endroit où cette différence entre Aet Bimporte? Si c'est le cas, nous pourrions peut-être adopter une approche légèrement différente. Nous avons toujours un Xqui est A'ou B', mais il n'appartient pas A. Ne O.doIts'en soucie que, alors passons-le uniquement à O.doIt:

class O {
    int x;
    int y;

    public void doIt(A a, X x) {
        x.doIt(a, x, y);
    }
}

Maintenant, notre site d'appel ressemble à:

A a = new A();
o.doIt(a, new B'());

Une fois de plus, Bdisparaît et l'abstraction se déplace vers le plus concentré X. Cette fois, cependant, Aest encore plus simple en sachant moins. C'est encore moins abstrait.

Il est important de réduire la duplication dans une base de code, mais nous devons considérer pourquoi la duplication se produit en premier lieu. La duplication peut être le signe d'abstractions plus profondes qui tentent de sortir.

L'abstraction par héritage peut devenir assez laide. Hiérarchies de classes parallèles avec des usines typiques. Le refactoring peut devenir un mal de tête. Et aussi le développement ultérieur, l'endroit où vous vous trouvez.

Il existe une alternative: des points d'extension , des abstractions strictes et une personnalisation à plusieurs niveaux. Dites une personnalisation des clients gouvernementaux, basée sur cette personnalisation pour une ville spécifique.

Un avertissement: Malheureusement, cela fonctionne mieux lorsque toutes (ou la plupart) des classes sont rendues extensibles. Aucune option pour vous, peut-être en petit.

Cette extensibilité fonctionne en ayant une classe de base d'objet extensible contenant des extensions:

void f(CreditorBO creditor) {
    creditor.as(AllowedCreditorBO.class).ifPresent(allowedCreditor -> ...);
}

En interne, il existe un mappage paresseux des objets aux objets étendus par classe d'extension.

Pour les classes et composants GUI, la même extensibilité, en partie avec héritage. Ajout de boutons et autres.

Dans votre cas, une validation doit vérifier si elle est étendue et se valider par rapport aux extensions. L'introduction de points d'extension juste pour un cas ajoute du code incompréhensible, pas bon.

Il n'y a donc pas d'autre solution que d'essayer de travailler dans le contexte actuel.

«Contrôle de flux caché» me semble trop ondulatoire.
Toute construction ou élément hors contexte peut avoir cette caractéristique.

Les abstractions sont bonnes. Je les tempère avec deux lignes directrices:

• Mieux vaut ne pas résumer trop tôt. Attendez plus d'exemples de motifs avant d'abstraction. «Plus» est bien sûr subjectif et spécifique à la situation difficile.

• Évitez trop de niveaux d'abstraction simplement parce que l'abstraction est bonne. Un programmeur devra garder ces niveaux dans sa tête pour le code nouveau ou modifié car ils sonder la base de code et aller 12 niveaux en profondeur. Le désir d'un code bien résumé peut conduire à tant de niveaux qu'ils sont difficiles à suivre pour de nombreuses personnes. Cela conduit également à des bases de code «uniquement maintenues par les ninjas».

Dans les deux cas, «plus et« trop »ne sont pas des nombres fixes. Ça dépend. C'est ce qui rend les choses difficiles.

J'aime aussi cet article de Sandi Metz

https://www.sandimetz.com/blog/2016/1/20/the-wrong-abstraction

la duplication est beaucoup moins chère que la mauvaise abstraction
et
préfère la duplication à la mauvaise abstraction