Si le modèle valide les données, ne devrait-il pas lever d'exceptions en cas de mauvaise entrée?

9

En lisant cette question SO, il semble que lever des exceptions pour valider les entrées utilisateur est mal vu.

Mais qui devrait valider ces données? Dans mes applications, toutes les validations sont effectuées dans la couche métier, car seule la classe elle-même sait vraiment quelles valeurs sont valides pour chacune de ses propriétés. Si je copiais les règles de validation d'une propriété sur le contrôleur, il est possible que les règles de validation changent et maintenant il y a deux endroits où la modification doit être faite.

Est-ce que ma prémisse que la validation doit être effectuée sur la couche métier est incorrecte?

Ce que je fais

Donc mon code se termine généralement comme ceci:

<?php
class Person
{
  private $name;
  private $age;

  public function setName($n) {
    $n = trim($n);
    if (mb_strlen($n) == 0) {
      throw new ValidationException("Name cannot be empty");
    }
    $this->name = $n;
  }

  public function setAge($a) {
    if (!is_int($a)) {
      if (!ctype_digit(trim($a))) {
        throw new ValidationException("Age $a is not valid");
      }
      $a = (int)$a;
    }
    if ($a < 0 || $a > 150) {
      throw new ValidationException("Age $a is out of bounds");
    }
    $this->age = $a;
  }

  // other getters, setters and methods
}

Dans le contrôleur, je passe simplement les données d'entrée au modèle et intercepte les exceptions levées pour montrer les erreurs à l'utilisateur:

<?php
$person = new Person();
$errors = array();

// global try for all exceptions other than ValidationException
try {

  // validation and process (if everything ok)
  try {
    $person->setAge($_POST['age']);
  } catch (ValidationException $e) {
    $errors['age'] = $e->getMessage();
  }

  try {
    $person->setName($_POST['name']);
  } catch (ValidationException $e) {
    $errors['name'] = $e->getMessage();
  }

  ...
} catch (Exception $e) {
  // log the error, send 500 internal server error to the client
  // and finish the request
}

if (count($errors) == 0) {
  // process
} else {
  showErrorsToUser($errors);
}

Est-ce une mauvaise méthodologie?

Méthode alternative

Dois-je peut-être créer des méthodes pour isValidAge($a)ce retour true / false puis les appeler à partir du contrôleur?

<?php
class Person
{
  private $name;
  private $age;

  public function setName($n) {
    $n = trim($n);
    if ($this->isValidName($n)) {
      $this->name = $n;
    } else {
      throw new Exception("Invalid name");
    }
  }

  public function setAge($a) {
    if ($this->isValidAge($a)) {
      $this->age = $a;
    } else {
      throw new Exception("Invalid age");
    }
  }

  public function isValidName($n) {
    $n = trim($n);
    if (mb_strlen($n) == 0) {
      return false;
    }
    return true;
  }

  public function isValidAge($a) {
    if (!is_int($a)) {
      if (!ctype_digit(trim($a))) {
        return false;
      }
      $a = (int)$a;
    }
    if ($a < 0 || $a > 150) {
      return false;
    }
    return true;
  }

  // other getters, setters and methods
}

Et le contrôleur sera fondamentalement le même, juste au lieu d'essayer / attraper il y a maintenant si / sinon:

<?php
$person = new Person();
$errors = array();
if ($person->isValidAge($age)) {
  $person->setAge($age);
} catch (Exception $e) {
  $errors['age'] = "Invalid age";
}

if ($person->isValidName($name)) {
  $person->setName($name);
} catch (Exception $e) {
  $errors['name'] = "Invalid name";
}

...

if (count($errors) == 0) {
  // process
} else {
  showErrorsToUser($errors);
}

Donc qu'est ce que je devrais faire?

Je suis assez content de ma méthode originale, et mes collègues à qui je l'ai montrée en général l'ont aimé. Malgré cela, dois-je passer à la méthode alternative? Ou est-ce que je fais ça terriblement mal et je devrais chercher une autre façon?

