Quelle est la meilleure façon de refactoriser une méthode qui a trop de paramètres (6+)?

Parfois, je rencontre des méthodes avec un nombre inconfortable de paramètres. Le plus souvent, ils semblent être des constructeurs. Il semble qu'il devrait y avoir un meilleur moyen, mais je ne vois pas ce que c'est.

return new Shniz(foo, bar, baz, quux, fred, wilma, barney, dino, donkey)

J'ai pensé à utiliser des structures pour représenter la liste des paramètres, mais cela semble simplement déplacer le problème d'un endroit à un autre et créer un autre type dans le processus.

ShnizArgs args = new ShnizArgs(foo, bar, baz, quux, fred, wilma, barney, dino, donkey)
return new Shniz(args);

Cela ne semble donc pas être une amélioration. Alors, quelle est la meilleure approche?

Le meilleur moyen serait de trouver des moyens de regrouper les arguments. Cela suppose, et ne fonctionne vraiment que si, vous vous retrouverez avec plusieurs "groupements" d'arguments.

Par exemple, si vous transmettez la spécification d'un rectangle, vous pouvez transmettre x, y, largeur et hauteur ou vous pouvez simplement passer un objet rectangle contenant x, y, largeur et hauteur.

Recherchez des éléments comme celui-ci lors de la refactorisation pour le nettoyer un peu. Si les arguments ne peuvent vraiment pas être combinés, commencez à voir si vous avez une violation du principe de responsabilité unique.

Je vais supposer que vous voulez dire C # . Certaines de ces choses s'appliquent également à d'autres langues.

Vous avez plusieurs options:

passer du constructeur aux setters de propriété . Cela peut rendre le code plus lisible, car il est évident pour le lecteur quelle valeur correspond à quels paramètres. La syntaxe de l'initialisation d'objet rend cela agréable. Il est également simple à implémenter, car vous pouvez simplement utiliser des propriétés générées automatiquement et ignorer l'écriture des constructeurs.

class C
{
    public string S { get; set; }
    public int I { get; set; }
}

new C { S = "hi", I = 3 };

Cependant, vous perdez l'immuabilité et vous perdez la possibilité de vous assurer que les valeurs requises sont définies avant d'utiliser l'objet au moment de la compilation.

Modèle de constructeur .

Pensez à la relation entre stringet StringBuilder. Vous pouvez l'obtenir pour vos propres cours. J'aime l'implémenter en tant que classe imbriquée, donc la classe Ca une classe associée C.Builder. J'aime aussi une interface fluide sur le constructeur. Bien fait, vous pouvez obtenir une syntaxe comme celle-ci:

C c = new C.Builder()
    .SetX(4)    // SetX is the fluent equivalent to a property setter
    .SetY("hello")
    .ToC();     // ToC is the builder pattern analog to ToString()

// Modify without breaking immutability
c = c.ToBuilder().SetX(2).ToC();

// Still useful to have a traditional ctor:
c = new C(1, "...");

// And object initializer syntax is still available:
c = new C.Builder { X = 4, Y = "boing" }.ToC();

J'ai un script PowerShell qui me permet de générer le code du générateur pour faire tout cela, où l'entrée ressemble à:

class C {
    field I X
    field string Y
}

Je peux donc générer au moment de la compilation. partialles classes me permettent d'étendre à la fois la classe principale et le générateur sans modifier le code généré.

Refactoring "Introduce Parameter Object" . Voir le catalogue de refactoring . L'idée est que vous prenez certains des paramètres que vous passez et les mettez dans un nouveau type, puis passez une instance de ce type à la place. Si vous faites cela sans réfléchir, vous reviendrez là où vous avez commencé:

new C(a, b, c, d);

devient

new C(new D(a, b, c, d));

Cependant, cette approche a le plus grand potentiel pour avoir un impact positif sur votre code. Alors, continuez en suivant ces étapes:

