Passer un objet dans une méthode qui change l'objet, est-ce un (anti) motif commun?

17

Je lis des odeurs de code communes dans le livre Refactoring de Martin Fowler . Dans ce contexte, je me posais des questions sur un modèle que je vois dans une base de code, et si l'on pouvait objectivement le considérer comme un anti-modèle.

Le modèle est celui où un objet est passé en argument à une ou plusieurs méthodes, qui modifient toutes l'état de l'objet, mais aucune ne renvoie l'objet. Il s'appuie donc sur la nature pass by reference de (dans ce cas) C # /. NET.

var something = new Thing();
// ...
Foo(something);
int result = Bar(something, 42);
Baz(something);

Je trouve que (en particulier lorsque les méthodes ne sont pas nommées de manière appropriée), je dois examiner ces méthodes pour comprendre si l'état de l'objet a changé. Cela rend la compréhension du code plus complexe, car j'ai besoin de suivre plusieurs niveaux de la pile d'appels.

Je voudrais proposer d'améliorer ce code pour retourner un autre objet (cloné) avec le nouvel état, ou tout ce qui est nécessaire pour changer l'objet sur le site d'appel.

var something1 =  new Thing();
// ...

// Let's return a new instance of Thing
var something2 = Foo(something1);

// Let's use out param to 'return' other info about the operation
int result;
var something3 = Bar(something2, out result);

// If necessary, let's capture and make explicit complex changes
var changes = Baz(something3)
something3.Apply(changes);

Il me semble que le premier modèle est choisi sur les hypothèses

  • que c'est moins de travail, ou nécessite moins de lignes de code
  • qu'il nous permet à la fois de changer l'objet et de renvoyer une autre information
  • qu'il est plus efficace car nous avons moins d'instances de Thing.

J'illustre une alternative, mais pour la proposer, il faut avoir des arguments contre la solution originale. Quels arguments, le cas échéant, peuvent être avancés pour démontrer que la solution originale est un anti-modèle?

Et qu'est-ce qui ne va pas, le cas échéant, avec ma solution alternative?

Michiel van Oosterhout
la source
1
Ceci est un effet secondaire
Dave Hillier
1
@DaveHillier Merci, je connaissais bien le terme, mais je n'avais pas fait le lien.
Michiel van Oosterhout

Réponses:

9

Oui, la solution d'origine est un anti-modèle pour les raisons que vous décrivez: il est difficile de raisonner sur ce qui se passe, l'objet n'est pas responsable de son propre état / implémentation (rupture de l'encapsulation). J'ajouterais également que tous ces changements d'état sont des contrats implicites de la méthode, ce qui rend cette méthode fragile face à l'évolution des exigences.

Cela dit, votre solution a certains de ses propres inconvénients, dont le plus évident est que le clonage d'objets n'est pas génial. Cela peut être lent pour les gros objets. Cela peut entraîner des erreurs lorsque d'autres parties du code s'accrochent aux anciennes références (ce qui est probablement le cas dans la base de code que vous décrivez). Rendre ces objets explicitement immuables résout au moins quelques-uns de ces problèmes, mais c'est un changement plus radical.