Carlos Campderrós
la source
J'ai un peu modifié le code "original" pour le gérer ValidationExceptionet d'autres exceptions
Carlos Campderrós
2
Un problème avec l'affichage des messages d'exception à l'utilisateur final est que le modèle a soudainement besoin de savoir quelle langue l'utilisateur parle, mais c'est surtout une préoccupation pour la vue.
Bart van Ingen Schenau
@BartvanIngenSchenau bonne capture. Mes applications ont toujours été en une seule langue, mais il est bon de penser aux problèmes de localisation qui peuvent survenir dans n'importe quelle implémentation.
Carlos Campderrós
Les exceptions pour les validations ne sont qu'un moyen sophistiqué d'injecter des types dans le processus. Vous pouvez obtenir les mêmes résultats en renvoyant un objet qui implémente une interface de validation comme IValidateResults.
Reactgular

Réponses:

7

L'approche que j'ai utilisée dans le passé est de mettre toutes les classes de validation dédiées à la logique de validation.

Vous pouvez ensuite injecter ces classes de validation dans votre couche de présentation pour une validation d'entrée précoce. Et rien n'empêche vos classes Model d'utiliser les mêmes classes pour appliquer Data Integrity.

En suivant cette approche, vous pouvez ensuite traiter les erreurs de validation différemment selon la couche dans laquelle elles se produisent:

  • Si la validation de l'intégrité des données échoue dans le modèle, lancez une exception.
  • Si la validation des entrées utilisateur échoue dans la couche de présentation, affichez une astuce utile et retardez la transmission de la valeur à votre modèle.
MetaFight
la source
Vous avez donc une classe PersonValidatoravec toute la logique pour valider les différents attributs de a Person, et la Personclasse qui en dépend PersonValidator, non? Quel est l'avantage de votre proposition par rapport à la méthode alternative que j'ai suggérée dans la question? Je ne vois que la capacité d'injecter différentes classes de validation pour un Person, mais je ne pense à aucun cas où cela serait nécessaire.
Carlos Campderrós
Je suis d'accord que l'ajout d'une toute nouvelle classe pour la validation est exagéré, au moins dans ce cas relativement simple. Cela pourrait être utile pour un problème beaucoup plus complexe.
Eh bien, pour une application que vous envisagez de vendre à plusieurs personnes / sociétés, cela peut avoir du sens, car chaque société peut avoir des règles différentes pour valider ce qui est une plage valide pour l'âge d'une personne. Il est donc possible utile, mais vraiment exagéré pour mes besoins. Quoi qu'il en soit, +1 pour vous aussi
Carlos Campderrós
1
Séparer la validation du modèle est également logique du point de vue du couplage et de la cohésion. Dans ce scénario simple, il peut être exagéré, mais il ne faudra qu'une seule règle de validation «champ croisé» pour rendre la classe de validateur séparée beaucoup plus attrayante.
Seth M.
8

Je suis assez content de ma méthode originale, et mes collègues à qui je l'ai montrée en général l'ont aimé. Malgré cela, dois-je passer à la méthode alternative? Ou est-ce que je fais ça terriblement mal et je devrais chercher une autre façon?

Si vous et vos collègues en êtes satisfait, je ne vois aucun besoin urgent de changer.

La seule chose qui est discutable d'un point de vue pragmatique est que vous lancez Exceptionplutôt que quelque chose de plus spécifique. Le problème est que si vous interceptez Exception, vous pouvez finir par intercepter des exceptions qui n'ont rien à voir avec la validation de l'entrée utilisateur.


Maintenant, il y a beaucoup de gens qui disent des choses comme "les exceptions ne devraient être utilisées que pour des choses exceptionnelles, et XYZ n'est pas exceptionnel". (Par exemple, la réponse de @ dann1111 ... où il qualifie les erreurs utilisateur de "parfaitement normales".)

