Ne jamais rendre les membres publics virtuels / abstraits - vraiment?

20

Dans les années 2000, un de mes collègues m'a dit que c'était un anti-modèle de rendre les méthodes publiques virtuelles ou abstraites.

Par exemple, il a considéré une classe comme celle-ci pas bien conçue:

public abstract class PublicAbstractOrVirtual
{
  public abstract void Method1(string argument);

  public virtual void Method2(string argument)
  {
    if (argument == null) throw new ArgumentNullException(nameof(argument));
    // default implementation
  }
}

Il a déclaré que

  • le développeur d'une classe dérivée qui implémente Method1et substitue Method2doit répéter la validation de l'argument.
  • dans le cas où le développeur de la classe de base décide d'ajouter quelque chose autour de la partie personnalisable de Method1ou Method2plus tard, il ne peut pas le faire.

Au lieu de cela, mon collègue a proposé cette approche:

public abstract class ProtectedAbstractOrVirtual
{
  public void Method1(string argument)
  {
    if (argument == null) throw new ArgumentNullException(nameof(argument));
    this.Method1Core(argument);
  }

  public void Method2(string argument)
  {
    if (argument == null) throw new ArgumentNullException(nameof(argument));
    this.Method2Core(argument);
  }

  protected abstract void Method1Core(string argument);

  protected virtual void Method2Core(string argument)
  {
    // default implementation
  }
}

Il m'a dit que rendre les méthodes publiques (ou propriétés) virtuelles ou abstraites est aussi mauvais que rendre les champs publics. En encapsulant des champs dans des propriétés, vous pouvez intercepter ultérieurement tout accès à ces champs, si nécessaire. La même chose s'applique aux membres publics virtuels / abstraits: les envelopper comme indiqué dans la ProtectedAbstractOrVirtualclasse permet au développeur de la classe de base d'intercepter tous les appels qui vont aux méthodes virtuelles / abstraites.

Mais je ne vois pas cela comme une directive de conception. Même Microsoft ne le suit pas: jetez un œil à la Streamclasse pour le vérifier.

Que pensez-vous de cette ligne directrice? Cela a-t-il un sens ou pensez-vous que cela complique trop l'API?

Peter Perot
la source
5
La création de méthodes virtual permet la substitution facultative. Votre méthode devrait probablement être publique, car elle pourrait ne pas être remplacée. Créer des méthodes abstract vous oblige à les ignorer; ils devraient probablement l'être protected, car ils ne sont pas particulièrement utiles dans un publiccontexte.
Robert Harvey
4
En fait, protectedest plus utile lorsque vous souhaitez exposer des membres privés de la classe abstraite à des classes dérivées. En tout cas, je ne suis pas particulièrement préoccupé par l'opinion de votre ami; choisissez le modificateur d'accès qui convient le mieux à votre situation particulière.
Robert Harvey
4
Votre collègue plaidait pour le modèle de méthode de modèle . Il peut y avoir des cas d'utilisation dans les deux sens, selon l'interdépendance des deux méthodes.
Greg Burghardt
8
@GregBurghardt: on dirait que le collègue du PO suggère de toujours utiliser le modèle de méthode de modèle, qu'il soit requis ou non. C'est une surutilisation typique des modèles - si l'on a un marteau, tôt ou tard, chaque problème commence à ressembler à un clou ;-)
Doc Brown
3
@PeterPerot: Je n'ai jamais eu de problème pour commencer avec de simples DTO avec juste des champs publics, et quand un tel DTO s'est avéré nécessiter des membres avec une logique métier, les refactoriser en classes avec des propriétés. Les choses sont sûrement différentes lorsque l'on travaille en tant que vendeur de bibliothèque et que l'on ne doit pas changer les API publiques, alors même transformer un champ public en une propriété publique du même nom peut causer des problèmes.
Doc Brown

Réponses:

30

En disant

qu'il s'agit d'un anti-modèle pour rendre les méthodes publiques virtuelles ou abstraites à cause du développeur d'une classe dérivée qui implémente la méthode 1 et remplace la méthode 2 doit répéter la validation de l'argument

