Comment refactoriser le code en un code commun?

Contexte

Je travaille sur un projet C # en cours. Je ne suis pas un programmeur C #, principalement un programmeur C ++. On m'a donc confié des tâches fondamentalement faciles et de refactorisation.

Le code est un gâchis. C'est un énorme projet. Comme notre client exigeait des versions fréquentes avec de nouvelles fonctionnalités et des corrections de bogues, tous les autres développeurs ont été forcés d'adopter une approche par force brute lors du codage. Le code est hautement impossible à maintenir et tous les autres développeurs sont d'accord avec lui.

Je ne suis pas ici pour débattre de savoir s'ils l'ont bien fait. En refactorisant, je me demande si je le fais de la bonne façon car mon code refactoré semble complexe! Voici ma tâche comme exemple simple.

Problème

Il y a six classes: A, B, C, D, Eet F. Toutes les classes ont une fonction ExecJob(). Les six implémentations sont très similaires. Fondamentalement, au début a A::ExecJob()été écrit. Ensuite, une version légèrement différente a été requise qui a été implémentée dans B::ExecJob()par copier-coller-modification de A::ExecJob(). Lorsqu'une autre version légèrement différente a été requise, a C::ExecJob()été écrite et ainsi de suite. Les six implémentations ont du code commun, puis des lignes de code différentes, puis à nouveau du code commun et ainsi de suite. Voici un exemple simple des implémentations:

A::ExecJob()
{
    S1;
    S2;
    S3;
    S4;
    S5;
}

B::ExecJob()
{
    S1;
    S3;
    S4;
    S5;
}

C::ExecJob()
{
    S1;
    S3;
    S4;
}

Où SNest un groupe d'exactement les mêmes déclarations.

Pour les rendre communs, j'ai créé une autre classe et déplacé le code commun dans une fonction. Utilisation de paramètres pour contrôler le groupe d'instructions à exécuter:

Base::CommonTask(param)
{
    S1;
    if (param.s2) S2;
    S3;
    S4;
    if (param.s5) S5;
}

A::ExecJob() // A inherits Base
{
    param.s2 = true;
    param.s5 = true;
    CommonTask(param);
}

B::ExecJob() // B inherits Base
{
    param.s2 = false;
    param.s5 = true;
    CommonTask(param);
}

C::ExecJob() // C inherits Base
{
    param.s2 = false;
    param.s5 = false;
    CommonTask(param);
}

Notez que cet exemple n'utilise que trois classes et des instructions simplifiées. En pratique, la CommonTask()fonction semble très complexe avec toutes ces vérifications de paramètres et il y a beaucoup plus d'instructions. En outre, dans le code réel, il existe plusieurs CommonTask()fonctions qui ressemblent à.

Bien que toutes les implémentations partagent un code commun et que les ExecJob()fonctions semblent plus mignonnes, il existe deux problèmes qui me dérangent:

