Validation des paramètres de constructeur en C # - Meilleures pratiques

34

Quelle est la meilleure pratique pour la validation des paramètres de constructeur?

Supposons un simple morceau de C #:

public class MyClass
{
    public MyClass(string text)
    {
        if (String.IsNullOrEmpty(text))
            throw new ArgumentException("Text cannot be empty");

        // continue with normal construction
    }
}

Serait-il acceptable de lancer une exception?

L'alternative que j'ai rencontrée était la pré-validation, avant d'instancier:

public class CallingClass
{
    public MyClass MakeMyClass(string text)
    {
        if (String.IsNullOrEmpty(text))
        {
            MessageBox.Show("Text cannot be empty");
            return null;
        }
        else
        {
            return new MyClass(text);
        }
    }
}
MPelletier
la source
10
"acceptable de lancer une exception?" Quelle est l'alternative?
S.Lott
10
@ S.Lott: ne rien faire. Définir une valeur par défaut. Imprimez un message sur la console / logfile. Sonner une cloche. Clignote une lumière.
FrustratedWithFormsDesigner
3
@FrustratedWithFormsDesigner: "Ne rien faire?" Comment la contrainte "texte non vide" sera-t-elle satisfaite? "Définir une valeur par défaut"? Comment la valeur de l'argument texte sera-t-elle utilisée? Les autres idées vont bien, mais ces deux-là mèneraient à un objet dans un état qui violerait les contraintes de cette classe.
S.Lott
1
@ S.Lott Par peur de me répéter, j'étais ouvert à la possibilité d'une autre approche, que je ne connaissais pas. Je crois que je ne suis pas tous au courant, je le sais avec certitude.
MPelletier
3
@MPelletier, la validation des paramètres à l’avance constituera une violation de DRY. (Ne vous répétez pas) car vous devrez copier cette pré-validation dans tout code essayant d'instancier cette classe.
CaffGeek

Réponses:

26

J'ai tendance à effectuer toute ma validation dans le constructeur. C'est indispensable car je crée presque toujours des objets immuables. Pour votre cas spécifique, je pense que cela est acceptable.

if (string.IsNullOrEmpty(text))
    throw new ArgumentException("message", "text");

Si vous utilisez .NET 4, vous pouvez le faire. Bien entendu, cela dépend si vous considérez qu'une chaîne ne contenant que des espaces n'est pas valide.

if (string.IsNullOrWhiteSpace(text))
    throw new ArgumentException("message", "text");
ChaosPandion
la source
Je ne suis pas sûr que son "cas spécifique" soit suffisamment spécifique pour donner un tel conseil. Nous n'avons aucune idée s'il est logique dans ce contexte de lever une exception ou simplement de permettre à l'objet d'exister de toute façon.
FrustratedWithFormsDesigner
@FrustratedWithFormsDesigner - Je suppose que vous m'avez voté pour cela? Bien sûr, vous avez raison, j'extrapole, mais il est clair que cet objet théorique doit être invalide si le texte saisi est vide. Souhaitez-vous vraiment avoir à appeler Initpour recevoir une sorte de code d'erreur lorsqu'une exception fonctionnera aussi bien et forcer votre objet à toujours conserver un état valide?
ChaosPandion
2
J'ai tendance à scinder ce premier cas en deux exceptions distinctes: une qui teste la nullité et jette un ArgumentNullExceptionet une autre qui vérifie si vide et jette un ArgumentException. Permet à l'appelant d'être plus difficile s'il le souhaite lors de la gestion des exceptions.
Jesse C. Slicer
1
@ Jesse - J'ai utilisé cette technique, mais je suis toujours en garde sur son utilité globale.
ChaosPandion
3
+1 pour les objets immuables! Je les utilise aussi chaque fois que possible (personnellement, je trouve presque toujours).
22

Beaucoup de gens disent que les constructeurs ne devraient pas lancer d'exceptions. KyleG sur cette page , par exemple, fait justement cela. Honnêtement, je ne peux pas penser à une raison pour laquelle pas.