mélange la cause et l'effet. Il suppose que chaque méthode remplaçable nécessite une validation d'argument non personnalisable. Mais c'est tout le contraire:

Si l' on veut concevoir une méthode d'une manière qui fournit des validations d'arguments fixes dans toutes les dérivations de la classe (ou - plus généralement - une partie personnalisable et non personnalisable), alors il est logique de rendre le point d'entrée non virtuel et fournissez à la place une méthode virtuelle ou abstraite pour la partie personnalisable qui est appelée en interne.

Mais il y a beaucoup d'exemples où il est parfaitement logique d'avoir une méthode virtuelle publique, car il n'y a pas de partie fixe non personnalisable: regardez les méthodes standard comme ToStringou Equalsou GetHashCode- cela améliorerait-il la conception de la objectclasse pour qu'elles ne soient pas publiques et virtuel en même temps? Je ne pense pas.

Ou, en termes de votre propre code: lorsque le code de la classe de base ressemble finalement et intentionnellement à ceci

 public void Method1(string argument)
 {
    // nothing to validate here, all strings including null allowed
    this.Method1Core(argument);
 }

avoir cette séparation entre Method1et Method1Corene fait que compliquer les choses sans raison apparente.

Doc Brown
la source
1
Dans le cas de la ToString()méthode, Microsoft ferait mieux de la rendre non virtuelle et a introduit une méthode de modèle virtuel ToStringCore(). Pourquoi: à cause de cela: ToString()- note aux héritiers . Ils déclarent que cela ToString()ne devrait pas retourner nul. Ils auraient pu contraindre cette demande en la mettant en œuvre ToString() => ToStringCore() ?? string.Empty.
Peter Perot
9
@PeterPerot: cette ligne directrice à laquelle vous avez lié recommande également de ne pas revenir string.Empty, avez-vous remarqué? Et il recommande beaucoup d'autres choses qui ne peuvent pas être appliquées dans le code en introduisant quelque chose comme une ToStringCoreméthode. Cette technique n'est donc probablement pas le bon outil ToString.
Doc Brown
3
@Theraot: on peut sûrement trouver des raisons ou des arguments pour lesquels ToString and Equals ou GetHashcode auraient pu être conçus différemment, mais aujourd'hui, ils sont tels qu'ils sont (au moins je pense que leur conception est assez bonne pour en faire un bon exemple).
Doc Brown
1
@PeterPerot "J'ai vu beaucoup de code comme anyObjectIGot.ToString()au lieu de anyObjectIGot?.ToString()" - en quoi est-ce pertinent? Votre ToStringCore()approche empêcherait le retour d'une chaîne nulle, mais elle lancerait quand même un NullReferenceExceptionsi l'objet est nul.
IMil
1
@PeterPerot Je n'essayais pas de transmettre un argument de l'autorité. Ce n'est pas tellement que Microsoft utilise public virtual, mais qu'il y a des cas dans lesquels ça public virtualva. Nous pourrions plaider pour une partie vide non personnalisable comme rendant le code à l'épreuve du temps ... Mais cela ne fonctionne pas. Revenir en arrière et le modifier pourrait casser les types dérivés. Ainsi, cela ne rapporte rien.
Theraot
6

Le faire comme le suggère votre collègue offre plus de flexibilité à l'implémenteur de la classe de base. Mais cela s'accompagne également d'une complexité accrue qui n'est généralement pas justifiée par les avantages présumés.

Gardez à l'esprit que la flexibilité accrue pour l'implémenteur de la classe de base se fait au détriment d' une flexibilité moindre pour la partie dominante. Ils obtiennent un comportement imposé dont ils ne se soucient peut-être pas particulièrement. Pour eux, les choses sont devenues plus rigides. Cela peut être justifié et utile, mais tout dépend du scénario.

La convention de dénomination pour implémenter cela (que je sache) est de réserver le bon nom pour l'interface publique et de préfixer le nom de la méthode interne avec "Do".

