Réduire la complexité d'une classe

10

J'ai regardé quelques réponses et cherché sur Google, mais je n'ai rien trouvé d'utile (c'est-à-dire que cela n'aurait pas d'effets secondaires gênants).

Mon problème, en résumé, est que j'ai un objet et que je dois y effectuer une longue séquence d'opérations; Je le vois comme une sorte de chaîne de montage, comme construire une voiture.

Je crois que ces objets s'appelleraient des objets de méthode .

Donc, dans cet exemple, à un moment donné, j'aurais un CarWithoutUpholstery sur lequel j'aurais alors besoin d'exécuter installBackSeat, installFrontSeat, installWoodenInserts (les opérations n'interfèrent pas entre elles et pourraient même être effectuées en parallèle). Ces opérations sont effectuées par CarWithoutUpholstery.worker () et produisent un nouvel objet qui serait un CarWithUpholstery, sur lequel j'exécuterais peut-être alors cleanInsides (), verifyNoUpholsteryDefects (), etc.

Les opérations en une seule phase sont déjà indépendantes, c'est-à-dire que je suis déjà aux prises avec un sous-ensemble d'entre elles qui peuvent être exécutées dans n'importe quel ordre (les sièges avant et arrière peuvent être installés dans n'importe quel ordre).

Ma logique utilise actuellement Reflection pour la simplicité de mise en œuvre.

Autrement dit, une fois que j'ai un CarWithoutUpholstery, l'objet se inspecte pour les méthodes appelées performSomething (). À ce stade, il exécute toutes ces méthodes:

myObject.perform001SomeOperation();
myObject.perform002SomeOtherOperation();
...

tout en vérifiant les erreurs et les trucs. Bien que l'ordre des opérations soit sans importance, j'ai assigné un ordre lexicographique au cas où je découvre que l'ordre est important après tout. Cela contredit YAGNI , mais cela coûte très peu - un simple tri () - et cela pourrait économiser un changement de nom massif de la méthode (ou l'introduction d'une autre méthode pour effectuer des tests, par exemple un tableau de méthodes) sur toute la ligne.

Un exemple différent

Disons qu'au lieu de construire une voiture, je dois compiler un rapport de police secrète sur quelqu'un et le soumettre à mon Evil Overlord . Mon dernier objet sera un ReadyReport. Pour le construire je commence par rassembler des informations de base (nom, prénom, conjoint ...). Il s'agit de ma phase A. Selon qu'il y a un conjoint ou non, je devrai peut-être ensuite passer aux phases B1 ou B2 et recueillir des données sur la sexualité d'une ou deux personnes. Il s'agit de plusieurs requêtes différentes adressées à différents Evil Minions contrôlant la vie nocturne, les caméras de rue, les reçus de vente des sex-shops et autres. Et ainsi de suite.

Si la victime n'a pas de famille, je n'entrerai même pas dans la phase GetInformationAboutFamily, mais si je le fais, alors ce n'est pas pertinent si je cible d'abord le père ou la mère ou les frères et sœurs (le cas échéant). Mais je ne peux pas le faire si je n'ai pas effectué un FamilyStatusCheck, qui appartient donc à une phase antérieure.

Tout fonctionne à merveille ...

  • si j'ai besoin d'une opération supplémentaire, j'ai seulement besoin d'ajouter une méthode privée,
  • si l'opération est commune à plusieurs phases je peux la faire hériter d'une superclasse,
  • les opérations sont simples et autonomes. Aucune valeur d'une opération n'est jamais requise par aucune des autres (les opérations qui le sont sont effectuées dans une phase différente),
  • les objets en aval n'ont pas besoin d'effectuer de nombreux tests car ils ne pourraient même pas exister si leurs objets créateurs n'avaient pas vérifié ces conditions en premier lieu. C'est-à-dire, lorsque vous placez des insertions dans le tableau de bord, nettoyez le tableau de bord et vérifiez le tableau de bord, je n'ai pas besoin de vérifier qu'un tableau de bord est réellement .
  • il permet des tests faciles. Je peux facilement simuler un objet partiel et exécuter n'importe quelle méthode dessus, et toutes les opérations sont des boîtes noires déterministes.

...mais...

Le problème est survenu lorsque j'ai ajouté une dernière opération dans l'un de mes objets de méthode, ce qui a fait que le module global dépassait un indice de complexité obligatoire ("moins de N méthodes privées").

J'ai déjà pris la question à l'étage et suggéré que dans ce cas, la richesse des méthodes privées n'est pas révélatrice d'un désastre. La complexité est là, mais elle est là parce que l'opération est complexe, et en fait ce n'est pas si complexe - c'est juste long .

