Odeur de code de logique de validation commerciale

Question

Considérez le code suivant:

partial class OurBusinessObject {
    partial void OnOurPropertyChanged() {
        if(ValidateOurProperty(this.OurProperty) == false) {
            this.OurProperty = OurBusinessObject.Default.OurProperty;
        }
    }
}

C'est-à-dire que lorsque la valeur de OurProperty dans OurBusinessObject est modifiée, si la valeur n'est pas valide, définissez-la comme valeur par défaut. Ce motif me semble odeur de code, mais d’autres ici (chez mon employeur) ne sont pas d’accord. Quelles sont vos pensées?

Modifié pour ajouter : on m'a demandé d'ajouter une explication des raisons pour lesquelles cela est considéré comme acceptable. L'idée était que, plutôt que de demander aux producteurs de l'objet métier de valider les données, celui-ci pouvait valider ses propres propriétés et définir des valeurs propres par défaut dans les cas où la validation échouait. En outre, si les règles de validation changeaient, les producteurs d'objet métier n'auraient pas à changer de logique car ce dernier se chargerait de la validation et du nettoyage des données.

Answer 1

C'est absolument horrible. Bonne chance pour essayer de déboguer des problèmes en production. La seule chose à laquelle cela peut conduire est de couvrir les bugs, qui apparaîtront simplement ailleurs, où leur origine ne sera pas évidente.

Answer 2

Je pense que je dois être d'accord avec vous. Cela pourrait certainement entraîner des problèmes où la logique reviendrait de manière inattendue aux valeurs par défaut, ce qui pourrait être très difficile à déboguer.

À tout le moins, ce comportement devrait être consigné, mais cela ressemble davantage à un cas de lancement d’une exception.

Answer 3

Pour moi, cela ressemble au symptôme plutôt qu'au problème réel. En réalité, le configurateur de OurProperty ne conserve pas la valeur d'origine à utiliser dans l'événement OnOurPropertyChanged . Si vous faites cela, il devient soudainement plus facile de faire de meilleurs choix quant à la manière de procéder.

D'ailleurs, ce que vous voulez vraiment, c'est un événement OnOurPropertyChanging qui est déclenché à partir du setter avant que ne se produise réellement. De cette façon, vous pouvez autoriser ou refuser la cession en premier lieu. Sinon, il y a peu de temps pendant lequel votre objet n'est pas valide, ce qui signifie que le type n'est pas thread-safe et que vous ne pouvez pas compter sur la cohérence si vous considérez que la concurrence d'accès est un problème.

Answer 4

Certainement une pratique discutable.

Comment une valeur non valide serait-elle affectée à cette propriété? Cela n'indiquerait-il pas qu'il y a un bogue quelque part dans le code d'appel, auquel cas vous voudrez probablement le savoir tout de suite? Ou qu'un utilisateur ait mal saisi quelque chose, auquel cas il devrait en être informé immédiatement?

En général, "échec rapide" facilite le traçage des bugs. Assigner silencieusement une valeur par défaut dans les coulisses s'apparente à "magique". et ne fera que causer de la confusion à quiconque doit maintenir la base de code.

Answer 5

Le goût pour le terme 'odeur de code' mis à part, vous avez peut-être raison. Selon son origine, changer silencieusement la valeur n'est probablement pas une bonne chose. Il serait préférable de vous assurer que votre valeur est valide au lieu de simplement revenir à la valeur par défaut.

Answer 6

Je recommande fortement de le refactoriser pour le valider avant de définir la propriété.

Vous pouvez toujours avoir une méthode qui ressemble plus à:

T GetValidValueForProperty<T>(T suggestedValue, T currentValue);

ou même:

T GetValidValueForProperty<T>(string propertyName, T suggestedValue, T currentValue);

Si vous le faites, avant de définir la propriété, vous pouvez la transmettre à la logique métier à valider. Cette dernière peut renvoyer la valeur de la propriété par défaut (votre comportement actuel) OU (plus raisonnable dans la plupart des cas), renvoyer currentValue, le paramétrage n'a donc eu aucun effet.

Ceci serait plutôt utilisé comme:

T OurProperty
{
    get
    {
        return this.propertyBackingField;
    }
    set
    {
        this.propertyBackingField = this.GetValidValueForProperty(value, this.propertyBackingField);
    }
}