En C ++, le lancement d'une exception à partir d'un constructeur est une mauvaise idée car cela vous laisse avec de la mémoire allouée contenant un objet non initialisé auquel vous n'avez aucune référence (c'est-à-dire qu'il s'agit d'une fuite de mémoire classique). C’est probablement de là que vient la stigmatisation: un groupe de développeurs C ++ de la vieille école ont mis en demi-arrière leur apprentissage de C # et ont simplement appliqué ce qu’ils savaient du C ++. En revanche, dans Objective-C, Apple a séparé l’étape d’allocation de l’étape d’initialisation afin que les constructeurs de cette langue puissent générer des exceptions.

C # ne peut pas perdre de la mémoire d'un appel infructueux à un constructeur. Même certaines classes du .NET Framework jetteront des exceptions dans leurs constructeurs.

Fourmi
la source
Vous allez tellement remuer des plumes avec votre commentaire C ++. :)
ChaosPandion, le
5
Ehh, C ++ est une langue géniale, mais l’un de mes animaux préférés sont les gens qui apprennent bien une langue et qui écrivent comme ça tout le temps . C # n'est pas C ++.
Ant
10
Il peut toujours être dangereux de lancer des exceptions d’un ctor en C #, car celui-ci aurait peut-être alloué des ressources non gérées qui ne seraient alors jamais éliminées. Et le finaliseur doit être écrit pour être défensif contre le scénario dans lequel un objet incomplètement construit est finalisé. Mais ces scénarios sont relativement rares. Un prochain article sur le Centre de développement C # couvrira ces scénarios et explique comment coder de manière défensive autour d'eux, alors surveillez cet espace pour plus de détails.
Eric Lippert
6
hein? lancer une exception à la cteur est la seule chose que vous pouvez faire en c ++ si vous ne pouvez pas construire l'objet, et il n'y a aucune raison pour cela est de provoquer une fuite de mémoire (bien évidemment son jusqu'à).
jk.
@jk: Hmm, tu as raison! Bien qu'il semble une alternative, il est préférable de signaler les objets défectueux comme des "zombies", ce que la STL fait apparemment au lieu de lancer des exceptions: yosefk.com/c++fqa/exceptions.html#fqa-17.2 La solution d'Objective-C est beaucoup plus ordonnée.
Ant
12

Lance une exception si la classe ne peut pas être mise dans un état cohérent en ce qui concerne son utilisation sémantique. Sinon ne le faites pas. N'autorisez JAMAIS un objet à exister dans un état incohérent. Cela implique de ne pas fournir de constructeurs complets (comme avoir un constructeur vide + initialize () avant que votre objet ne soit réellement construit) ... JUST SAY NO!

À la limite, tout le monde le fait. Je l'ai fait l'autre jour pour un objet très étroitement utilisé dans un périmètre étroit. Un jour ou l'autre, quelqu'un ou moi-même paierons probablement le prix de ce feuillet dans la pratique.

Je devrais noter que par "constructeur" je veux dire la chose que le client appelle pour construire l'objet. Cela pourrait tout aussi bien être autre chose qu'une construction réelle appelée "Constructeur". Par exemple, quelque chose comme cela en C ++ ne violerait pas le principe IMNSHO:

struct funky_object
{
  ...
private:
  funky_object();
  bool initialize(std::string);

  friend boost::optional<funky_object> build_funky(std::string);
};
boost::optional<funky_object> build_funky(std::string str)
{
  funky_object fo;
  if (fo.initialize(str)) return fo;
  return boost::optional<funky_object>();
}

Comme le seul moyen de créer un funky_objectest d'appeler build_funkyle principe de ne jamais permettre à un objet invalide d'exister reste intact même si le "constructeur" lui-même ne termine pas le travail.

Cela représente beaucoup de travail supplémentaire pour un gain discutable (peut-être même une perte). Je préférerais toujours la route d'exception.

Edward Strange
la source
9

Dans ce cas, j'utiliserais la méthode de l'usine. Fondamentalement, votre classe a uniquement des constructeurs privés et une méthode de fabrique qui renvoie une instance de votre objet. Si les paramètres initiaux ne sont pas valides, il suffit de renvoyer null et de laisser le code appelant décider quoi faire.

public class MyClass
{
    private MyClass(string text)
    {
        //normal construction
    }

    public static MyClass MakeMyClass(string text)
    {
        if (String.IsNullOrEmpty(text))
            return null;
        else
            return new MyClass(text);
    }
}
public class CallingClass
{
    public MyClass MakeMyClass(string text)
    {
        var cls = MyClass.MakeMyClass(text);
        if(cls == null)
             //show messagebox or throw exception
        return cls;
    }
}

Ne jamais lancer d'exceptions à moins que les conditions ne soient exceptionnelles. Je pense que dans ce cas, une valeur vide peut être passée facilement. Si tel est le cas, l'utilisation de ce modèle éviterait les exceptions et les pénalités de performance qu'elles entraînent tout en maintenant l'état MyClass valide.

Donn Relacion
la source
1
Je ne vois pas l'avantage. Pourquoi tester le résultat d'une usine pour null est préférable à un test-catch pouvant capturer l'exception du constructeur? Votre approche semble être "ignorer les exceptions, revenez à la vérification explicite de la valeur de retour des méthodes pour les erreurs". Nous avons depuis longtemps accepté que les exceptions étaient généralement plus efficaces que de devoir explicitement tester chaque erreur de valeur de retour de méthode. Votre conseil semble donc suspect. Je voudrais voir une justification pour expliquer pourquoi vous pensez que la règle générale ne s'applique pas ici.
DW
nous n’avons jamais accepté que les exceptions étaient supérieures aux codes de retour, elles peuvent être un énorme succès si elles sont lancées trop souvent. Toutefois, le lancement d'une exception à partir d'un constructeur entraînera une perte de mémoire, même dans .NET, si vous avez alloué un élément non géré. Vous devez faire beaucoup de travail dans votre finaliseur pour compenser ce cas. En tout cas, l’usine est une bonne idée ici, et TBH devrait lancer une exception au lieu de renvoyer une valeur nulle. En supposant que vous ne créez que quelques «mauvaises» classes, il est préférable de lancer la projection en usine.
gbjbaanb
2
  • Un constructeur ne devrait avoir aucun effet secondaire.
    • Tout ce qui dépasse l’initialisation du champ privé doit être considéré comme un effet secondaire.
    • Un constructeur avec des effets secondaires enfreint le principe de responsabilité unique (SRP) et va à l'encontre de l'esprit de la programmation orientée objet (OOP).
  • Un constructeur doit être léger et ne doit jamais échouer.
    • Par exemple, je frémis toujours lorsque je vois un bloc try-catch dans un constructeur. Un constructeur ne doit pas lancer d’exceptions ni enregistrer les erreurs.

On pourrait raisonnablement remettre en question ces directives et dire: "Mais je ne respecte pas ces règles et mon code fonctionne bien!" À cela, je répondrais: "Cela peut être vrai jusqu'à ce que ce ne soit pas le cas."

  • Les exceptions et les erreurs à l'intérieur d'un constructeur sont très inattendues. Sauf indication contraire de leur part, les futurs programmeurs ne seront pas enclins à entourer ces appels de constructeur de code défensif.
  • Si quelque chose échoue en production, la trace de pile générée peut être difficile à analyser. Le haut de la trace de la pile peut pointer vers l'appel du constructeur, mais de nombreux événements se produisent dans le constructeur et il peut ne pas pointer vers le LOC réel qui a échoué.
    • J'ai analysé de nombreuses traces de pile .NET là où c'était le cas.
Jim G.
la source
0

Cela dépend de ce que fait MyClass. Si MyClass était en réalité une classe de référentiel de données et que le paramètre text était une chaîne de connexion, il est recommandé de générer une exception ArgumentException. Toutefois, si MyClass est la classe StringBuilder (par exemple), vous pouvez simplement la laisser vide.

Cela dépend donc de la pertinence du texte du paramètre pour la méthode - l'objet a-t-il un sens avec une valeur nulle ou vide?

Steven Striga
la source
2
Je pense que la question implique que la classe exige en fait que la chaîne ne soit pas vide. Ce n'est pas le cas dans votre exemple StringBuilder.
Sergio Acosta
Alors la réponse doit être oui - il est acceptable de lancer une exception. Pour éviter d'avoir à essayer / intercepter cette erreur, vous pouvez utiliser un modèle d'usine pour gérer la création d'objet pour vous, ce qui inclurait une casse de chaîne vide.
Steven Striga
0

Ma préférence est de définir une valeur par défaut, mais je sais que Java a la bibliothèque "Apache Commons" qui fait quelque chose comme cela, et je pense que c'est aussi une assez bonne idée. Je ne vois pas d'inconvénient à lancer une exception si la valeur non valide placerait l'objet dans un état inutilisable. une ficelle n'est pas un bon exemple, mais que se passerait-il si c'était pour le DI du pauvre? Vous ne pourriez pas fonctionner si une valeur de nullétait passée à la place d'une ICustomerRepositoryinterface, par exemple. Dans une situation comme celle-ci, lancer une exception est la bonne façon de gérer les choses, dirais-je.

Wayne Molina
la source
-1

Quand j’avais l’habitude de travailler en c ++, je n’avais pas pour habitude de lancer une exception dans le constructeur à cause d’un problème de fuite de mémoire qu’elle pouvait causer, j’avais appris à la dure. Quiconque travaille avec c ++ sait à quel point une fuite de mémoire peut être difficile et problématique.

Mais si vous êtes en c # / Java, vous n’avez pas ce problème, car le garbage collector va collecter la mémoire. Si vous utilisez C #, je pense qu'il est préférable d'utiliser property pour obtenir un moyen agréable et cohérent de s'assurer que les contraintes de données sont garanties.

public class MyClass
{
    private string _text;
    public string Text 
    {
        get
        {
            return _text;
        } 
        set
        {
            if (string.IsNullOrWhiteSpace(value))
                throw new ArgumentException("Text cannot be empty");
            _text = value;
        } 
    }

    public MyClass(string text)
    {
        Text = text;
        // continue with normal construction
    }
}
ajitdh
la source
-3

Lancer une exception sera une vraie douleur pour les utilisateurs de votre classe. Si la validation pose problème, la création de l'objet est généralement effectuée en 2 étapes.

1 - Créer l'instance

2 - Appelez une méthode Initialize avec un résultat.

OU

Appelez une méthode / fonction qui crée la classe pour vous et qui peut effectuer la validation.

OU

Ayez un drapeau isValid, ce que je n'aime pas particulièrement mais que certaines personnes aiment bien.

Tremper
la source
3
@Dunk, c'est faux.
CaffGeek
4
@Chad, c'est votre avis, mais mon avis n'est pas faux. C'est juste différent du vôtre. Je trouve le code try-catch beaucoup plus difficile à lire et j'ai vu beaucoup plus d'erreurs créées lorsque les utilisateurs utilisent try-catch pour la gestion d'erreurs triviales par rapport aux méthodes plus traditionnelles. Cela a été mon expérience et ce n'est pas faux. C'est ce que c'est.
Dunk
5
Le POINT est qu'après que j'appelle un constructeur, s'il ne crée pas parce qu'une entrée est invalide, c'est un EXCEPTION. Les données de l'application sont maintenant dans un état incohérent / invalide si je crée simplement l'objet et mets un indicateur isValid = false. Avoir à tester après la création d’une classe s’il est valide est une conception horrible et très sujette aux erreurs. Et, vous avez dit, avoir un constructeur, puis appeler initialiser ... Et si je n'appelle pas initialiser? Quoi alors? Et si Initialize obtenait de mauvaises données, puis-je lancer une exception maintenant? Votre classe ne devrait pas être difficile à utiliser.
CaffGeek
5
@Dunk, si vous trouvez les exceptions difficiles à utiliser, vous les utilisez mal. En termes simples, une mauvaise entrée a été fournie à un constructeur. C'est une exception. L'application ne doit pas continuer à s'exécuter sauf si elle est corrigée. Période. Si tel est le cas, vous vous retrouvez avec un problème pire: de mauvaises données. Si vous ne savez pas comment gérer l'exception, vous la laissez bouillonner la pile d'appels jusqu'à ce qu'elle puisse être traitée, même si cela signifie simplement la journalisation et l'arrêt de l'exécution. En termes simples, vous n'avez pas besoin de gérer les exceptions, ni de les tester. Ils travaillent simplement.
CaffGeek
3
Désolé, c'est trop d'un anti-modèle à prendre en compte.
MPelletier le