En utilisant l'exemple de Evil Overlord, mon problème est que Evil Overlord (alias He Who Shall Not Be Denied ) ayant demandé toutes les informations diététiques, mes Dietary Minions me disant que je dois interroger les restaurants, les kitchenettes, les vendeurs de rue, les vendeurs de rue sans licence, les serres propriétaires, etc., et le (sub) Overlord maléfique - connu sous le nom de celui qui ne doit pas non plus être refusé - se plaignant que j'exécute trop de requêtes dans la phase GetDietaryInformation.

Remarque : je suis conscient que de plusieurs points de vue, ce n'est pas du tout un problème (en ignorant les éventuels problèmes de performances, etc.). Tout ce qui se passe, c'est qu'une mesure spécifique n'est pas satisfaisante, et cela se justifie.

Ce que je pense pouvoir faire

Hormis la première, toutes ces options sont réalisables et, je pense, défendables.

  • J'ai vérifié que je peux être sournois et déclarer la moitié de mes méthodes protected. Mais j'exploiterais une faiblesse dans la procédure de test, et à part me justifier quand je suis attrapé, je n'aime pas ça. C'est aussi une mesure provisoire. Et si le nombre d'opérations requises double? Peu probable, mais quoi alors?
  • Je peux diviser arbitrairement cette phase en AnnealedObjectAlpha, AnnealedObjectBravo et AnnealedObjectCharlie, et avoir un tiers des opérations effectuées à chaque étape. J'ai l'impression que cela ajoute en fait de la complexité (N-1 classes supplémentaires), sans aucun avantage à part réussir un test. Je peux bien sûr considérer qu'un CarWithFrontSeatsInstalled et un CarWithAllSeatsInstalled sont logiquement des étapes successives . Le risque qu'une méthode Bravo soit plus tard requise par Alpha est petit, et encore plus petit si je la joue bien. Mais reste.
  • Je peux regrouper différentes opérations, à distance similaires, en une seule. performAllSeatsInstallation(). Il ne s'agit que d'une mesure provisoire et cela augmente la complexité de l'opération unique. Si jamais je dois faire les opérations A et B dans un ordre différent, et que je les ai regroupées dans E = (A + C) et F (B + D), je devrai dégrouper E et F et mélanger le code autour .
  • Je peux utiliser un tableau de fonctions lambda et contourner complètement le contrôle, mais je trouve cela maladroit. C'est cependant la meilleure alternative à ce jour. Cela éliminerait la réflexion. Les deux problèmes que j'ai sont que l'on me demanderait probablement de réécrire tous les objets de la méthode, pas seulement l'hypothétique CarWithEngineInstalled, et bien que ce soit une très bonne sécurité d'emploi, cela n'attire vraiment pas beaucoup; et que le vérificateur de couverture de code a des problèmes avec les lambdas (qui sont résolubles, mais quand même ).

Donc...

  • Quelle est, selon vous, ma meilleure option?
  • Y a-t-il une meilleure façon que je n'ai pas envisagée? ( peut - être que je ferais mieux de venir net et de demander directement de quoi il s'agit? )
  • Cette conception est-elle désespérément imparfaite, et je ferais mieux d'admettre la défaite et le fossé - cette architecture tout à fait? Pas bon pour ma carrière, mais écrire un code mal conçu serait-il meilleur à long terme?
  • Mon choix actuel est-il en fait le One True Way, et je dois me battre pour obtenir des métriques (et / ou des instruments) de meilleure qualité installés? Pour cette dernière option, j'aurais besoin de références ... Je ne peux pas simplement agiter la main au @PHB tout en murmurant Ce ne sont pas les mesures que vous recherchez . Peu importe combien j'aimerais pouvoir
LSerni
la source
3
Ou, vous pouvez ignorer l'avertissement "dépassement de la complexité maximale".
Robert Harvey
Si je pouvais je le ferais. D'une certaine manière, cette chose métrique a acquis une qualité sacrée qui lui est propre. Je continue d'essayer de convaincre le PTB de les accepter comme un outil utile, mais seulement comme un outil. Je pourrais encore réussir ...
LSerni
Les opérations construisent-elles réellement un objet, ou utilisez-vous simplement la métaphore de la chaîne d'assemblage car elle partage la propriété spécifique que vous mentionnez (de nombreuses opérations avec un ordre de dépendance partiel)? L'importance est que le premier suggère le modèle de constructeur, tandis que le second pourrait suggérer un autre modèle avec plus de détails remplis.
outis
2
La tyrannie des métriques. Les métriques sont des indicateurs utiles, mais les appliquer tout en ignorant pourquoi cette métrique est utilisée n'est pas utile.
Jaydee
2
Expliquez aux gens à l'étage la différence entre la complexité essentielle et la complexité accidentelle. en.wikipedia.org/wiki/No_Silver_Bullet Si vous êtes sûr que la complexité qui enfreint la règle est essentielle, alors si vous refactorisez par programmers.stackexchange.com/a/297414/51948 vous patinez peut-être autour de la règle et répandez sur la complexité. Si l'encapsulation des choses en phases n'est pas arbitraire (les phases sont liées au problème) et que la refonte réduit la charge cognitive pour les développeurs qui gèrent le code, alors il est logique de le faire.
Fuhrmanator du

