Quels sont les bons moyens d'équilibrer les exceptions informatives et le code propre?

11

Avec notre SDK public, nous avons tendance à vouloir donner des messages très informatifs sur les raisons pour lesquelles une exception se produit. Par exemple:

if (interfaceInstance == null)
{
     string errMsg = string.Format(
          "Construction of Action Argument: {0}, via the empty constructor worked, but type: {1} could not be cast to type {2}.",
          ParameterInfo.Name,
          ParameterInfo.ParameterType,
          typeof(IParameter)
    );

    throw new InvalidOperationException(errMsg);
}

Cependant, cela a tendance à encombrer le flux du code, car il a tendance à mettre l'accent sur les messages d'erreur plutôt que sur ce que fait le code.

Un collègue a commencé à refactoriser une partie de l'exception en lançant quelque chose comme ceci:

if (interfaceInstance == null)
    throw EmptyConstructor();

...

private Exception EmptyConstructor()
{
    string errMsg = string.Format(
          "Construction of Action Argument: {0}, via the empty constructor worked, but type: {1} could not be cast to type {2}.",
          ParameterInfo.Name,
          ParameterInfo.ParameterType,
          typeof(IParameter)
    );

    return new InvalidOperationException(errMsg);
}

Ce qui rend la logique du code plus facile à comprendre, mais ajoute de nombreuses méthodes supplémentaires pour gérer les erreurs.

Quelles sont les autres façons d'éviter le problème de la "logique d'encombrement des messages à longue exception"? Je pose principalement des questions sur C # /. NET idiomatique, mais comment les autres langages le gèrent également.

[Éditer]

Ce serait bien d'avoir les avantages et les inconvénients de chaque approche également.

FriendlyGuy
la source
4
À mon humble avis, la solution de votre collègue est très bonne, et je suppose que si vous obtenez vraiment beaucoup de méthodes supplémentaires de ce type, vous pouvez réutiliser au moins certaines d'entre elles. Avoir beaucoup de petites méthodes est bien quand vos méthodes sont bien nommées, tant qu'elles créent des blocs de construction faciles à comprendre de votre programme - cela semble être le cas ici.
Doc Brown
@DocBrown - Oui, j'aime l'idée (ajoutée comme réponse ci-dessous, avec des avantages / inconvénients), mais pour intellisense et pour le nombre potentiel de méthodes, cela commence à ressembler à de l'encombrement.
FriendlyGuy
1
Réflexion: ne faites pas du message le porteur de tous les détails de l'exception. Une combinaison de l'utilisation de la Exception.Datapropriété, de la capture d'exceptions "pointilleuse", de la capture de code et de l'ajout de son propre contexte, ainsi que de la pile d'appels capturés, fournissent toutes des informations qui devraient permettre des messages beaucoup moins verbeux. System.Reflection.MethodBaseSemble enfin prometteur pour fournir des détails à passer à votre méthode de "construction d'exception".
radarbob
@radarbob suggérez-vous que les exceptions sont peut-être trop verbeuses? Peut-être que nous le faisons trop comme la journalisation.
FriendlyGuy
1
@MackleChan, j'ai lu que le paradigme ici est de mettre des informations dans un message qui tente de dire 'zactly ce qui s'est passé, est nécessairement grammaticalement correct, et une prétention d'IA: ".. via le constructeur vide a fonctionné, mais. .." " Vraiment? Mon codeur médico-légal interne voit cela comme une conséquence évolutive de l'erreur commune des relances perdant la trace de la pile et de l'ignorance Exception.Data. L'accent devrait être mis sur la capture de la télémétrie. Refactoring ici est bien, mais il manque le problème.
radarbob

Réponses:

10

Pourquoi ne pas avoir des classes d'exception spécialisées?

if (interfaceInstance == null)
{
    throw new ThisParticularEmptyConstructorException(<maybe a couple parameters>);
}

Cela pousse la mise en forme et les détails à l'exception elle-même et laisse la classe principale épurée.

