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?

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.

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.

À 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.

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.

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.

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.