Réponses:

15

La longue séquence d'opérations semble être sa propre classe, qui a alors besoin d'objets supplémentaires pour effectuer ses tâches. Il semble que vous soyez proche de cette solution. Cette longue procédure peut être décomposée en plusieurs étapes ou phases. Chaque étape ou phase peut être sa propre classe qui expose une méthode publique. Vous souhaitez que chaque phase implémente la même interface. Ensuite, votre chaîne de montage conserve une liste de phases.

Pour construire sur votre exemple d'assemblage de voiture, imaginez la Carclasse:

public class Car
{
    public IChassis Chassis { get; private set; }
    public Dashboard { get; private set; }
    public IList<Seat> Seats { get; private set; }

    public Car()
    {
        Seats = new List<Seat>();
    }

    public void AddChassis(IChassis chassis)
    {
        Chassis = chassis;
    }

    public void AddDashboard(Dashboard dashboard)
    {
        Dashboard = dashboard;
    }
}

Je vais laisser de côté les implémentations de certains composants, comme IChassis, Seatet Dashboardpar souci de concision.

Le Carn'est pas responsable de se construire. La chaîne d'assemblage est, alors encapsulons cela dans une classe:

public class CarAssembler
{
    protected List<IAssemblyPhase> Phases { get; private set; }

    public CarAssembler()
    {
        Phases = new List<IAssemblyPhase>()
        {
            new ChassisAssemblyPhase(),
            new DashboardAssemblyPhase(),
            new SeatAssemblyPhase()
        };
    }

    public void Assemble(Car car)
    {
        foreach (IAssemblyPhase phase in Phases)
        {
            phase.Assemble(car);
        }
    }
}

La distinction importante ici est que le CarAssembler a une liste d'objets de phase d'assemblage à implémenter IAssemblyPhase. Vous traitez maintenant explicitement avec des méthodes publiques, ce qui signifie que chaque phase peut être testée par elle-même, et votre outil de qualité de code est plus heureux parce que vous ne remplissez pas tellement une seule classe. Le processus d'assemblage est également très simple. Il suffit de parcourir les phases et d'appeler en Assemblepassant dans la voiture. Terminé. L' IAssemblyPhaseinterface est également très simple avec une seule méthode publique qui prend un objet Car:

public interface IAssemblyPhase
{
    void Assemble(Car car);
}

Maintenant, nous avons besoin de nos classes concrètes implémentant cette interface pour chaque phase spécifique:

public class ChassisAssemblyPhase : IAssemblyPhase
{
    public void Assemble(Car car)
    {
        car.AddChassis(new UnibodyChassis());
    }
}

public class DashboardAssemblyPhase : IAssemblyPhase
{
    public void Assemble(Car car)
    {
        Dashboard dashboard = new Dashboard();

        dashboard.AddComponent(new Speedometer());
        dashboard.AddComponent(new FuelGuage());
        dashboard.Trim = DashboardTrimType.Aluminum;

        car.AddDashboard(dashboard);
    }
}

public class SeatAssemblyPhase : IAssemblyPhase
{
    public void Assemble(Car car)
    {
        car.Seats.Add(new Seat());
        car.Seats.Add(new Seat());
        car.Seats.Add(new Seat());
        car.Seats.Add(new Seat());
    }
}

Chaque phase individuellement peut être assez simple. Certains ne sont qu'une doublure. Certains pourraient simplement aveugler l'ajout de quatre sièges, et un autre doit configurer le tableau de bord avant de l'ajouter à la voiture. La complexité de ce processus de longue haleine est divisée en plusieurs classes réutilisables qui sont facilement testables.

Même si vous avez dit que la commande n'avait pas d'importance en ce moment, elle pourrait le devenir à l'avenir.

Pour terminer, regardons le processus d'assemblage d'une voiture:

Car car = new Car();
CarAssembler assemblyLine = new CarAssembler();

assemblyLine.Assemble(car);

Vous pouvez sous-classer CarAssembler pour assembler un type spécifique de voiture ou de camion. Chaque phase est une unité dans un ensemble plus grand, ce qui facilite la réutilisation du code.


Cette conception est-elle désespérément défectueuse, et je ferais mieux d'admettre la défaite et le fossé - cette architecture tout à fait? Pas bon pour ma carrière, mais écrire un code mal conçu serait-il meilleur à long terme?

Si décider ce que j'ai écrit doit être réécrit était une marque noire dans ma carrière, alors je travaillerais comme creuseur de fossés en ce moment, et vous ne devriez pas suivre mes conseils. :)

Le génie logiciel est un processus éternel d'apprentissage, d'application, puis de réapprentissage. Ce n'est pas parce que vous décidez de réécrire votre code que vous allez être viré. Si tel était le cas, il n'y aurait pas d'ingénieurs logiciels.

Mon choix actuel est-il en fait le One True Way, et je dois me battre pour obtenir des métriques (et / ou des instruments) de meilleure qualité installés?

Ce que vous avez décrit est pas le seul vrai chemin, cependant garder à l' esprit que les paramètres de code sont également pas le seul vrai chemin. Les mesures de code doivent être considérées comme des avertissements. Ils peuvent signaler des problèmes de maintenance à l'avenir. Ce sont des lignes directrices, pas des lois. Lorsque votre outil de mesure de code pointe quelque chose, j'examine d'abord le code pour voir s'il implémente correctement les principaux SOLID . Plusieurs fois, le refactoring du code pour le rendre plus SOLIDE satisfera les outils de mesure du code. Parfois non. Vous devez prendre ces mesures au cas par cas.

Greg Burghardt
la source
1
Oui, beaucoup mieux que d'avoir une classe de dieu.
BЈовић
Cette approche fonctionne, mais surtout parce que vous pouvez obtenir les éléments pour la voiture sans paramètres. Et si vous aviez besoin de les passer? Ensuite, vous pouvez jeter tout cela par la fenêtre et repenser complètement la procédure, car vous n'avez pas vraiment une usine automobile avec 150 paramètres, c'est fou.
Andy
@DavidPacker: Même si vous devez paramétrer un tas de choses, vous pouvez également l'encapsuler dans une sorte de classe Config que vous passez dans le constructeur de votre objet "assembleur". Dans cet exemple de voiture, la couleur de la peinture, la couleur intérieure et les matériaux, l'ensemble de garniture, le moteur et bien plus peuvent être personnalisés, mais vous n'aurez pas nécessairement 150 personnalisations. Vous en aurez probablement une douzaine, ce qui se prête à un objet options.
Greg Burghardt
Mais seulement l'ajout de la configuration irait à l'encontre du but de la belle boucle for, ce qui est une honte énorme, car il vous faudrait analyser la configuration et l'assigner à des méthodes appropriées qui vous donneraient en retour des objets appropriés. Donc, vous vous retrouveriez probablement avec quelque chose de très similaire à ce qui était le design original.
Andy
Je vois. Au lieu d'avoir une classe et cinquante méthodes, j'aurais en fait cinquante classes maigres assembleur et une classe orchestrante. En fin de compte, le résultat semble le même, également en termes de performances, mais la disposition du code est plus propre. Je l'aime. Il sera un peu plus gênant d'ajouter des opérations (je devrai les configurer dans leur classe, plus informer la classe d'orchestration; je ne vois pas de moyen facile de sortir de cette nécessité), mais je l'aime toujours beaucoup. Permettez-moi quelques jours pour explorer cette approche.
LSerni
1

Je ne sais pas si cela est possible pour vous, mais je penserais à utiliser une approche plus orientée données. L'idée est que vous capturiez une expression (déclarative) des règles et contraintes, des opérations, des dépendances, de l'état, etc. changer d'état, en utilisant la capture déclarative des règles et des changements d'état plutôt qu'en utilisant du code plus directement. On pourrait utiliser des déclarations de données dans le langage de programmation, ou, certains outils DSL, ou un moteur de règles ou logique.

Erik Eidt
la source
Certaines opérations sont en fait implémentées de cette façon (sous forme de listes de règles). Pour garder l'exemple de la voiture, lors de l'installation d'une boîte de fusibles, de lumières et de fiches, je n'utilise pas en fait trois méthodes différentes, mais une seule, qui adapte en général les trucs électriques à deux broches en place, et prend la liste des trois listes de fusibles , lumières et prises. La grande majorité des autres méthodes ne se prêtent malheureusement pas facilement à cette approche.
LSerni
1

D'une certaine manière, le problème me rappelle les dépendances. Vous voulez une voiture dans une configuration spécifique. Comme Greg Burghardt, j'irais avec des objets / classes pour chaque étape / élément /….

Chaque étape déclare ce qu'elle ajoute au mix (ce qu'elle provides) (peut être plusieurs choses), mais aussi ce qu'elle requiert.