Un cas utile est lorsque l'action effectuée nécessite une configuration et une fermeture. Comme ouvrir un flux et le fermer une fois le remplacement terminé. En général, même type d'initialisation et de finalisation. C'est un modèle valide à utiliser, mais il serait inutile de rendre obligatoire son utilisation dans tous les scénarios abstraits et virtuels.

Martin Maat
la source
1
Le préfixe de la méthode Do est une option. Microsoft utilise souvent le suffixe de la méthode Core .
Peter Perot
@Peter Perot. Je n'ai jamais vu le préfixe Core dans aucun document Microsoft, mais cela peut être dû au fait que je n'y ai pas prêté beaucoup d'attention ces derniers temps. Je soupçonne qu'ils ont commencé à le faire récemment juste pour promouvoir le surnom de Core, pour faire un nom pour .NET Core.
Martin Maat
Non, c'est un vieux chapeau: BindingList. De plus, j'ai trouvé la recommandation quelque part, peut-être leurs directives de conception de cadre ou quelque chose de similaire. Et: c'est un suffixe . ;-)
Peter Perot
Moins de flexibilité pour la classe dérivée est le point. La classe de base est une frontière d'abstraction. La classe de base indique au consommateur ce que fait son API publique et elle définit l'API requise pour atteindre ces objectifs. Si une classe dérivée peut remplacer une méthode publique dans la classe de base, vous courez un risque accru de violer le principe de substitution Liskov.
Adrian McCarthy
2

En C ++, cela s'appelle le modèle d'interface non virtuelle (NVI). (Il était une fois la méthode Template. C'était déroutant, mais certains des anciens articles ont cette terminologie.) NVI est promu par Herb Sutter, qui a écrit à ce sujet au moins quelques fois. Je pense que l'un des premiers est ici .

Si je me souviens bien, la prémisse est qu'une classe dérivée ne devrait pas changer ce que fait la classe de base mais comment elle le fait.

Par exemple, une forme peut avoir une méthode Move pour déplacer la forme. Une implémentation concrète (par exemple, comme Square et Circles) ne doit pas directement remplacer Move, car Shape définit ce que Moving signifie (au niveau conceptuel). Un carré peut avoir des détails d'implémentation différents d'un cercle en termes de représentation interne de la position, ils devront donc remplacer une méthode pour fournir la fonctionnalité de déplacement.

Dans des exemples simples, cela se résume souvent à un Move public qui délègue juste tout le travail à un ReallyDoTheMove virtuel privé, donc cela semble être beaucoup de frais généraux sans aucun avantage.

Mais cette correspondance individuelle n'est pas une exigence. Par exemple, vous pouvez ajouter une méthode Animate à l'API publique de Shape et l'implémenter en appelant ReallyDoTheMove en boucle. Vous vous retrouvez avec deux API de méthodes publiques non virtuelles qui s'appuient toutes deux sur la seule méthode abstraite privée. Vos cercles et carrés n'ont pas besoin de faire de travail supplémentaire, pas plus que l'animer .

La classe de base définit l'interface publique utilisée par les consommateurs et définit une interface d'opérations primitives dont elle a besoin pour implémenter ces méthodes publiques. Les types dérivés sont chargés de fournir des implémentations de ces opérations primitives.

Je ne suis au courant d'aucune différence entre C # et C ++ qui changerait cet aspect de la conception de classe.

Adrian McCarthy
la source
Bonne trouvaille! Maintenant, je me souviens que j'ai trouvé exactement ce post vers lequel votre deuxième lien pointe (ou une copie), dans les années 2000. Je me souviens que je cherchais plus de preuves de la réclamation de mon collègue et que je n'ai rien trouvé dans le contexte C #, sauf C ++. Cette. Est. Il! :-) Mais, de retour en C #, il semble que ce modèle ne soit pas très utilisé. Peut-être que les gens se sont rendus compte que l'ajout ultérieur de fonctionnalités de base peut également casser les classes dérivées et que l'utilisation stricte du TMP ou du NVIP au lieu des méthodes virtuelles publiques n'a pas toujours de sens.
Peter Perot
Note à soi-même: bon de savoir que ce modèle a un nom: NVIP.
Peter Perot