Passage d'une variable membre en tant que paramètre de méthode

Dans un projet, j'ai trouvé un code comme celui-ci:

class SomeClass
{
    private SomeType _someField;

    public SomeType SomeField
    {
        get { return _someField; }
        set { _someField = value; }
    }

    protected virtual void SomeMethod(/*...., */SomeType someVar)
    {
    }

    private void SomeAnotherMethod()
    {
        //.............
        SomeMethod(_someField);
        //.............
    }

};

Comment convaincre mes coéquipiers que c'est du mauvais code?

Je crois que c'est une complication inutile. Pourquoi passer une variable membre en tant que paramètre de méthode si vous y avez déjà accès? C'est aussi une violation de l'encapsulation.

Voyez-vous d'autres problèmes avec ce code?

Je pense que c'est un sujet valable, mais la raison pour laquelle vous obtenez des réponses mitigées est due à la façon dont la question a été formée. Personnellement, j’ai eu les mêmes expériences avec mon équipe, où il était inutile de passer des arguments comme arguments et qui compliquait le code. Nous aurions une classe qui fonctionne avec un ensemble de membres mais certaines fonctions accèdent directement aux membres et d'autres fonctions modifieraient les mêmes membres par le biais de paramètres (c'est-à-dire utilisent des noms complètement différents) et il n'y avait aucune raison technique de le faire. Pour des raisons techniques, je veux dire un exemple fourni par Kate.

Je recommanderais de prendre du recul et, au lieu de se concentrer uniquement sur les paramètres de passage de membre, amorcez des discussions avec votre équipe sur la clarté et la lisibilité. De manière plus formelle ou uniquement dans les couloirs, discutez de ce qui rend certains segments de code plus faciles à lire et d'autres segments de code plus difficiles. Ensuite, identifiez les mesures de qualité ou les attributs du code épuré qu’en tant qu’équipe, vous voudrez travailler. Après tout, même lorsque nous travaillons sur des projets en champ vert, nous passons plus de 90% de notre temps à lire et dès que le code est écrit (disons 10 à 15 minutes plus tard), il passe en maintenance où la lisibilité est encore plus importante.

Donc, pour votre exemple spécifique ici, l'argument que j'utiliserais est que moins de code est toujours plus facile à lire que plus de code. Une fonction qui a 3 paramètres est plus difficile à traiter pour le cerveau qu'une fonction qui n'a aucun ou 1 paramètre. S'il existe un autre nom de variable, le cerveau doit garder une trace de la lecture du code. Donc, souvenez-vous de "int m_value" puis de "int localValue" et souvenez-vous que l'un signifie vraiment que l'autre est toujours plus cher pour votre cerveau que de travailler simplement avec "m_value".

Pour plus de munitions et d’idées, je vous conseillerais de vous procurer un exemplaire du Clean Code de Oncle Bob .

Je peux penser à une justification pour passer un champ de membre en tant que paramètre dans une méthode (privée): cela explique explicitement ce dont dépend votre méthode.

Comme vous le dites, tous les champs membres sont des paramètres implicites de votre méthode, car l'objet entier l'est. Cependant, l'objet complet est-il vraiment nécessaire pour calculer le résultat? S'il SomeMethods'agit d'une méthode interne qui ne dépend que de _someField, n'est-il pas plus propre de rendre cette dépendance explicite? En fait, rendre cette dépendance explicite peut également suggérer que vous pouvez réellement refactoriser ce morceau de code hors de votre classe! (Notez que je suppose que nous ne parlons pas de getters ou de setters ici, mais du code qui calcule réellement quelque chose)

Je ne ferais pas le même argument pour les méthodes publiques, car l'appelant ne sait ni ne se soucie de la partie de l'objet qui est pertinente pour calculer le résultat ...

Je vois une bonne raison de passer des variables de membre en tant qu’arguments de fonction à des méthodes privées : la pureté de la fonction. Les variables membres sont en réalité un état global du point de vue, voire un état global mutable si ledit membre change pendant l'exécution de la méthode. En remplaçant les références de variable membre par les paramètres de méthode, nous pouvons effectivement rendre une fonction pure. Les fonctions pures ne dépendent pas d'un état externe et n'ont pas d'effets secondaires, renvoyant toujours les mêmes résultats avec le même jeu de paramètres d'entrée, ce qui facilite les tests et la maintenance future.

Bien sûr, il n’est ni facile ni pratique d’avoir toutes vos méthodes en tant que méthodes pures dans un langage POO. Mais je pense que vous gagnez beaucoup en termes de clarté du code en ayant les méthodes qui gèrent la logique complexe pure et en utilisant des variables immuables, tout en maintenant la gestion des états "globale" impure séparée au sein de méthodes dédiées.

Passer des variables de membre à une fonction publique du même objet lors de l'appel externe de la fonction constituerait toutefois une odeur majeure de code à mon avis.

Si la fonction est appelée plusieurs fois, transmettant parfois cette variable membre et transmettant autre chose, alors tout va bien. Par exemple, je ne considérerais pas cela du tout mauvais:

if ( CalculateCharges(newStartDate) > CalculateCharges(m_StartDate) )
{
     //handle increase in charges
}

où newStartDateest une variable locale et m_StartDateest une variable membre.

Cependant, si la fonction n'est appelée qu'avec la variable membre qui lui est transmise, c'est étrange. Les fonctions membres travaillent sur les variables membres tout le temps. Ils peuvent le faire (selon la langue dans laquelle vous travaillez) pour obtenir une copie de la variable membre - si c'est le cas et que vous ne le voyez pas, le code pourrait être plus agréable s'il rendait tout le processus explicite.

Ce que personne n'a dit, c'est que SomeMethod est protégé virtuel. Cela signifie qu'une classe dérivée peut l'utiliser ET réimplémenter ses fonctionnalités. Une classe dérivée n'aurait pas accès à la variable privée et ne pourrait donc pas fournir d'implémentation personnalisée de SomeMethod qui dépend de la variable privée. Au lieu de prendre la dépendance sur la variable privée, la déclaration demande à l'appelant de la transmettre.

Les frameworks d'interface graphique ont généralement une sorte de classe 'View' qui représente les éléments dessinés à l'écran, et cette classe fournit généralement une méthode invalidateRect(Rect r)permettant de marquer une partie de sa zone de dessin comme devant être redessinée. Les clients peuvent appeler cette méthode pour demander une mise à jour d'une partie de la vue. Mais une vue peut aussi appeler sa propre méthode, comme:

invalidateRect(m_frame);

provoquer le redessin de toute la zone. Par exemple, il peut le faire lorsqu’il est ajouté pour la première fois à la hiérarchie des vues.

Cela ne présente aucun inconvénient: le cadre de la vue est un rectangle valide et la vue elle-même sait qu'elle veut se redessiner. La classe View pourrait fournir une méthode distincte qui ne prend aucun paramètre et utilise plutôt le cadre de la vue:

invalidateFrame();

Mais pourquoi ajouter une méthode spéciale juste pour cela quand vous pouvez utiliser la méthode plus générale invalidateRect()? Ou, si vous choisissez de fournir invalidateFrame(), vous l'implémenterez très probablement en termes plus généraux invalidateRect():

View::invalidateFrame(void)
{
    invalidateRect(m_frame)
}

Pourquoi passer variable comme paramètre de méthode, si vous y avez déjà accès?

Vous devez passer les variables d'instance en tant que paramètres à votre propre méthode si la méthode ne fonctionne pas spécifiquement sur cette variable d'instance. Dans l'exemple ci-dessus, le cadre de la vue n'est qu'un autre rectangle en ce qui concerne la invalidateRect()méthode.

Cela a du sens si la méthode est une méthode utilitaire. Disons par exemple que vous devez dériver un nom court unique à partir de plusieurs chaînes de texte libres.

Vous ne voudriez pas coder une implémentation distincte pour chaque chaîne, mais la transmettre à une méthode commune est logique.

Cependant, si la méthode fonctionne toujours sur un seul membre, il semble un peu idiot de la passer en paramètre.

La raison principale pour avoir une variable membre dans une classe est de vous permettre de la définir à un endroit et d’avoir sa valeur disponible pour toutes les autres méthodes de la classe. En général, vous vous attendez donc à ce qu'il ne soit pas nécessaire de passer la variable membre à une méthode de la classe.

Je peux toutefois penser à deux raisons pour lesquelles vous pourriez souhaiter passer la variable membre à une autre méthode de la classe. La première est si vous devez vous assurer que la valeur de la variable membre doit être utilisée telle quelle, lorsqu'elle est utilisée avec la méthode appelée, même si cette méthode doit modifier la valeur de la variable membre réelle à un moment donné du processus. La deuxième raison est liée à la première en ce sens que vous pouvez souhaiter garantir l’immuabilité d’une valeur dans le cadre d’une chaîne de méthodes - lors de l’implémentation d’une syntaxe fluide, par exemple.

Avec tout cela à l'esprit, je n'irais pas jusqu'à dire que le code est "mauvais" en tant que tel si vous passez une variable membre à l'une des méthodes de sa classe. Cependant, je suggérerais que ce n'est généralement pas idéal, car cela pourrait encourager beaucoup de duplication de code où le paramètre est assigné à une variable locale par exemple, et les paramètres supplémentaires ajoutent du "bruit" dans le code là où il n'est pas nécessaire. Si vous êtes un fan du livre Clean Code, vous savez qu'il indique que vous devez conserver le nombre minimal de paramètres de méthode, et uniquement s'il n'existe pas d'autre moyen plus raisonnable pour que la méthode accède au paramètre. .

Quelques choses qui me viennent quand on pense à ça:

1. En général, les signatures de méthode avec moins de paramètres sont plus faciles à comprendre. L'une des principales raisons pour lesquelles les méthodes ont été inventées est d'éliminer les longues listes de paramètres en les associant aux données sur lesquelles elles fonctionnent.
2. Si la signature de la méthode dépend de la variable membre, il sera plus difficile de modifier ces variables à l'avenir, car vous devez non seulement changer à l'intérieur de la méthode, mais partout où la méthode est également appelée. Et comme SomeMethod dans votre exemple est protégé, les sous-classes devront également être modifiées.
3. Les méthodes (publiques ou privées) qui ne dépendent pas de la classe interne n'ont pas besoin d'être sur cette classe. Ils pourraient être intégrés aux méthodes d’utilité et être tout aussi heureux. Ils n'ont presque aucune entreprise faisant partie de cette classe. Si aucune autre méthode ne dépend de cette variable après avoir déplacé la ou les méthodes, alors cette variable devrait également l'être! Très probablement, la variable devrait être sur son propre objet avec la ou les méthodes qui opèrent, devenant publique et composée par la classe parente.
4. La transmission de données à diverses fonctions telles que votre classe est un programme procédural avec des variables globales qui va à l’encontre de la conception d’OO. Ce n'est pas l'intention des variables membres et des fonctions membres (voir ci-dessus), et il semble que votre classe ne soit pas très cohérente. Je pense que c'est une odeur de code qui suggère une meilleure façon de regrouper vos données et méthodes.

Vous êtes manquant si le paramètre ("argument") est référence ou en lecture seule.

class SomeClass
{
    protected SomeType _someField;
    public SomeType SomeField
    {
        get { return _someField; }

        set {
          if (doSomeValidation(value))
          {
            _someField = value;
          }
        }
    }

    protected virtual void ModifyMethod(/*...., */ ref SomeType someVar)
    { 
      // ...
    }    

    protected virtual void ReadMethod(/*...., */ SomeType someVar)
    { 
      // ...
    }

    private void SomeAnotherMethod()
    {
        //.............

        // not recommended, but, may be required in some cases
        ModifyMethod(ref this._someField);

        //.............

        // recommended, but, verbose
        SomeType SomeVar = this.someField;
        ModifyMethod(ref SomeVar);
        this.someField = SomeVar;

        //.............

        ReadMethod(this.someField);
        //.............
    }

};

De nombreux développeurs assignent généralement directement les champs internes d'une variable dans les méthodes du constructeur. Il y a quelques exceptions.

Rappelez-vous que les "setters" peuvent avoir des méthodes supplémentaires, pas seulement assignament, et parfois même être des méthodes virtuelles, ou appeler des méthodes virtuelles.

Remarque: Je suggère de conserver les champs internes des propriétés en tant que "protégées".