Ensuite, vous définissez la configuration finale requise quelque part, et disposez d'un algorithme qui examine ce dont vous avez besoin, et décide quelles étapes doivent être exécutées / quelles parties doivent être ajoutées / quels documents doivent être rassemblés.

Les algorithmes de résolution des dépendances ne sont pas si difficiles. Le cas le plus simple est celui où chaque étape fournit une chose, et pas deux choses fournissent la même chose, et les dépendances sont simples (pas de «ou» dans les dépendances). Pour les cas plus complexes: regardez simplement un outil comme debian apt ou quelque chose.

Enfin, la construction de votre voiture se réduit alors aux étapes possibles et laisse le CarBuilder déterminer les étapes nécessaires.

Cependant, une chose importante est que vous devez trouver un moyen d'informer le CarBuilder de toutes les étapes. Essayez de le faire en utilisant un fichier de configuration. L'ajout d'une étape supplémentaire n'aurait alors besoin que d'un ajout au fichier de configuration. Si vous avez besoin de logique, essayez d'ajouter un DSL dans le fichier de configuration. Si tout le reste échoue, vous êtes bloqué en les définissant dans le code.

Sjoerd Job Postmus
la source
0

Quelques choses que vous mentionnez me paraissent «mauvaises»

"Ma logique utilise actuellement Reflection pour la simplicité de mise en œuvre"

Il n'y a vraiment aucune excuse pour cela. utilisez-vous vraiment la comparaison de chaînes de noms de méthode pour déterminer quoi exécuter?! si vous changez la commande, devrez-vous changer le nom de la méthode?!

"Les opérations en une seule phase sont déjà indépendantes"

S'ils sont vraiment indépendants les uns des autres, cela suggère que votre classe a assumé plus d'une responsabilité. Pour moi, vos deux exemples semblent se prêter à une sorte de single

MyObject.AddComponent(IComponent component) 

approche qui vous permettrait de diviser la logique de chaque opération en sa propre classe ou ses propres classes.

En fait j'imagine qu'ils ne sont pas vraiment indépendants, j'imagine que chacun examine l'état de l'objet et le modifie, ou au moins effectue une sorte de validation avant de commencer.

Dans ce cas, vous pouvez soit:

  1. Jetez la POO par la fenêtre et ayez des services qui opèrent sur les données exposées dans votre classe, par exemple.

    Service.AddDoorToCar (Voiture de voiture)

  2. Avoir une classe de constructeur qui construit votre objet en assemblant d'abord les données nécessaires et en renvoyant l'objet terminé, c'est-à-dire.

    Car = CarBuilder.WithDoor (). WithDoor (). WithWheels ();

(la logique withX peut à nouveau être divisée en sous-constructeurs)

  1. Adopter une approche fonctionnelle et passer des fonctions / délégués dans une méthode de modification

    Car.Modify ((c) => c.doors ++);

Ewan
la source
Ewan a dit: "Jetez la POO par la fenêtre et ayez des services qui opèrent sur les données exposées dans votre classe" --- Comment est-ce que ce n'est pas orienté objet?
Greg Burghardt
?? ce n'est pas le cas, c'est une option non OO
Ewan
Vous utilisez des objets, ce qui signifie qu'il est "orienté objet". Ce n'est pas parce que vous ne pouvez pas identifier un modèle de programmation pour votre code qu'il n'est pas orienté objet. Les principes SOLIDES sont une bonne règle à suivre. Si vous mettez en œuvre tout ou partie des principes SOLID, il est orienté objet. Vraiment, si vous utilisez des objets et pas seulement en passant des structures à différentes procédures ou méthodes statiques, il est orienté objet. Il y a une différence entre "code orienté objet" et "bon code". Vous pouvez écrire du mauvais code orienté objet.
Greg Burghardt
son «pas OO» parce que vous exposez les données comme s'il s'agissait d'une structure et que vous l'utilisez dans une classe distincte comme dans une procédure
Ewan
tbh J'ai seulement ajouté le commentaire principal pour essayer d'arrêter l'inévitable "ce n'est pas OOP!" commentaires
Ewan