ptyx
la source
1
Avantages: Très propre et organisé, fournit des noms significatifs pour chaque exception. Inconvénients: potentiellement beaucoup de classes d'exceptions supplémentaires .
FriendlyGuy
Si cela est important, vous pouvez avoir un nombre limité de classes d'exceptions, passer la classe d'où provient l'exception en tant que paramètre et avoir un commutateur géant à l'intérieur de classes d'exceptions plus génériques. Mais cela a une légère odeur - je ne suis pas sûr que j'irais là-bas.
ptyx
ça sent vraiment mauvais (exceptions étroitement couplées avec les classes). Je suppose qu'il serait difficile d'équilibrer les messages d'exception personnalisables par rapport au nombre d'exceptions.
FriendlyGuy
7

Microsoft semble (en regardant la source .NET) utiliser parfois des chaînes de ressources / environnement. Par exemple ParseDecimal:

throw new OverflowException(Environment.GetResourceString("Overflow_Decimal"));

Avantages:

  • Centraliser les messages d'exceptions, permettant leur réutilisation
  • Garder le message d'exception (sans doute peu important à coder) loin de la logique des méthodes
  • Le type d'exception levée est clair
  • Les messages peuvent être localisés

Les inconvénients:

  • Si un message d'exception est modifié, ils changent tous
  • Le message d'exception n'est pas aussi facilement accessible au code lançant l'exception.
  • Le message est statique et ne contient aucune information sur les valeurs incorrectes. Si vous voulez le formater, c'est plus d'encombrement dans le code.