À moins que les objets ne soient petits et quelque peu transitoires (ce qui en fait de bons candidats pour l'immuabilité), je serais enclin à simplement déplacer davantage la transition d'état dans les objets eux-mêmes. Cela vous permet de masquer les détails d'implémentation de ces transitions et de définir des exigences plus strictes concernant qui / quoi / où ces transitions d'état se produisent.

Telastyn
la source
1
Par exemple, si j'ai un objet "Fichier", je n'essaierais pas de déplacer une méthode de changement d'état dans cet objet - cela violerait le SRP. Cela reste valable même lorsque vous avez vos propres classes au lieu d'une classe de bibliothèque comme "File" - mettre chaque logique de transition d'état dans la classe de l'objet n'a pas vraiment de sens.
Doc Brown
@Tetastyn Je sais que c'est une vieille réponse, mais j'ai du mal à visualiser concrètement votre suggestion dans le dernier paragraphe. Pourriez-vous élaborer ou donner un exemple?
AaronLS
@AaronLS - Au lieu de Bar(something)(et de modifier l'état de something), créez Barun membre de somethingtype. something.Bar(42)est plus susceptible de muter something, tout en vous permettant également d'utiliser des outils OO (état privé, interfaces, etc.) pour protéger somethingl'état de
Telastyn
14

lorsque les méthodes ne sont pas nommées de manière appropriée

En fait, c'est la vraie odeur de code. Si vous avez un objet mutable, il fournit des méthodes pour changer son état. Si vous avez un appel à une telle méthode intégrée dans une tâche de quelques autres instructions, il est bon de refactoriser cette tâche à une méthode qui lui est propre - ce qui vous laisse exactement dans la situation décrite. Mais si vous n'avez pas de noms de méthode comme Fooet Bar, mais des méthodes qui indiquent clairement qu'ils changent l'objet, je ne vois pas de problème ici. Penser à

void AddMessageToLog(Logger logger, string msg)
{
    //...
}

ou

void StripInvalidCharsFromName(Person p)
{
// ...
}

ou

void AddValueToRepo(Repository repo,int val)
{
// ...
}

ou

void TransferMoneyBetweenAccounts(Account source, Account destination, decimal amount)
{
// ...
}

ou quelque chose comme ça - je ne vois aucune raison ici de retourner un objet cloné pour ces méthodes, et il n'y a pas non plus de raison d'examiner leur implémentation pour comprendre qu'elles changeront l'état de l'objet passé.

Si vous ne voulez pas d'effets secondaires, rendez vos objets immuables, il appliquera des méthodes comme celles ci-dessus pour renvoyer un objet modifié (cloné) sans changer celui d'origine.

Doc Brown
la source
Vous avez raison, la refactorisation de la méthode de renommage peut améliorer la situation en clarifiant les effets secondaires. Cela peut devenir difficile si les modifications sont telles qu'un nom de méthode concis n'est pas possible.
Michiel van Oosterhout
2
@michielvoo: si la dénomination de méthode consise ne semble pas possible, votre méthode regroupe les mauvaises choses au lieu de construire une abstraction fonctionnelle pour la tâche qu'elle effectue (et cela est vrai avec ou sans effets secondaires).
Doc Brown
4

Oui, voir http://codebetter.com/matthewpodwysocki/2008/04/30/side-effecting-functions-are-code-smells/ pour l'un des nombreux exemples de personnes soulignant que les effets secondaires inattendus sont mauvais.

En général, le principe fondamental est que le logiciel est construit en couches, et chaque couche doit présenter l'abstraction la plus propre possible à la suivante. Et une abstraction propre est celle où vous devez garder le moins possible à l'esprit pour l'utiliser. C'est ce qu'on appelle la modularité, et s'applique à tout, des fonctions simples aux protocoles en réseau.

btilly
la source
Je caractériserais ce que l'OP décrit comme des «effets secondaires attendus». Par exemple, un délégué que vous pouvez transmettre à un moteur quelconque qui opère sur chaque élément d'une liste. C'est essentiellement ce qui ForEach<T>fait.
Robert Harvey
@RobertHarvey Les plaintes concernant les méthodes qui ne sont pas nommées correctement et le fait d'avoir à lire du code pour comprendre les effets secondaires, les rendent définitivement inattendus.
btilly
Je vous l'accorde. Mais le corollaire est qu'une méthode documentée correctement nommée avec des effets de site attendus pourrait ne pas être un anti-modèle après tout.
Robert Harvey
@RobertHarvey Je suis d'accord. La clé est que les effets secondaires importants sont très importants à connaître et doivent être documentés attentivement (de préférence au nom de la méthode).
btilly
Je dirais que c'est un mélange d'effets secondaires inattendus et non évidents. Merci pour le lien.
Michiel van Oosterhout
3

Tout d'abord, cela ne dépend pas de la "nature passante de référence", cela dépend des objets qui sont des types de référence mutables. Dans les langages non fonctionnels, cela sera presque toujours le cas.

Deuxièmement, que ce soit un problème ou non, cela dépend à la fois de l'objet et de la façon dont les changements dans les différentes procédures sont liés - si vous ne faites pas de changement dans Foo et que Bar se bloque, alors c'est un problème. Pas nécessairement une odeur de code, mais c'est un problème avec Foo ou Bar ou Something (probablement Bar car il devrait vérifier son entrée, mais il pourrait s'agir de quelque chose mis dans un état invalide qu'il devrait empêcher).

Je ne dirais pas que cela s'élève au niveau d'un anti-pattern, mais plutôt quelque chose à savoir.

jmoreno
la source
2

Je dirais qu'il y a peu de différence entre A.Do(Something)modifier somethinget something.Do()modifier something. Dans les deux cas, il doit être clair d'après le nom de la méthode invoquée qui somethingsera modifiée. Si le nom de la méthode n'est pas clair, qu'il s'agisse d' somethingun paramètre thisou d'une partie de l'environnement, il ne doit pas être modifié.

svidgen
la source
1

Je pense que c'est bien de changer l'état de l'objet dans certains scénarios. Par exemple, j'ai une liste d'utilisateurs et je souhaite appliquer différents filtres à la liste avant de la renvoyer au client.

var users = Dependency.Resolve<IGetUsersQuery>().GetAll();

var excludeAdminUsersFilter = new ExcludeAdminUsersFilter();
var filterByAnotherCriteria = new AnotherCriteriaFilter();

excludeAdminUsersFilter.Apply(users);
filterByAnotherCriteria.Apply(users); 

Et oui, vous pouvez rendre cela joli en déplaçant le filtrage dans une autre méthode, vous vous retrouverez donc avec quelque chose comme:

var users = Dependency.Resolve<IGetUsersQuery>().GetAll();
Filter(users);

Filter(users)exécuterait les filtres ci-dessus.

Je ne me souviens pas exactement où je suis tombé sur ce sujet auparavant, mais je pense qu'il était appelé pipeline de filtrage.

CodeART
la source
0

Je ne sais pas si la nouvelle solution proposée (de copier des objets) est un modèle. Le problème, comme vous l'avez souligné, est la mauvaise nomenclature des fonctions.

Disons que j'écris une opération mathématique complexe en tant que fonction f () . Je documente que f () est une fonction qui correspond NXNà N, et l'algorithme derrière elle. Si la fonction est nommée de manière inappropriée et n'est pas documentée, et n'a pas de cas de test d'accompagnement, et vous devrez comprendre le code, auquel cas le code est inutile.

À propos de votre solution, quelques observations:

  • Les applications sont conçues sous différents aspects: lorsqu'un objet est utilisé uniquement pour contenir des valeurs, ou est passé à travers les limites des composants, il est sage de modifier les entrailles de l'objet en externe plutôt que de le remplir avec des détails sur la façon de changer.
  • Le clonage d'objets entraîne des besoins de mémoire gonflés et, dans de nombreux cas, conduit à l'existence d'objets équivalents dans des états incompatibles ( Xdevenus Yaprès f(), mais le Xsont en fait Y) et éventuellement une incohérence temporelle.

Le problème que vous essayez de résoudre est valide; cependant, même avec une énorme ingénierie excessive, le problème est contourné, pas résolu.

CMR
la source
2
Ce serait une meilleure réponse si vous reliez votre observation à la question du PO. En l'état, c'est plus un commentaire qu'une réponse.
Robert Harvey
1
@RobertHarvey +1, bonne observation, je suis d'accord, va le modifier.
CMR