Peu importe ce que vous faites, mais il est important de valider avant de modifier votre valeur actuelle. Si vous modifiez votre valeur avant de déterminer si la nouvelle valeur est bonne, vous vous inquiétez à long terme.

Answer 7

Ça peut ou peut ne pas "sentir", mais je penche plutôt vers "Oui, ça sent".

Définir OurProperty sur la valeur par défaut a-t-il une raison logique de le faire ou est-il simplement pratique de le faire dans le code? Il est possible, bien que cela soit peu probable en pratique, d’organiser un scénario dans lequel cela serait le comportement attendu, mais je suppose que dans la plupart des cas, vous devriez lancer une exception et la gérer proprement quelque part.

La définition de la valeur par défaut vous rapproche-t-elle ou vous éloigne-t-elle de la description des spécifications fonctionnelles de la manière dont l'application est supposée fonctionner?

Answer 8

Vous validez une modification après l'avoir effectuée? La validation doit être effectuée avant que la propriété business ne soit modifiée.

Réponse à vos questions: la solution présentée dans cet extrait de code peut générer de gros problèmes en production, vous ne savez pas si la valeur par défaut y est apparue en raison d'une entrée non valide ou simplement parce qu'un autre paramètre a défini la valeur par défaut

Answer 9

Il est difficile de dire sans connaître le contexte ou les règles de gestion. En règle générale, vous devez simplement valider au moment de la saisie, et peut-être une fois de plus avant la persistance, mais votre façon de le faire ne vous permettra pas vraiment de valider car vous ne permettez pas à une propriété de contenir une valeur non valide.

Answer 10

Je pense que votre logique de validation devrait générer une exception si vous êtes invité à utiliser une valeur non valide. Si votre consommateur souhaite utiliser une valeur par défaut, il doit le demander explicitement, soit via une valeur documentée spéciale, soit via une autre méthode.

Le seul type d'exception que je puisse penser qui serait pardonnable serait, par exemple, la normalisation de cas, comme dans les champs de courrier électronique pour détecter les doublons.

Answer 11

De plus, pourquoi dans le monde est-ce partiel? Utiliser des classes partielles pour tout ce qui n’est pas un framework de code généré est en soi un code, car vous les utilisez probablement pour cacher la complexité, ce qui devrait être divisé de toute façon!

Answer 12

Je suis d'accord avec Grzenio et ajouterais que le meilleur moyen de gérer une erreur de validation dans la couche de domaine (ou objets métier) est de générer une exception. Cette exception pourrait se propager jusque dans la couche d'interface utilisateur, où elle pourrait être gérée et rectifiée de manière interactive avec l'utilisateur. Toutefois, en fonction des capacités et des technologies impliquées, cela risque de ne pas toujours être réalisable. Dans ce cas, vous devriez probablement valider dans la couche UI (éventuellement en plus de la couche Domaine). C'est moins qu'idéal, mais pourrait être votre seule option viable. Dans tous les cas, le paramétrer sur une valeur par défaut est une chose horrible à faire et conduira à des bugs subtils qu'il sera presque impossible de diagnostiquer. Si cela est fait à grande échelle, vous aurez un système impossible à maintenir en un rien de temps (surtout si vous n'avez pas de tests unitaires vous sauvegardant).

Answer 13

Un argument que j’ai contre ceci est le suivant. Supposons que l'utilisateur / producteur de l'objet métier saisisse accidentellement une valeur non valide. Ensuite, ce modèle dissimulera ce fait et utilisera par défaut les données propres. Mais la bonne façon de gérer cela consiste à envoyer une erreur et à demander à l’utilisateur / producteur de vérifier / nettoyer leurs données d’entrée.

Answer 14

Je dirais que vous devez implémenter PropertyChanging et permettre à la logique métier d'approuver / refuser une valeur, puis de lever une exception pour les valeurs non valides.

De cette façon, vous n’avez jamais de valeur invalide. Cela, et vous ne devriez jamais changer les informations d'un utilisateur. Que se passe-t-il si un utilisateur ajoute une entrée à la base de données et en garde la trace pour ses propres enregistrements? Votre code réaffecterait la valeur à la valeur par défaut et il suivra maintenant les informations erronées. Il vaut mieux informer l'utilisateur dès que possible.