• Pour tout changement CommonTask(), les six (et peut-être plus à l'avenir) fonctionnalités doivent être testées.
• CommonTask()est déjà complexe. Cela deviendra plus complexe au fil du temps.

Est-ce que je le fais de la bonne manière?

Oui, vous êtes absolument sur la bonne voie!

D'après mon expérience, j'ai remarqué que lorsque les choses sont compliquées, les changements se produisent par petites étapes. Ce que vous avez fait, c'est l' étape 1 du processus d'évolution (ou processus de refactoring). Voici les étapes 2 et 3:

Étape 2

class Base {
  method ExecJob() {
    S1();
    S2();
    S3();
    S4();
    S5();
  }
  method S1() { //concrete implementation }
  method S3() { //concrete implementation }
  method S4() { //concrete implementation}
  abstract method S2();
  abstract method S5();
}

class A::Base {
  method S2() {//concrete implementation}
  method S5() {//concrete implementation}
}

class B::Base {
  method S2() { // empty implementation}
  method S5() {//concrete implementation}
}

class C::Base {
  method S2() { // empty implementation}
  method S5() { // empty implementation}
}

Il s'agit du «modèle de conception de modèle» et c'est une longueur d'avance dans le processus de refactorisation. Si la classe de base change, les sous-classes (A, B, C) n'ont pas besoin d'être affectées. Vous pouvez ajouter de nouvelles sous-classes relativement facilement. Cependant, tout de suite sur l'image ci-dessus, vous pouvez voir que l'abstraction est cassée. La nécessité d'une «mise en œuvre vide» est un bon indicateur; cela montre qu'il y a un problème avec votre abstraction. Cela aurait pu être une solution acceptable à court terme, mais il semble y en avoir une meilleure.

Étape 3

interface JobExecuter {
  void executeJob();
}
class A::JobExecuter {
  void executeJob(){
     helper = new Helper();
     helper->S1();
     helper->S2();
     helper->S3();
     helper->S4();
     helper->S5();
  }
}

class B::JobExecuter {
  void executeJob(){
     helper = new Helper();
     helper->S1();
     helper->S3();
     helper->S4();
     helper->S5();
  }
}

class C::JobExecuter {
  void executeJob(){
     helper = new Helper();
     helper->S1();
     helper->S3();
     helper->S4();
  }
}

class Base{
   void ExecJob(JobExecuter executer){
       executer->executeJob();
   }
}

class Helper{
    void S1(){//Implementation} 
    void S2(){//Implementation}
    void S3(){//Implementation}
    void S4(){//Implementation} 
    void S5(){//Implementation}
}

Ceci est le «modèle de conception de stratégie» et semble être un bon choix pour votre cas. Il existe différentes stratégies pour exécuter le travail et chaque classe (A, B, C) l'implémente différemment.

Je suis sûr qu'il y a une étape 4 ou une étape 5 dans ce processus ou de bien meilleures approches de refactoring. Cependant, celui-ci vous permettra d'éliminer le code en double et de vous assurer que les modifications sont localisées.

Vous faites réellement la bonne chose. Je dis cela parce que:

1. Si vous devez changer le code d'une fonctionnalité de tâche commune, vous n'avez pas besoin de le changer dans les 6 classes qui contiendraient le code si vous ne l'écrivez pas dans une classe commune.
2. Le nombre de lignes de code sera diminué.

Vous voyez ce type de code partager beaucoup avec la conception pilotée par les événements (.NET en particulier). La manière la plus maintenable est de garder votre comportement partagé en aussi petits morceaux que possible.

Laissez le code de haut niveau réutiliser un tas de petites méthodes, laissez le code de haut niveau hors de la base partagée.

Vous aurez beaucoup de plaques de chaudière dans vos implémentations de feuilles / béton. Pas de panique, ça va. Tout ce code est direct, facile à comprendre. Vous devrez le réorganiser de temps en temps lorsque les choses se cassent, mais ce sera facile à changer.

Vous verrez beaucoup de modèles dans le code de haut niveau. Parfois, ils sont réels, la plupart du temps ils ne le sont pas. Les «configurations» des cinq paramètres là-haut se ressemblent , mais elles ne le sont pas. Ce sont trois stratégies entièrement différentes.

Vous devez également noter que vous pouvez faire tout cela avec la composition et ne jamais vous soucier de l'héritage. Vous aurez moins de couplage.

Si j'étais vous, j'ajouterai probablement 1 étape supplémentaire au début: une étude basée sur UML.

Refactoriser le code en fusionnant toutes les parties communes n'est pas toujours le meilleur choix, cela ressemble plus à une solution temporaire qu'à une bonne approche.

Dessine un schéma UML, gardez les choses simples mais efficaces, gardez à l'esprit certains concepts de base sur votre projet comme "qu'est-ce qui est censé faire ce logiciel?" "quelle est la meilleure façon de garder ce logiciel abstrait, modulaire, extensible, ... etc etc?" "comment je peux implémenter l'encapsulation à son meilleur?"

Je dis juste ceci: ne vous souciez pas du code en ce moment, vous n'avez qu'à vous soucier de la logique, quand vous avez une logique claire à l'esprit, tout le reste peut devenir une tâche vraiment facile, au final tout ce genre des problèmes que vous rencontrez est simplement causé par une mauvaise logique.

La toute première étape, peu importe où cela va, devrait être de diviser la méthode apparemment grande A::ExecJoben petits morceaux.

Par conséquent, au lieu de

A::ExecJob()
{
    S1; // many lines of code
    S2; // many lines of code
    S3; // many lines of code
    S4; // many lines of code
    S5; // many lines of code
}

vous obtenez

A::ExecJob()
{
    S1();
    S2();
    S3();
    S4();
    S5();
}

A:S1()
{
   // many lines of code
}

A:S2()
{
   // many lines of code
}

A:S3()
{
   // many lines of code
}

A:S4()
{
   // many lines of code
}

A:S5()
{
   // many lines of code
}

À partir d'ici, il existe de nombreuses façons de procéder. Mon point de vue: faites de A la classe de base de votre hiérarchie de classe et ExecJob virtuelle et il devient facile de créer B, C, ... sans trop copier-coller - remplacez simplement ExecJob (maintenant un cinq lignes) par une version modifiée version.

B::ExecJob()
{
    S1();
    S3();
    S4();
    S5();
}

Mais pourquoi avoir autant de cours? Peut-être que vous pouvez tous les remplacer par une seule classe qui a un constructeur qui peut être informé des actions nécessaires dans ExecJob.

Je suis d'accord avec les autres réponses selon lesquelles votre approche va dans le bon sens, même si je ne pense pas que l'héritage soit le meilleur moyen de mettre en œuvre un code commun - je préfère la composition. De la FAQ C ++ qui peut l'expliquer bien mieux que jamais: http://www.parashift.com/c++-faq/priv-inherit-vs-compos.html

Tout d' abord, vous devez vous assurer que l' héritage est vraiment l'outil ici pour le travail - seulement parce que vous avez besoin d' un lieu commun pour les fonctions utilisées par vos classes Apour Fne pas dire qu'une classe de base commune est la bonne chose ici - parfois une aide séparée la classe fait mieux le travail. Il se peut que ce soit le cas. Cela dépend de l'existence d'une relation «est-un» entre A à F et votre classe de base commune, impossible à dire à partir des noms artificiels AF. Ici vous trouverez un blog traitant de ce sujet.

Supposons que vous décidiez que la classe de base commune est la bonne chose dans votre cas. Ensuite , la deuxième chose que je voudrais faire est de vous assurer que vos fragments de code S1 à S5 sont chacun mis en œuvre une des méthodes distinctes S1()pour S5()votre classe de base. Ensuite, les fonctions "ExecJob" devraient ressembler à ceci:

A::ExecJob()
{
    S1();
    S2();
    S3();
    S4();
    S5();
}

B::ExecJob()
{
    S1();
    S3();
    S4();
    S5();
}

C::ExecJob()
{
    S1();
    S3();
    S4();
}

Comme vous le voyez maintenant, puisque S1 à S5 ne sont que des appels de méthode, aucun code ne bloque plus, la duplication de code a été presque complètement supprimée, et vous n'avez plus besoin de vérifier les paramètres, évitant ainsi le problème de complexité croissante que vous pourriez obtenir autrement.

Enfin, mais seulement dans une troisième étape (!), Vous pourriez penser à combiner toutes ces méthodes ExecJob dans une de votre classe de base, où l'exécution de ces parties peut être contrôlée par des paramètres, comme vous l'avez suggéré, ou en utilisant modèle de méthode de modèle. Vous devez décider vous-même si cela vaut la peine dans votre cas, sur la base du vrai code.

Mais à mon humble avis, la technique de base pour décomposer les grandes méthodes en petites méthodes est beaucoup, beaucoup plus importante pour éviter la duplication de code que pour appliquer des modèles.