FriendlyGuy
la source
2
Vous avez omis un énorme avantage: Localisation du texte d'exception
17 du 26
@ 17of26 - Bon point, l'ajouta.
FriendlyGuy
J'ai surévalué votre réponse, mais les messages d'erreur ainsi créés sont "statiques"; vous ne pouvez pas leur ajouter de modificateurs comme l'OP le fait dans son code. Vous avez donc effectivement laissé de côté certaines de ses fonctionnalités.
Robert Harvey
@RobertHarvey - Je l'ai ajouté comme con. Cela explique pourquoi les exceptions intégrées ne vous donnent jamais vraiment d'informations locales. Aussi, pour info je suis OP (je connaissais cette solution, mais je voulais savoir si d'autres ont de meilleures solutions).
FriendlyGuy
6
@ 17of26 en tant que développeur, je déteste les exceptions localisées avec passion. Je dois les délocaliser à chaque fois avant de pouvoir par exemple. google pour des solutions.
Konrad Morawski
2

Pour le scénario du SDK public, j'envisagerais fortement d'utiliser les contrats de code Microsoft car ils fournissent des erreurs informatives, des vérifications statiques et vous pouvez également générer de la documentation à ajouter aux documents XML et aux fichiers d'aide générés par Sandcastle . Il est pris en charge dans toutes les versions payantes de Visual Studio.

Un avantage supplémentaire est que si vos clients utilisent C #, ils peuvent tirer parti de vos assemblys de référence de contrat de code pour détecter les problèmes potentiels avant même d'exécuter leur code.

La documentation complète pour les contrats de code est ici .

James World
la source
2

La technique que j'utilise est de combiner et d'externaliser la validation et le lancement à une fonction d'utilité.

L'avantage le plus important est qu'il est réduit à une seule ligne dans la logique métier .

Je parie que vous ne pouvez pas faire mieux si vous ne pouvez pas le réduire davantage - pour éliminer toutes les validations d'arguments et les gardes d'état d'objet de la logique métier, en ne gardant que les conditions exceptionnelles opérationnelles.

Il y a, bien sûr, des façons de le faire - Langage fortement tapé, conception "aucun objet invalide autorisé à tout moment", Design by Contract , etc.

Exemple:

internal static class ValidationUtil
{
    internal static void ThrowIfRectNullOrInvalid(int imageWidth, int imageHeight, Rect rect)
    {
        if (rect == null)
        {
            throw new ArgumentNullException("rect");
        }
        if (rect.Right > imageWidth || rect.Bottom > imageHeight || MoonPhase.Now == MoonPhase.Invisible)
        {
            throw new ArgumentException(
                message: "This is uselessly informative",
                paramName: "rect");
        }
    }
}

public class Thing
{
    public void DoSomething(Rect rect)
    {
        ValidationUtil.ThrowIfRectNullOrInvalid(_imageWidth, _imageHeight, rect);
        // rest of your code
    }
}
rwong
la source
1

[note] J'ai copié ceci de la question dans une réponse au cas où il y aurait des commentaires à ce sujet.

Déplacez chaque exception dans une méthode de la classe, en prenant tous les arguments qui ont besoin de la mise en forme.

private Exception EmptyConstructor()
{
    string errMsg = string.Format(
          "Construction of Action Argument: {0}, via the empty constructor worked, but type: {1} could not be cast to type {2}.",
          ParameterInfo.Name,
          ParameterInfo.ParameterType,
          typeof(IParameter)
    );

    return new InvalidOperationException(errMsg);
}

Veuillez joindre toutes les méthodes d'exception dans la région, et placez - les à la fin de la classe.

Avantages:

  • Éloigne le message de la logique de base de la méthode
  • Permet d'ajouter des informations logiques à chaque message (vous pouvez passer des arguments à la méthode)

Les inconvénients:

  • Méthode encombrée. Potentiellement, vous pourriez avoir beaucoup de méthodes qui ne font que renvoyer des exceptions et qui ne sont pas vraiment liées à la logique métier.
  • Impossible de réutiliser les messages dans d'autres classes
FriendlyGuy
la source
Je pense que les inconvénients que vous avez cités l'emportent largement sur les avantages.
neontapir
@neontapir les inconvénients sont tous facilement abordés. Les méthodes auxiliaires doivent être affectées à IntentionRevealingNames et regroupées en certaines #region #endregion(ce qui les rend cachées à l'EDI par défaut), et si elles sont applicables à différentes classes, mettez-les dans une internal static ValidationUtilityclasse. Au fait, ne vous plaignez jamais de longs noms d'identifiants devant un programmeur C #.
rwong
Je me plaindrais cependant des régions. À mon avis, si vous avez envie de recourir aux régions, la classe a probablement trop de responsabilités.
neontapir
0

Si vous pouvez vous en sortir avec des erreurs un peu plus générales, vous pouvez écrire une fonction de conversion générique statique publique pour vous, qui déduit le type de source:

public static I CastOrThrow<I,T>(T t, string source)
{
    if (t is I)
        return (I)t;

    string errMsg = string.Format(
          "Failed to complete {0}, because type: {1} could not be cast to type {2}.",
          source,
          typeof(T),
          typeof(I)
        );

    throw new InvalidOperationException(errMsg);
}


/// and then:

var interfaceInstance = SdkHelper.CastTo<IParameter>(passedObject, "Action constructor");

Il existe des variantes possibles (pensez SdkHelper.RequireNotNull()), qui vérifient uniquement les exigences sur les entrées, et lancent si elles échouent, mais dans cet exemple, la combinaison de la distribution avec la production du résultat est auto-documentée et compacte.

Si vous êtes sur .net 4.5, il existe des moyens pour que le compilateur insère le nom de la méthode / du fichier actuel comme paramètre de méthode (voir CallerMemberAttibute ). Mais pour un SDK, vous ne pouvez probablement pas obliger vos clients à passer à 4.5.

jdv-Jan de Vaan
la source
Il s'agit davantage de lancer des exceptions en général (et de gérer les informations dans une exception par rapport à l'encombrement du code), pas de cet exemple spécifique de casting.
FriendlyGuy
0

