Lancer une exception ou laisser le code échouer

Je me demande s'il y a des avantages et des inconvénients contre ce style:

private void LoadMaterial(string name)
{
    if (_Materials.ContainsKey(name))
    {
        throw new ArgumentException("The material named " + name + " has already been loaded.");
    }

    _Materials.Add(
        name,
        Resources.Load(string.Format("Materials/{0}", name)) as Material
    );
}

Cette méthode devrait, pour chacun name, être exécutée une seule fois. _Materials.Add()lancera une exception si elle sera appelée plusieurs fois pour le même name. En conséquence, ma garde est-elle complètement redondante ou présente-t-elle des avantages moins évidents?

C'est C #, Unité, si quelqu'un est intéressé.

L'avantage est que votre exception "personnalisée" contient un message d'erreur qui a du sens pour quiconque appelle cette fonction sans savoir comment elle est mise en œuvre (ce qui pourrait être vous à l'avenir!).

Certes, dans ce cas, ils seraient probablement en mesure de deviner ce que signifiait l'exception "standard", mais vous indiquez toujours qu'ils ont violé votre contrat, plutôt que de trébucher sur un étrange bogue dans votre code.

Je suis d'accord avec la réponse d' Ixrec . Cependant, vous pouvez envisager une troisième alternative: rendre la fonction idempotente. En d’autres termes, revenez tôt au lieu de lancer un ArgumentException. C'est souvent préférable si vous êtes obligé de vérifier s'il a déjà été chargé avant d'appeler à LoadMaterialchaque fois. Moins vous avez de conditions préalables, moins la charge cognitive du programmeur est importante.

Lancer une exception serait préférable si c'est vraiment une erreur de programmeur, où il devrait être évident et connu au moment de la compilation si le matériel a déjà été chargé, sans avoir à vérifier au moment de l'exécution avant d'appeler.

La question fondamentale que vous devez vous poser est la suivante: que voulez-vous que l'interface de votre fonction soit? En particulier, la condition est-elle _Materials.ContainsKey(name)une condition préalable à la fonction?

Si ce n'est pas une condition préalable, la fonction doit fournir un résultat bien défini pour toutes les valeurs possibles de name. Dans ce cas, l'exception levée si elle namene fait pas partie de _Materialsl'interface devient une partie de l'interface de la fonction. Cela signifie que cela doit faire partie de la documentation de l'interface et que si vous décidiez de modifier cette exception à l'avenir, ce serait un changement d'interface révolutionnaire .

La question la plus intéressante est de savoir ce qui se passe si c'est une condition préalable. Dans ce cas, la condition préalable elle-même devient une partie de l'interface de la fonction, mais la manière dont une fonction se comporte lorsque cette condition est violée ne fait pas nécessairement partie de l'interface.

Dans ce cas, l’approche que vous avez publiée, qui consiste à rechercher les violations des conditions préalables et à signaler l’erreur, correspond à ce que l’on appelle la programmation défensive . La programmation défensive est bonne en ce sens qu’elle avertit l’utilisateur tôt quand il fait une erreur et appelle la fonction avec un argument bidon. En effet, cela alourdit considérablement la charge de maintenance, car le code utilisateur peut dépendre du fait que la fonction gère les violations de conditions préalables d’une certaine manière. En particulier, si vous constatez que la vérification de l'exécution devient jamais un goulot d'étranglement en termes de performances (peu probable dans un cas aussi simple, mais assez commun pour des conditions préalables plus complexes), vous ne pourrez peut-être plus le supprimer.

Il s’avère que ces inconvénients peuvent être très importants, ce qui a donné une mauvaise réputation à la programmation défensive dans certains milieux. Cependant, l'objectif initial est toujours valable: nous voulons que les utilisateurs de notre fonction remarquent tôt lorsqu'ils commettent une erreur.

C'est pourquoi de nombreux développeurs proposent aujourd'hui une approche légèrement différente pour ce type de problèmes. Au lieu de lancer une exception, ils utilisent un mécanisme de type assert pour vérifier les conditions préalables. Autrement dit, les conditions préalables peuvent être vérifiées dans les versions de débogage pour aider les utilisateurs à détecter les erreurs plus tôt, mais elles ne font pas partie de l'interface des fonctions. La différence peut sembler subtile au premier abord, mais elle peut faire une énorme différence dans la pratique.

Techniquement, appeler une fonction avec des conditions préalables violées constitue un comportement indéfini. Mais l’implémentation peut décider de détecter ces cas et d’en informer immédiatement l’utilisateur si cela se produit. Les exceptions ne sont malheureusement pas un bon outil pour implémenter ceci, car le code utilisateur peut réagir sur elles et pourrait donc commencer à compter sur leur présence.

Pour une explication détaillée des problèmes liés à l'approche défensive classique, ainsi qu'une implémentation possible pour les contrôles de condition préalable de type assert, reportez-vous à l'exposé de John Lakos intitulé Defensive Programming Done Right de CppCon 2014 ( diapositives , vidéo ).

Il y a déjà quelques réponses ici, mais voici la réponse qui prend Unity3D en compte (la réponse est très spécifique à Unity3D, je ferais la plupart de ces choses très différemment dans la plupart des contextes):

En général, Unity3D n'utilise pas les exceptions de manière traditionnelle. Si vous générez une exception dans Unity3D, ce n'est pas comme dans votre application .NET moyenne, à savoir que cela n'arrête pas le programme, vous pouvez configurer l'éditeur pour qu'il suspende. Il suffit de vous connecter. Cela peut facilement mettre le jeu dans un état non valide et créer un effet de cascade rendant les erreurs difficiles à localiser. Donc, je dirais que dans le cas de Unity, laisser Addune exception est une option particulièrement indésirable.

Toutefois, l'examen de la vitesse des exceptions ne constitue pas un cas d'optimisation prématurée dans certains cas, en raison du fonctionnement de Mono dans Unity sur certaines plates-formes. En fait, Unity3D sur iOS prend en charge certaines optimisations de script avancées, et les exceptions désactivées * constituent l'un des effets secondaires de l'une d'entre elles. C’est vraiment quelque chose à prendre en compte car ces optimisations se sont révélées très utiles pour de nombreux utilisateurs, ce qui représente un cas réaliste pour envisager de limiter l’utilisation des exceptions dans Unity3D. (* exceptions gérées du moteur, pas votre code)

Je dirais que dans Unity, vous voudrez peut-être adopter une approche plus spécialisée. Ironiquement, une réponse très votée au moment de la rédaction montre que je pourrais implémenter un tel système, notamment dans le contexte d’Unity3D (ailleurs, il est vraiment inacceptable, et même dans Unity, il est assez inélégant).

Une autre approche que je considérerais en réalité n’indique pas une erreur en ce qui concerne l’appelant, mais utilise plutôt les Debug.LogXXfonctions. De cette façon, vous obtenez le même comportement que de lancer une exception non gérée (à cause de la façon dont Unity3D les gère) sans risquer de placer quelque chose dans un état étrange en bout de ligne. Déterminez également s’il s’agit vraiment d’une erreur (essayer de charger deux fois le même matériel est-il nécessairement une erreur dans votre cas? Ou peut-être est-ce un cas où cela Debug.LogWarnings’applique plus).

Et en ce qui concerne l'utilisation d'éléments tels que des Debug.LogXXfonctions plutôt que des exceptions, vous devez toujours considérer ce qui se produit lorsqu'une exception serait levée à partir de quelque chose qui retourne une valeur (comme GetMaterial). J'ai tendance à aborder ceci en passant null avec l'enregistrement de l'erreur ( encore une fois, uniquement dans Unity ). J'utilise ensuite des contrôles nuls dans MonoBehaviors pour garantir que toute dépendance, telle qu'un matériau, n'est pas une valeur nulle, et désactive le MonoBehavior si c'est le cas. Voici un exemple de comportement simple nécessitant quelques dépendances:

    public void Awake()
    {
        _inputParameters = GetComponent<VehicleInputParameters>();
        _rigidbody = GetComponent<Rigidbody>();
        _rigidbodyTransform = _rigidbody.transform;
        _raycastStrategySelector = GetComponent<RaycastStrategySelectionBehavior>();

        _trackParameters =
            SceneManager.InstanceOf.CurrentSceneData.GetValue<TrackParameters>();

        this.DisableIfNull(() => _rigidbody);
        this.DisableIfNull(() => _raycastStrategySelector);
        this.DisableIfNull(() => _inputParameters);
        this.DisableIfNull(() => _trackParameters);
    }

SceneData.GetValue<>est similaire à votre exemple en ce sens qu'il appelle une fonction sur un dictionnaire qui lève une exception. Mais au lieu de lancer une exception, il utilise Debug.LogErrorune trace de pile, comme une exception normale, et renvoie null. Les vérifications qui suivent * désactiveront le comportement au lieu de le laisser continuer d'exister dans un état non valide.

* les contrôles ressemblent à cela à cause d'un petit assistant que j'utilise qui imprime un message formaté lorsqu'il désactive l'objet du jeu **. Des contrôles nuls simples avec iftravail ici (** les contrôles de l'assistant sont uniquement compilés dans des versions Debug (comme des assertions. L'utilisation de lambdas et d'expressions comme celle-là dans Unity peut nuire à la performance)

J'aime les deux réponses principales, mais je voudrais suggérer que les noms de vos fonctions pourraient être améliorés. Je suis habitué à Java, donc à YMMV, mais une méthode "add" devrait, IMO, ne pas lancer d'exception si l'élément est déjà là. Il devrait ajouter l'élément à nouveau ou, si la destination est un ensemble, ne rien faire. Etant donné que ce n'est pas le comportement de Materials.Add, il convient de renommer TryPut ou AddOnce ou AddOrThrow ou similaire.

De même, votre LoadMaterial doit être renommé LoadIfAbsent ou Put, TryLoad ou LoadOrThrow (selon que vous utilisez la réponse n ° 1 ou n ° 2) ou une réponse similaire.

Suivez les conventions de nommage de C # Unity quelles qu’elles soient.

Cela sera particulièrement utile si vous avez d’autres fonctions AddFoo et LoadBar où il est permis de charger deux fois la même chose. Sans noms clairs, les développeurs seront frustrés.

Toutes les réponses apportent des idées précieuses, je voudrais les combiner:

Décidez de la sémantique prévue et attendue de l' LoadMaterial()opération. Au moins les options suivantes existent:

• Une condition préalable sur name∉ LoadedMaterials : →

  En cas de violation d'une condition préalable, l'effet de LoadMaterial()n'est pas spécifié (comme dans la réponse de ComicSansMS ). Cela permet plus de liberté dans la mise en œuvre et les modifications futures de LoadMaterial(). Ou,

• les effets d'appeler LoadMaterial(name)avec name∈ LoadedMaterials sont spécifiés; Soit:

  • la spécification indique qu'une exception est levée; ou

  • la spécification , on indique que le résultat est idempotent (comme dans la réponse par Karl Bielefeldt ),

    • renvoyer éventuellement un booléen "la collection a-t-il été modifié" (comme proposé par User949300 et proposé (selon certaines interprétations) par Mrlveck .

Une fois la sémantique choisie, vous devez choisir une implémentation. Les options et considérations suivantes ont été proposées:

• Lance une exception personnalisée (comme suggéré par Ixrec ) →

  L'avantage est que votre exception "personnalisée" a un message d'erreur significatif pour quiconque appelle cette fonction - Ixrec et User16547

  • Pour éviter les coûts liés à la vérification répétée des name∉ ChargéesMatériaux, vous pouvez suivre les conseils de Marc van Leeuwen :

    ... Au lieu de cela, envisagez de vous plonger dans _Materials.Add sans condition, puis capturez une erreur potentielle et envoyez une autre erreur par le gestionnaire.

• Laissons Dictionay.Add lever l'exception →

  Le code de lancement d'exception est redondant. - Jon Raynor

  Bien que la plupart des électeurs soient plus d'accord avec Ixrec

  Les raisons supplémentaires pour choisir cette implémentation sont:

  • que les appelants peuvent déjà gérer les ArgumentExceptionexceptions, et
  • pour éviter de perdre des informations de pile.

  Mais si ces deux raisons sont importantes, vous pouvez également dériver votre exception personnalisée ArgumentExceptionet utiliser l’original en tant qu’exception chaînée.

• Rendez LoadMaterial()idempotent comme réponse à Karl Bielefeldt et votez le plus souvent (75 fois).

  • renvoyer éventuellement un booléen "la collection a-t-il été modifié" (comme proposé par User949300 et proposé (selon certaines interprétations) par Mrlveck .

  Les options d'implémentation pour ce comportement:

  • Vérifiez avec Dictionary.ContainsKey ()

  • Appelez toujours Dictionary.Add () attrapez-le ArgumentExceptions'il lève quand la clé à insérer existe déjà et ignorez cette exception. Indiquez que le fait d'ignorer l'exception est ce que vous avez l'intention de faire et pourquoi.

    • Quand LoadMaterials()est (presque) toujours appelé une fois pour chaque name, cela évite le coût en vérifiant à plusieurs reprises name∉ LoadedMaterials cf. Marc van Leeuwen . cependant,
    • quand LoadedMaterials()est souvent appelé plusieurs fois pour la même chose name, cela implique le coût (coûteux) de lancer la ArgumentExceptionpile et de la dérouler.

  • Je pensais qu’il existait un TryAddanalogue de méthode avec TryGet () qui vous aurait permis d’éviter l’exception coûteuse lors du lancement et du dépilage de pile des appels défaillants de Dictionary.Add.

    Mais cette TryAddméthode ne semble pas exister.

Le code de lancement d'exception est redondant.

Si vous appelez:

_Materials.Add (name, Resources.Load (string.Format ("Materials / {0}", name))) en tant que Material

avec la même clé, il jettera un

System.ArgumentException

Le message sera: "Un élément avec la même clé a déjà été ajouté."

Le ContainsKeycontrôle est redondant car le même comportement est obtenu sans lui. Le seul élément différent est le message réel dans l'exception. S'il y a un réel avantage à avoir un message d'erreur personnalisé, le code de garde a un certain mérite.

Sinon, je refacturerais probablement la garde dans ce cas.

Je pense que c'est davantage une question de conception d'API qu'une question de convention de codage.

Quel est le résultat attendu (le contrat) de l'appel:

LoadMaterial("wood");

Si l'appelant peut s'attendre à / est assuré qu'après l'appel de cette méthode, le matériau "bois" est chargé, alors je ne vois aucune raison de lever une exception lorsque ce matériau est déjà chargé.

Si une erreur se produit lors du chargement du matériau, par exemple, si la connexion à la base de données n'a pas pu être ouverte ou s'il n'y a pas de matériau "bois" dans le référentiel, il est correct de déclencher une exception pour avertir l'appelant de ce problème.

Pourquoi ne pas changer la méthode pour qu’elle retourne 'true' si la matière est ajoutée et 'false' si elle ne l’était pas?

private bool LoadMaterial(string name)
{
   if (_Materials.ContainsKey(name))

    {

        return false; //already present
    }

    _Materials.Add(
        name,
        Resources.Load(string.Format("Materials/{0}", name)) as Material
    );

    return true;

}