1. Recherchez des sous - ensembles de paramètres qui ont du sens ensemble. Regrouper sans réfléchir tous les paramètres d'une fonction ne vous apporte pas grand-chose; le but est d'avoir des groupements qui ont du sens. Vous saurez que vous avez raison lorsque le nom du nouveau type est évident.

2. Recherchez d'autres emplacements où ces valeurs sont utilisées ensemble et utilisez également le nouveau type. Il y a de fortes chances que, lorsque vous avez trouvé un bon nouveau type pour un ensemble de valeurs que vous utilisez déjà partout, ce nouveau type aura également un sens dans tous ces endroits.

3. Recherchez les fonctionnalités qui se trouvent dans le code existant, mais qui appartiennent au nouveau type.

Par exemple, vous voyez peut-être du code qui ressemble à:

bool SpeedIsAcceptable(int minSpeed, int maxSpeed, int currentSpeed)
{
    return currentSpeed >= minSpeed & currentSpeed < maxSpeed;
}

Vous pouvez prendre les minSpeedet les maxSpeedparamètres et les mettre dans un nouveau type:

class SpeedRange
{
   public int Min;
   public int Max;
}

bool SpeedIsAcceptable(SpeedRange sr, int currentSpeed)
{
    return currentSpeed >= sr.Min & currentSpeed < sr.Max;
}

C'est mieux, mais pour vraiment profiter du nouveau type, déplacez les comparaisons dans le nouveau type:

class SpeedRange
{
   public int Min;
   public int Max;

   bool Contains(int speed)
   {
       return speed >= min & speed < Max;
   }
}

bool SpeedIsAcceptable(SpeedRange sr, int currentSpeed)
{
    return sr.Contains(currentSpeed);
}

Et maintenant, nous arrivons quelque part: l'implémentation de SpeedIsAcceptable()now dit ce que vous voulez dire, et vous avez une classe utile et réutilisable. (La prochaine étape évidente est de faire SpeedRangepour Range<Speed>.)

Comme vous pouvez le voir, Introduce Parameter Object était un bon début, mais sa vraie valeur était qu'il nous a aidés à découvrir un type utile qui manquait à notre modèle.

S'il s'agit d'un constructeur, en particulier s'il existe plusieurs variantes surchargées, vous devriez regarder le modèle Builder:

Foo foo = new Foo()
          .configBar(anything)
          .configBaz(something, somethingElse)
          // and so on

S'il s'agit d'une méthode normale, vous devriez réfléchir aux relations entre les valeurs transmises et peut-être créer un objet de transfert.

La réponse classique à cela est d'utiliser une classe pour encapsuler certains ou tous les paramètres. En théorie, cela sonne bien, mais je suis le genre de gars qui crée des cours pour des concepts qui ont un sens dans le domaine, donc ce n'est pas toujours facile d'appliquer ces conseils.

Par exemple au lieu de:

driver.connect(host, user, pass)

Vous pourriez utiliser

config = new Configuration()
config.setHost(host)
config.setUser(user)
config.setPass(pass)
driver.connect(config)

YMMV

Ceci est cité du livre de Fowler et Beck: "Refactoring"

Longue liste de paramètres