Ce que nous aimons faire pour les erreurs de logique métier (pas nécessairement les erreurs d'argument, etc.) est d'avoir une seule énumération qui définit tous les types d'erreurs potentiels:

/// <summary>
/// This enum is used to identify each business rule uniquely.
/// </summary>
public enum BusinessRuleId {

    /// <summary>
    /// Indicates that a valid body weight value of a patient is missing for dose calculation.
    /// </summary>
    [Display(Name = @"DoseCalculation_PatientBodyWeightMissing")]
    PatientBodyWeightMissingForDoseCalculation = 1,

    /// <summary>
    /// Indicates that a valid body height value of a patient is missing for dose calculation.
    /// </summary>
    [Display(Name = @"DoseCalculation_PatientBodyHeightMissing")]
    PatientBodyHeightMissingForDoseCalculation = 2,

    // ...
}

Les [Display(Name = "...")]attributs définissent la clé dans les fichiers de ressources à utiliser pour traduire les messages d'erreur.

En outre, ce fichier peut être utilisé comme point de départ pour rechercher toutes les occurrences où un certain type d'erreur est généré dans votre code.

La vérification des règles métier peut être déléguée à des classes Validator spécialisées qui produisent des listes de règles métier violées.

Nous utilisons ensuite un type d'exception personnalisé pour transporter les règles violées:

[Serializable]
public class BusinessLogicException : Exception {

    /// <summary>
    /// The Business Rule that was violated.
    /// </summary>
    public BusinessRuleId ViolatedBusinessRule { get; set; }

    /// <summary>
    /// Optional: additional parameters to be used to during generation of the error message.
    /// </summary>
    public string[] MessageParameters { get; set; }

    /// <summary>
    /// This exception indicates that a Business Rule has been violated. 
    /// </summary>
    public BusinessLogicException(BusinessRuleId violatedBusinessRule, params string[] messageParameters) {
        ViolatedBusinessRule = violatedBusinessRule;
        MessageParameters = messageParameters;
    }
}

Les appels de service backend sont enveloppés dans un code générique de gestion des erreurs, qui traduit la règle Busines violée en un message d'erreur lisible par l'utilisateur:

public object TryExecuteServiceAction(Action a) {
    try {
        return a();
    }
    catch (BusinessLogicException bex) {
        _logger.Error(GenerateErrorMessage(bex));
    }
}

public string GenerateErrorMessage(BusinessLogicException bex) {
    var translatedError = bex.ViolatedBusinessRule.ToTranslatedString();
    if (bex.MessageParameters != null) {
        translatedError = string.Format(translatedError, bex.MessageParameters);
    }
    return translatedError;
}

Voici ToTranslatedString()une méthode d'extension permettant de enumlire les clés de ressource à partir des [Display]attributs et d'utiliser le ResourceManagerpour traduire ces clés. La valeur de la clé de ressource respective peut contenir des espaces réservés pour string.Format, qui correspondent à ceux fournis MessageParameters. Exemple d'une entrée dans le fichier resx:

<data name="DoseCalculation_PatientBodyWeightMissing" xml:space="preserve">
    <value>The dose can not be calculated because the body weight observation for patient {0} is missing or not up to date.</value>
    <comment>{0} ... Patient name</comment>
</data>

Exemple d'utilisation:

throw new BusinessLogicException(BusinessRuleId.PatientBodyWeightMissingForDoseCalculation, patient.Name);

Avec cette approche, vous pouvez dissocier la génération du message d'erreur de la génération de l'erreur, sans avoir à introduire une nouvelle classe d'exception pour chaque nouveau type d'erreur. Utile si différentes interfaces doivent afficher des messages différents, si le message affiché doit dépendre de la langue et / ou du rôle de l'utilisateur, etc.

Georg Patscheider
la source
0

J'utiliserais des clauses de garde comme dans cet exemple pour que les vérifications de paramètres puissent être réutilisées.

De plus, vous pouvez utiliser le modèle de générateur pour que plus d'une validation puisse être chaînée. Cela ressemblera un peu à des assertions fluides .

Martin K
la source