Ma réponse à cela est qu'il n'y a pas de critère objectif pour décider si quelque chose ("XY Z") est exceptionnel ou non. Il s'agit d'une mesure subjective . (Le fait que tout programme doive vérifier les erreurs dans les entrées utilisateur ne rend pas les erreurs d'occurrence "normales". En fait, "normales" n'a pas beaucoup de sens d'un point de vue objectif.)

Il y a un grain de vérité dans ce mantra. Dans certains langages (ou plus précisément, certaines implémentations de langage ), la création, le lancement et / ou la capture d'exceptions sont beaucoup plus chers que les simples conditionnels. Mais si vous le regardez de ce point de vue, vous devez comparer le coût de création / lancement / capture avec le coût des tests supplémentaires que vous pourriez avoir besoin d'effectuer si vous évitiez d'utiliser des exceptions. Et «l'équation» doit tenir compte de la probabilité que l'exception doive être levée.

L'autre argument contre les exceptions est qu'elles peuvent rendre le code plus difficile à comprendre. Mais le revers de la médaille est que lorsqu'ils sont utilisés de manière appropriée, ils peuvent rendre le code plus facile à comprendre.


En bref - la décision d'utiliser ou de ne pas utiliser d'exceptions devrait être prise après avoir évalué les mérites ... et NON sur la base d'un dogme simpliste.

Stephen C
la source
Bon point sur le générique Exceptionlancé / capturé. Je lève vraiment une sous-classe de Exception, et le code des setters ne fait généralement rien qui puisse lever une autre exception.
Carlos Campderrós
J'ai un peu modifié le code "original" pour gérer ValidationException et d'autres exceptions / cc @ dan1111
Carlos Campderrós
1
+1, je préfère de loin avoir une ValidationException descriptive plutôt que de revenir à l'âge sombre de devoir vérifier la valeur de retour de chaque appel de méthode. Code plus simple = potentiellement moins d'erreurs.
Heinzi
2
@ dan1111 - Bien que je respecte votre droit d'avoir une opinion, rien dans votre commentaire n'est autre chose qu'une opinion. Il n'y a pas de lien logique entre la «normalité» de la validation et le mécanisme de gestion des erreurs de validation. Tout ce que vous faites, c'est réciter le dogme.
Stephen C
@StephenC, après réflexion, je sens que j'ai énoncé mon cas trop fortement. Je suis d'accord que c'est plus une préférence personnelle.
6

À mon avis, il est utile de faire la distinction entre les erreurs d'application et les erreurs utilisateur , et de n'utiliser des exceptions que pour les premières.

  • Les exceptions sont destinées à couvrir les éléments qui empêchent votre programme de s'exécuter correctement .

    Ce sont des événements inattendus qui vous empêchent de continuer, et leur conception reflète cela: ils interrompent l'exécution normale et sautent à un endroit qui permet la gestion des erreurs.

  • Les erreurs utilisateur telles qu'une entrée invalide sont parfaitement normales (du point de vue de votre programme) et ne doivent pas être traitées comme inattendues par votre application .

    Si l'utilisateur entre une valeur erronée et que vous affichez un message d'erreur, votre programme a-t-il "échoué" ou a-t-il eu une erreur de quelque façon? Non. Votre candidature a été acceptée - compte tenu d'un certain type de saisie, elle a produit la sortie correcte dans cette situation.

    La gestion des erreurs utilisateur, car elle fait partie de l'exécution normale, devrait faire partie de votre flux de programme normal, plutôt que d'être gérée en sautant avec une exception.

Bien sûr, il est possible d'utiliser des exceptions à des fins autres que celles pour lesquelles elles sont prévues, mais cela crée une confusion dans le paradigme et risque un comportement incorrect lorsque ces erreurs se produisent.

Votre code d'origine est problématique:

  • L'appelant de la setAge()méthode doit en savoir trop sur la gestion des erreurs internes de la méthode: l'appelant doit savoir qu'une exception est levée lorsque l'âge n'est pas valide et qu'aucune autre exception ne peut être levée dans la méthode . Cette hypothèse pourrait être brisée plus tard si vous ajoutiez des fonctionnalités supplémentaires à l'intérieur setAge().
  • Si l'appelant ne détecte pas d'exceptions, l'exception d'âge non valide sera plus tard gérée d'une autre manière, très probablement opaque. Ou même provoquer un crash d'exception non géré. Pas de bon comportement pour les données non valides entrées.

Le code alternatif a également des problèmes:

  • Une méthode supplémentaire, éventuellement inutile, isValidAge()a été introduite.
  • Maintenant, la setAge()méthode doit supposer que l'appelant a déjà vérifié isValidAge()(une hypothèse terrible) ou valider à nouveau l'âge. S'il valide à nouveau l'âge, setAge() il doit toujours fournir une sorte de gestion des erreurs, et vous êtes de retour à la case départ.

Conception suggérée

  • Rendre setAge()vrai en cas de succès et faux en cas d'échec.

  • Vérifiez la valeur de retour de setAge()et si elle a échoué, informez l'utilisateur que l'âge n'était pas valide, non pas avec une exception, mais avec une fonction normale qui affiche une erreur à l'utilisateur.


la source
Alors comment dois-je le faire? Avec la méthode alternative que j'ai proposée ou avec quelque chose de totalement différent auquel je n'ai pas pensé? En outre, ma prémisse selon laquelle "la validation doit être effectuée sur la couche métier" est-elle fausse?
Carlos Campderrós
@ CarlosCampderrós, voir la mise à jour; J'ajoutais cette information comme vous l'avez dit. Votre conception d'origine avait une validation au bon endroit, mais c'était une erreur d'utiliser des exceptions pour effectuer cette validation.
La méthode alternative oblige le setAgeà valider à nouveau, mais comme la logique est fondamentalement "si elle est valide, alors définissez l'exception de levée age else", cela ne me ramène pas à la case départ.
Carlos Campderrós
2
Un problème que je vois à la fois avec la méthode alternative et la conception suggérée est qu'ils perdent la capacité de distinguer POURQUOI l'âge n'était pas valide. Il pourrait être fait pour renvoyer true ou une chaîne d'erreur (ouais, php est tellement sale), mais cela pourrait entraîner beaucoup de problèmes, car "The entered age is out of bounds" == trueet les gens devraient toujours utiliser ===, donc cette approche serait plus problématique que le problème qu'elle essaie de résoudre
Carlos Campderrós
2
Mais le codage de l'application est vraiment fastidieux car pour tout ce setAge()que vous faites n'importe où, vous devez vérifier que cela a vraiment fonctionné. Lancer des exceptions signifie que vous ne devriez pas vous soucier de vous souvenir de vérifier que tout s'est bien passé. Comme je le vois, essayer de définir une valeur non valide dans un attribut / propriété est quelque chose d'exceptionnel, et cela vaut la peine de lancer le Exception. Le modèle ne devrait pas se soucier s'il obtient son entrée de la base de données ou de l'utilisateur. Il ne devrait jamais recevoir de mauvaise entrée, donc je vois qu'il est légitime de lancer une exception là-bas.
Carlos Campderrós
4

De mon point de vue (je suis un gars Java), il est tout à fait valable comment vous l'avez implémenté de la première façon.

Il est valide qu'un objet lève une exception lorsque certaines conditions préalables ne sont pas remplies (par exemple, chaîne vide). En Java, le concept d'exceptions vérifiées est destiné à un tel but - des exceptions qui doivent être déclarées dans la signature pour être proprement levées, et l'appelant doit explicitement les attraper. En revanche, des exceptions non contrôlées (alias RuntimeExceptions) peuvent survenir à tout moment sans qu'il soit nécessaire de définir une clause catch dans le code. Alors que les premiers sont utilisés pour les cas récupérables (par exemple, entrée utilisateur incorrecte, le nom de fichier n'existe pas), les derniers sont utilisés pour les cas où l'utilisateur / programmeur ne peut rien faire (par exemple, Out-Of-Memory).

Cependant, comme déjà mentionné par @Stephen C, vous devez définir vos propres exceptions et intercepter spécifiquement celles pour ne pas en attraper d'autres involontairement.

Une autre façon, cependant, serait d'utiliser des objets de transfert de données qui sont simplement des conteneurs de données sans aucune logique. Vous transférez ensuite un tel DTO à un validateur ou à l'objet-modèle lui-même pour validation, et uniquement en cas de réussite, effectuez les mises à jour dans l'objet-modèle. Cette approche est souvent utilisée lorsque la logique de présentation et la logique d'application sont des niveaux séparés (la présentation est une page Web, l'application un service Web). De cette façon, ils sont physiquement séparés, mais si vous avez les deux sur un seul niveau (comme dans votre exemple), vous devez vous assurer qu'il n'y aura pas de solution pour définir une valeur sans validation.

Andy
la source
4

Avec mon chapeau Haskell, les deux approches sont fausses.

Ce qui se passe conceptuellement, c'est que vous avez d'abord un tas d'octets, et après avoir analysé et validé, vous pouvez ensuite construire une personne.

La personne a certains invariants, comme la précision d'un nom et d'un âge.

Être capable de représenter une personne qui n'a qu'un nom, mais sans âge est quelque chose que vous voulez éviter à tout prix, car c'est ce qui crée la complétude. Des invariants stricts signifient que vous n'avez pas besoin de vérifier la présence d'un âge plus tard par exemple.

Donc, dans mon monde, Person est créé atomiquement en utilisant un seul constructeur ou une seule fonction. Ce constructeur ou cette fonction peut à nouveau vérifier la validité des paramètres, mais aucune demi-personne ne doit être construite.

Malheureusement, Java, PHP et d'autres langages OO rendent l'option correcte assez verbeuse. Dans les API Java appropriées, les objets de générateur sont souvent utilisés. Dans une telle API, la création d'une personne ressemblerait à ceci:

Person p = new Person.Builder().setName(name).setAge(age).build();

ou le plus verbeux:

Person.Builder builder = new Person.Builder();
builder.setName(name);
builder.setAge(age);
Person p = builder.build();
// Person object must have name and age here

Dans ces cas, peu importe où des exceptions sont levées ou où la validation se produit, il est impossible de recevoir une instance Person qui n'est pas valide.

user239558
la source
Tout ce que vous avez fait ici est de déplacer le problème vers la classe Builder, à laquelle vous n'avez pas vraiment répondu.
Cypher
2
J'ai localisé le problème dans la fonction builder.build () qui est exécutée atomiquement. Cette fonction est une liste de toutes les étapes de vérification. Il existe une énorme différence entre cette approche et les approches ad hoc. La classe Builder n'a pas d'invariants au-delà des types simples, tandis que la classe Person a des invariants forts. La création de programmes corrects consiste à appliquer des invariants puissants dans vos données.
user239558
Il ne répond toujours pas à la question (du moins pas complètement). Pourriez-vous expliquer comment les messages d'erreur individuels sont transmis de la classe Builder jusqu'à la pile des appels vers la vue?
Cypher
Trois possibilités: build () peut lever des exceptions spécifiques, comme dans le premier exemple de l'OP. Il peut y avoir un ensemble public <String> validate () qui renvoie un ensemble d'erreurs lisibles par l'homme. Il doit y avoir un ensemble public <Error> validate () pour les erreurs prêtes pour i18n. Le fait est que cela se produit lors de la conversion en objet Person.
user239558
2

Dans les mots du profane:

La première approche est la bonne.

La deuxième approche suppose que ces classes métier ne seront appelées que par ces contrôleurs et qu'elles ne seront jamais appelées à partir d'un autre contexte.

Les classes commerciales doivent lever une exception chaque fois qu'une règle commerciale est violée.

Le contrôleur ou la couche de présentation doit décider s'il les lance ou s'il effectue ses propres validations pour empêcher les exceptions de se produire.

N'oubliez pas: vos classes seront potentiellement utilisées dans différents contextes et par différents intégrateurs. Ils doivent donc être suffisamment intelligents pour lever des exceptions aux mauvaises entrées.

Tulains Córdova
la source