Dans nos premiers jours de programmation, on nous a appris à transmettre comme paramètres tout ce dont une routine a besoin. Cela était compréhensible car l'alternative était les données mondiales, et les données mondiales sont mauvaises et généralement douloureuses. Les objets changent cette situation car si vous n'avez pas quelque chose dont vous avez besoin, vous pouvez toujours demander à un autre objet de l'obtenir pour vous. Ainsi, avec les objets, vous ne transmettez pas tout ce dont la méthode a besoin; au lieu de cela, vous passez suffisamment pour que la méthode puisse accéder à tout ce dont elle a besoin. Une grande partie de ce dont une méthode a besoin est disponible sur la classe hôte de la méthode. Dans les programmes orientés objet, les listes de paramètres ont tendance à être beaucoup plus petites que dans les programmes traditionnels. C'est bien car les longues listes de paramètres sont difficiles à comprendre, car elles deviennent incohérentes et difficiles à utiliser, et parce que vous les changez pour toujours car vous avez besoin de plus de données. La plupart des modifications sont supprimées en passant des objets, car il est beaucoup plus probable que vous n'ayez besoin que de quelques requêtes pour accéder à un nouvel élément de données. Utilisez Remplacer le paramètre par la méthode lorsque vous pouvez obtenir les données dans un paramètre en faisant une demande d'un objet que vous connaissez déjà. Cet objet peut être un champ ou un autre paramètre. Utilisez Preserve Whole Object pour prendre un ensemble de données glanées à partir d'un objet et le remplacer par l'objet lui-même. Si vous avez plusieurs éléments de données sans objet logique, utilisez Introduce Parameter Object. Il y a une exception importante à faire ces changements. C'est à ce moment que vous ne souhaitez explicitement pas créer de dépendance entre l'objet appelé et l'objet plus grand. Dans ces cas, il est raisonnable de décompresser les données et de les envoyer en tant que paramètres, mais faites attention à la douleur impliquée. Si la liste de paramètres est trop longue ou change trop souvent, vous devez repenser votre structure de dépendances.

Je ne veux pas ressembler à une fissure sage, mais vous devriez également vérifier pour vous assurer que les données que vous transmettez doivent vraiment être transmises: passer des choses à un constructeur (ou à une méthode d'ailleurs) sent un peu comme peu d'emphase sur le comportement d'un objet.

Ne vous méprenez pas: les méthodes et les constructeurs auront parfois beaucoup de paramètres. Mais une fois rencontré, essayez plutôt d'envisager d'encapsuler les données avec un comportement .

Ce genre d'odeur (puisque nous parlons de refactoring, ce mot horrible semble approprié ...) pourrait également être détecté pour les objets qui ont beaucoup (lire: toutes) propriétés ou getters / setters.

Quand je vois de longues listes de paramètres, ma première question est de savoir si cette fonction ou cet objet en fait trop. Considérer:

EverythingInTheWorld earth=new EverythingInTheWorld(firstCustomerId,
  lastCustomerId,
  orderNumber, productCode, lastFileUpdateDate,
  employeeOfTheMonthWinnerForLastMarch,
  yearMyHometownWasIncorporated, greatGrandmothersBloodType,
  planetName, planetSize, percentWater, ... etc ...);

Bien sûr, cet exemple est délibérément ridicule, mais j'ai vu beaucoup de programmes réels avec des exemples à peine moins ridicules, où une classe est utilisée pour contenir de nombreuses choses à peine liées ou non liées, apparemment simplement parce que le même programme appelant a besoin des deux ou parce que le le programmeur a pensé aux deux en même temps. Parfois, la solution la plus simple consiste simplement à diviser la classe en plusieurs morceaux dont chacun fait son propre travail.

Un peu plus compliqué, c'est quand une classe doit vraiment gérer plusieurs choses logiques, comme à la fois une commande client et des informations générales sur le client. Dans ces cas, créez une classe pour le client et une classe pour la commande, et laissez-les se parler si nécessaire. Donc au lieu de:

 Order order=new Order(customerName, customerAddress, customerCity,
   customerState, customerZip,
   orderNumber, orderType, orderDate, deliveryDate);

Nous pourrions avoir:

Customer customer=new Customer(customerName, customerAddress,
  customerCity, customerState, customerZip);
Order order=new Order(customer, orderNumber, orderType, orderDate, deliveryDate);

Bien que je préfère bien sûr les fonctions qui ne prennent que 1 ou 2 ou 3 paramètres, il faut parfois accepter que, de manière réaliste, cette fonction en prenne beaucoup et que le nombre en soi ne crée pas vraiment de complexité. Par exemple:

Employee employee=new Employee(employeeId, firstName, lastName,
  socialSecurityNumber,
  address, city, state, zip);

Oui, c'est un tas de champs, mais probablement tout ce que nous allons faire avec eux est de les enregistrer dans un enregistrement de base de données ou de les jeter sur un écran ou quelque chose du genre. Il n'y a pas vraiment beaucoup de traitement ici.

Lorsque mes listes de paramètres deviennent longues, je préfère de loin que je puisse donner aux champs différents types de données. Comme quand je vois une fonction comme:

void updateCustomer(String type, String status,
  int lastOrderNumber, int pastDue, int deliveryCode, int birthYear,
  int addressCode,
  boolean newCustomer, boolean taxExempt, boolean creditWatch,
  boolean foo, boolean bar);

Et puis je le vois appelé avec:

updateCustomer("A", "M", 42, 3, 1492, 1969, -7, true, false, false, true, false);

Je m'inquiète. En regardant l'appel, on ne sait pas du tout ce que signifient tous ces nombres, codes et drapeaux cryptiques. C'est juste demander des erreurs. Un programmeur pourrait facilement se perdre dans l'ordre des paramètres et en changer accidentellement deux, et s'ils sont du même type de données, le compilateur l'accepterait simplement. Je préfère de loin avoir une signature où toutes ces choses sont des énumérations, donc un appel passe par des choses comme Type.ACTIVE au lieu de "A" et CreditWatch.NO au lieu de "false", etc.

Si certains des paramètres du constructeur sont facultatifs, il est logique d'utiliser un constructeur, qui obtiendrait les paramètres requis dans le constructeur, et des méthodes pour les optionnels, renvoyant le constructeur, à utiliser comme ceci:

return new Shniz.Builder(foo, bar).baz(baz).quux(quux).build();

Les détails de ceci sont décrits dans Effective Java, 2e édition, p. 11. Pour les arguments de méthode, le même livre (p. 189) décrit trois approches pour raccourcir les listes de paramètres:

• Divisez la méthode en plusieurs méthodes qui prennent moins d'arguments
• Créez des classes de membres auxiliaires statiques pour représenter des groupes de paramètres, c'est-à-dire passez un DinoDonkeyau lieu de dinoetdonkey
• Si les paramètres sont facultatifs, le générateur ci-dessus peut être adopté pour les méthodes, définissant un objet pour tous les paramètres, définissant les paramètres requis, puis appelant une méthode d'exécution dessus

J'utiliserais le constructeur par défaut et les settors de propriété. C # 3.0 a une bonne syntaxe pour faire cela automatiquement.

return new Shniz { Foo = foo,
                   Bar = bar,
                   Baz = baz,
                   Quuz = quux,
                   Fred = fred,
                   Wilma = wilma,
                   Barney = barney,
                   Dino = dino,
                   Donkey = donkey
                 };

L'amélioration du code consiste à simplifier le constructeur et à ne pas avoir à prendre en charge plusieurs méthodes pour prendre en charge diverses combinaisons. La syntaxe "appelante" est encore un peu "verbeuse", mais pas vraiment pire que d'appeler manuellement les settors de propriété.

Vous n'avez pas fourni suffisamment d'informations pour justifier une bonne réponse. Une longue liste de paramètres n'est pas intrinsèquement mauvaise.

Shniz (foo, bar, baz, quux, fred, wilma, barney, dino, âne)

pourrait être interprété comme:

void Shniz(int foo, int bar, int baz, int quux, int fred, 
           int wilma, int barney, int dino, int donkey) { ...

Dans ce cas, il est préférable de créer une classe pour encapsuler les paramètres, car vous donnez un sens aux différents paramètres d'une manière que le compilateur peut vérifier et en facilitant la lecture visuelle du code. Cela facilite également la lecture et la refactorisation plus tard.

// old way
Shniz(1,2,3,2,3,2,1,2);
Shniz(1,2,2,3,3,2,1,2); 

//versus
ShnizParam p = new ShnizParam { Foo = 1, Bar = 2, Baz = 3 };
Shniz(p);

Alternativement si vous aviez:

void Shniz(Foo foo, Bar bar, Baz baz, Quux quux, Fred fred, 
           Wilma wilma, Barney barney, Dino dino, Donkey donkey) { ...

C'est un cas très différent car tous les objets sont différents (et ne risquent pas d'être confus). Il est convenu que si tous les objets sont nécessaires et qu'ils sont tous différents, il est peu judicieux de créer une classe de paramètres.

De plus, certains paramètres sont-ils facultatifs? Existe-t-il des substitutions de méthode (même nom de méthode, mais signatures de méthode différentes?) Ces sortes de détails comptent tous pour savoir quelle est la meilleure réponse.

* Un sac de propriétés peut également être utile, mais pas spécifiquement mieux étant donné qu'il n'y a pas de contexte donné.

Comme vous pouvez le voir, il y a plus d'une réponse correcte à cette question. Faites votre choix.

Vous pouvez essayer de regrouper votre paramètre en plusieurs structures / classes significatives (si possible).

Je pencherais généralement vers l'approche structs - vraisemblablement la majorité de ces paramètres sont liés d'une manière ou d'une autre et représentent l'état d'un élément pertinent pour votre méthode.

Si l'ensemble de paramètres ne peut pas être transformé en un objet significatif, c'est probablement un signe qui en Shnizfait trop, et la refactorisation devrait impliquer la décomposition de la méthode en préoccupations distinctes.

Vous pouvez échanger la complexité contre des lignes de code source. Si la méthode elle-même en fait trop (couteau suisse), essayez de diviser par deux ses tâches en créant une autre méthode. Si la méthode est simple seulement, elle a besoin de trop de paramètres, alors les objets dits paramètres sont la solution.

Si votre langage le prend en charge, utilisez des paramètres nommés et rendez autant optionnels (avec des valeurs par défaut raisonnables) que possible.

Je pense que la méthode que vous avez décrite est la voie à suivre. Lorsque je trouve une méthode avec beaucoup de paramètres et / ou une méthode qui en aura probablement besoin à l'avenir, je crée généralement un objet ShnizParams à traverser, comme vous le décrivez.

Que diriez-vous de ne pas le définir en une seule fois chez les constructeurs mais de le faire via properties / setters ? J'ai vu des classes .NET qui utilisent cette approche, comme la Processclasse:

        Process p = new Process();

        p.StartInfo.UseShellExecute = false;
        p.StartInfo.CreateNoWindow = true;
        p.StartInfo.RedirectStandardOutput = true;
        p.StartInfo.RedirectStandardError = true;
        p.StartInfo.FileName = "cmd";
        p.StartInfo.Arguments = "/c dir";
        p.Start();

Je suis d'accord avec l'approche consistant à déplacer les paramètres dans un objet paramètre (struct). Plutôt que de les coller tous dans un seul objet, vérifiez si d'autres fonctions utilisent des groupes de paramètres similaires. Un objet de paramètre a plus de valeur s'il est utilisé avec plusieurs fonctions où vous vous attendez à ce que cet ensemble de paramètres change de manière cohérente entre ces fonctions. Il se peut que vous ne mettiez que certains des paramètres dans le nouvel objet de paramètre.

Si vous avez autant de paramètres, il y a de fortes chances que la méthode en fasse trop, alors abordez-la d'abord en divisant la méthode en plusieurs méthodes plus petites. Si vous avez encore trop de paramètres après cela, essayez de regrouper les arguments ou de transformer certains paramètres en membres d'instance.

Préférez les petites classes / méthodes aux grandes. Souvenez-vous du principe de responsabilité unique.

Les arguments nommés sont une bonne option (en supposant un langage qui les prend en charge) pour lever l'ambiguïté des listes de paramètres longues (ou même courtes!) Tout en permettant (dans le cas des constructeurs) les propriétés de la classe d'être immuables sans imposer une exigence pour lui permettre d'exister dans un état partiellement construit.

L'autre option que je rechercherais en faisant ce genre de refactor serait des groupes de paramètres liés qui pourraient être mieux traités comme un objet indépendant. En utilisant la classe Rectangle d'une réponse précédente comme exemple, le constructeur qui prend les paramètres pour x, y, hauteur et largeur pourrait factoriser x et y dans un objet Point, vous permettant de passer trois paramètres au constructeur du Rectangle. Ou allez un peu plus loin et faites-en deux paramètres (UpperLeftPoint, LowerRightPoint), mais ce serait un refactoring plus radical.

Cela dépend du type d'arguments que vous avez, mais s'il y a beaucoup de valeurs / options booléennes, vous pourriez peut-être utiliser un Flag Enum?

Je pense que ce problème est profondément lié au domaine du problème que vous essayez de résoudre avec la classe.

Dans certains cas, un constructeur à 7 paramètres peut indiquer une mauvaise hiérarchie de classes: dans ce cas, la structure / classe d'assistance suggérée ci-dessus est généralement une bonne approche, mais vous avez également tendance à vous retrouver avec des tas de structures qui ne sont que des sacs de propriétés et ne faites rien d'utile. Le constructeur à 8 arguments peut également indiquer que votre classe est trop générique / trop polyvalente, elle a donc besoin de beaucoup d'options pour être vraiment utile. Dans ce cas, vous pouvez soit refactoriser la classe, soit implémenter des constructeurs statiques qui masquent les vrais constructeurs complexes: par exemple. Shniz.NewBaz (foo, bar) pourrait en fait appeler le constructeur réel en passant les bons paramètres.

Une considération est laquelle des valeurs serait en lecture seule une fois que l'objet est créé?

Les propriétés publiquement accessibles en écriture pourraient peut-être être attribuées après la construction.

D'où viennent finalement les valeurs? Peut-être que certaines valeurs sont vraiment externes alors que d'autres proviennent vraiment d'une configuration ou de données globales gérées par la bibliothèque.

Dans ce cas, vous pouvez cacher le constructeur à une utilisation externe et lui fournir une fonction Create. La fonction create prend les valeurs véritablement externes et construit l'objet, puis utilise des accesseurs uniquement disponibles pour la bibliothèque pour terminer la création de l'objet.

Il serait vraiment étrange d'avoir un objet qui nécessite 7 paramètres ou plus pour donner à l'objet un état complet et tous étant vraiment de nature externe.

Quand une classe a un constructeur qui prend trop d'arguments, c'est généralement le signe qu'il a trop de responsabilités. Il peut probablement être divisé en classes distinctes qui coopèrent pour donner les mêmes fonctionnalités.

Au cas où vous auriez vraiment besoin de autant d'arguments pour un constructeur, le modèle Builder peut vous aider. Le but est de toujours transmettre tous les arguments au constructeur, afin que son état soit initialisé depuis le début et que vous puissiez toujours rendre la classe immuable si nécessaire.

Voir ci-dessous :

public class Toto {
    private final String state0;
    private final String state1;
    private final String state2;
    private final String state3;

    public Toto(String arg0, String arg1, String arg2, String arg3) {
        this.state0 = arg0;
        this.state1 = arg1;
        this.state2 = arg2;
        this.state3 = arg3;
    }

    public static class TotoBuilder {
        private String arg0;
        private String arg1;
        private String arg2;
        private String arg3;

        public TotoBuilder addArg0(String arg) {
            this.arg0 = arg;
            return this;
        }
        public TotoBuilder addArg1(String arg) {
            this.arg1 = arg;
            return this;
        }
        public TotoBuilder addArg2(String arg) {
            this.arg2 = arg;
            return this;
        }
        public TotoBuilder addArg3(String arg) {
            this.arg3 = arg;
            return this;
        }

        public Toto newInstance() {
            // maybe add some validation ...
            return new Toto(this.arg0, this.arg1, this.arg2, this.arg3);
        }
    }

    public static void main(String[] args) {
        Toto toto = new TotoBuilder()
            .addArg0("0")
            .addArg1("1")
            .addArg2("2")
            .addArg3("3")
            .newInstance();
    }

}