Wenn das Modell die Daten validiert, sollte es dann keine Ausnahmen bei fehlerhaften Eingaben auslösen?

9

Beim Lesen dieser SO-Frage scheint es verpönt zu sein, Ausnahmen für die Überprüfung von Benutzereingaben auszulösen.

Aber wer sollte diese Daten validieren? In meinen Anwendungen werden alle Überprüfungen in der Geschäftsschicht durchgeführt, da nur die Klasse selbst wirklich weiß, welche Werte für jede ihrer Eigenschaften gültig sind. Wenn ich die Regeln für die Validierung einer Eigenschaft in den Controller kopieren würde, könnten sich die Validierungsregeln ändern, und jetzt gibt es zwei Stellen, an denen die Änderung vorgenommen werden sollte.

Ist meine Prämisse, dass die Validierung auf der Business-Ebene durchgeführt werden sollte, falsch?

Was ich mache

Mein Code endet normalerweise so:

<?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
}

In der Steuerung übergebe ich einfach die Eingabedaten an das Modell und fange ausgelöste Ausnahmen ab, um dem Benutzer die Fehler anzuzeigen:

<?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);
}

Ist das eine schlechte Methode?

Alternative Methode

Sollte ich vielleicht Methoden für isValidAge($a)diese Rückgabe true / false erstellen und sie dann vom Controller aufrufen?

<?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
}

Und der Controller wird im Grunde der gleiche sein, nur anstatt zu versuchen / fangen gibt es jetzt if / else:

<?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);
}

Also was soll ich tun?

Ich bin ziemlich zufrieden mit meiner ursprünglichen Methode, und meine Kollegen, denen ich sie im Allgemeinen gezeigt habe, haben sie gemocht. Sollte ich trotzdem auf die alternative Methode umsteigen? Oder mache ich das furchtbar falsch und sollte nach einem anderen Weg suchen?

Carlos Campderrós
quelle
Ich habe den "ursprünglichen" Code ein wenig geändert, um damit ValidationException
umzugehen,
2
Ein Problem beim Anzeigen von Ausnahmemeldungen für den Endbenutzer besteht darin, dass das Modell plötzlich wissen muss, welche Sprache der Benutzer spricht. Dies ist jedoch hauptsächlich ein Problem für die Ansicht.
Bart van Ingen Schenau
@ BartvanIngenSchenau guter Fang. Meine Anwendungen waren immer einsprachig, aber es ist gut, an Lokalisierungsprobleme zu denken, die bei jeder Implementierung auftreten können.
Carlos Campderrós
Ausnahmen für Validierungen sind nur eine ausgefallene Methode, um Typen in den Prozess einzufügen. Sie können dieselben Ergebnisse erzielen, indem Sie ein Objekt zurückgeben, das eine Validierungsschnittstelle wie implementiert IValidateResults.
Reactgular

Antworten:

7

Der Ansatz, den ich in der Vergangenheit verwendet habe, besteht darin, alle Validierungsklassen dedizierten Validierungsklassen zuzuordnen.

Sie können diese Validierungsklassen dann zur frühen Eingabevalidierung in Ihre Präsentationsschicht einfügen. Und nichts hindert Ihre Modellklassen daran, dieselben Klassen zu verwenden, um die Datenintegrität zu erzwingen.

Nach diesem Ansatz können Sie Validierungsfehler je nach Ebene, in der sie auftreten, unterschiedlich behandeln:

  • Wenn die Überprüfung der Datenintegrität im Modell fehlschlägt, lösen Sie eine Ausnahme aus.
  • Wenn die Überprüfung der Benutzereingaben in der Präsentationsebene fehlschlägt, zeigen Sie einen hilfreichen Tipp an und verzögern Sie die Übertragung des Werts auf Ihr Modell.
MetaFight
quelle
Sie haben also eine Klasse PersonValidatormit der ganzen Logik, um die verschiedenen Attribute von a zu validieren Person, und die PersonKlasse, die davon abhängt PersonValidator, richtig? Welchen Vorteil bietet Ihr Vorschlag gegenüber der alternativen Methode, die ich in der Frage vorgeschlagen habe? Ich sehe nur die Fähigkeit, verschiedene Validierungsklassen für a einzufügen Person, aber ich kann mir keinen Fall vorstellen, in dem dies erforderlich wäre.
Carlos Campderrós
Ich bin damit einverstanden, dass das Hinzufügen einer völlig neuen Klasse zur Validierung zumindest in diesem relativ einfachen Fall übertrieben ist. Es könnte für ein Problem mit viel mehr Komplexität nützlich sein.
Nun, für eine Anwendung, die Sie an mehrere Personen / Unternehmen verkaufen möchten, ist es möglicherweise sinnvoll, sie zu haben, da jedes Unternehmen möglicherweise andere Regeln für die Überprüfung eines gültigen Bereichs für das Alter einer Person hat. So ist es möglich nützlich, aber wirklich übertrieben für meine Bedürfnisse. Wie auch immer, +1 auch für Sie
Carlos Campderrós
1
Die Trennung der Validierung vom Modell ist auch unter dem Gesichtspunkt der Kopplung und des Zusammenhalts sinnvoll. In diesem einfachen Szenario ist es möglicherweise übertrieben, aber es wird nur eine einzige "feldübergreifende" Validierungsregel benötigt, um die separate Validator-Klasse attraktiver zu machen.
Seth M.
8

Ich bin ziemlich zufrieden mit meiner ursprünglichen Methode, und meine Kollegen, denen ich sie im Allgemeinen gezeigt habe, haben sie gemocht. Sollte ich trotzdem auf die alternative Methode umsteigen? Oder mache ich das furchtbar falsch und sollte nach einem anderen Weg suchen?

Wenn Sie und Ihre Kollegen damit zufrieden sind, sehe ich keinen dringenden Änderungsbedarf.

Das einzige, was aus pragmatischer Sicht fraglich ist, ist, dass Sie eher werfen Exceptionals etwas Spezifischeres. Das Problem ist, dass Sie beim Abfangen Exceptionmöglicherweise Ausnahmen abfangen , die nichts mit der Validierung von Benutzereingaben zu tun haben.


Jetzt gibt es viele Leute, die Dinge sagen wie "Ausnahmen sollten nur für außergewöhnliche Dinge verwendet werden, und XYZ ist nicht außergewöhnlich". (Zum Beispiel die Antwort von @ dann1111 ... wo er Benutzerfehler als "völlig normal" bezeichnet.)

Meine Antwort darauf ist, dass es kein objektives Kriterium für die Entscheidung gibt, ob etwas ("XY Z") außergewöhnlich ist oder nicht. Es ist eine subjektive Maßnahme. (Die Tatsache , dass einiger Programmbedarf für Fehler in Benutzereingaben überprüfen nicht den Auftritt Fehler „normal“ machen. In der Tat, „normal“ ist weitgehend sinnlos von einem objektiven Standpunkt aus .)

In diesem Mantra steckt ein Körnchen Wahrheit. In einigen Sprachen (oder genauer gesagt in einigen Sprachimplementierungen ) ist das Erstellen, Werfen und / oder Abfangen von Ausnahmen erheblich teurer als einfache Bedingungen. Wenn Sie es jedoch aus dieser Perspektive betrachten, müssen Sie die Kosten für das Erstellen / Werfen / Fangen mit den Kosten der zusätzlichen Tests vergleichen, die Sie möglicherweise durchführen müssen, wenn Sie die Verwendung von Ausnahmen vermeiden. Und die "Gleichung" muss die Wahrscheinlichkeit berücksichtigen, dass die Ausnahme ausgelöst werden muss.

Das andere Argument gegen Ausnahmen ist, dass sie das Verständnis von Code erschweren können . Aber die Kehrseite ist , dass , wenn sie in geeigneter Weise verwendet werden, können sie Code machen leichter zu verstehen.


Kurz gesagt - die Entscheidung, Ausnahmen zu verwenden oder nicht zu verwenden, sollte nach Abwägung der Verdienste getroffen werden ... und NICHT auf der Grundlage eines simplen Dogmas.

Stephen C.
quelle
Guter Punkt Exception, wenn das Generikum geworfen / gefangen wird. Ich werfe wirklich eine eigene Unterklasse von Exception, und der Code der Setter macht normalerweise nichts, was eine weitere Ausnahme auslösen könnte.
Carlos Campderrós
Ich habe den "ursprünglichen" Code ein wenig geändert, um ValidationException und andere Ausnahmen zu behandeln / cc @ dan1111
Carlos Campderrós
1
+1, ich hätte viel lieber eine beschreibende ValidationException, als in die dunklen Zeiten zurückzukehren, in denen der Rückgabewert jedes Methodenaufrufs überprüft werden musste. Einfacherer Code = möglicherweise weniger Fehler.
Heinzi
2
@ dan1111 - Obwohl ich Ihr Recht auf eine Meinung respektiere, ist nichts in Ihrem Kommentar etwas anderes als eine Meinung. Es gibt keinen logischen Zusammenhang zwischen der "Normalität" der Validierung und dem Mechanismus zur Behandlung von Validierungsfehlern. Alles, was Sie tun, ist das Dogma zu rezitieren.
Stephen C
@StephenC, nach Überlegung habe ich das Gefühl, dass ich meinen Fall zu stark dargelegt habe. Ich stimme zu, dass es eher eine persönliche Präferenz ist.
6

Meiner Meinung nach ist es nützlich, zwischen Anwendungsfehlern und Benutzerfehlern zu unterscheiden und nur Ausnahmen für die ersteren zu verwenden.

  • Ausnahmen sollen Dinge abdecken, die verhindern, dass Ihr Programm ordnungsgemäß ausgeführt wird .

    Es handelt sich um unerwartete Ereignisse, die Sie daran hindern, fortzufahren, und ihr Design spiegelt dies wider: Sie unterbrechen die normale Ausführung und springen an einen Ort, an dem Fehler behandelt werden können.

  • Benutzerfehler wie ungültige Eingaben sind (aus Sicht Ihres Programms) völlig normal und sollten von Ihrer Anwendung nicht als unerwartet behandelt werden .

    Wenn der Benutzer den falschen Wert eingibt und Sie eine Fehlermeldung anzeigen, ist Ihr Programm "fehlgeschlagen" oder ist in irgendeiner Weise ein Fehler aufgetreten? Nein. Ihre Bewerbung war erfolgreich. Bei einer bestimmten Art von Eingabe wurde in dieser Situation die richtige Ausgabe erstellt.

    Die Behandlung von Benutzerfehlern sollte, da sie Teil der normalen Ausführung ist, Teil Ihres normalen Programmablaufs sein und nicht durch Herausspringen mit einer Ausnahme behandelt werden.

Natürlich ist es möglich, Ausnahmen für andere Zwecke als den beabsichtigten Zweck zu verwenden, aber dies verwirrt das Paradigma und riskiert falsches Verhalten, wenn diese Fehler auftreten.

Ihr ursprünglicher Code ist problematisch:

  • Der Aufrufer der setAge()Methode muss viel zu viel über die interne Fehlerbehandlung der Methode wissen: Der Aufrufer muss wissen, dass eine Ausnahme ausgelöst wird, wenn das Alter ungültig ist, und dass keine anderen Ausnahmen innerhalb der Methode ausgelöst werden können . Diese Annahme könnte später aufgehoben werden, wenn Sie zusätzliche Funktionen hinzufügen setAge().
  • Wenn der Anrufer keine Ausnahmen abfängt, wird die ungültige Altersausnahme später auf eine andere, höchstwahrscheinlich undurchsichtige Weise behandelt. Oder sogar einen unbehandelten Ausnahme-Absturz verursachen. Kein gutes Verhalten für die Eingabe ungültiger Daten.

Der alternative Code hat auch Probleme:

  • Eine zusätzliche, möglicherweise unnötige Methode isValidAge()wurde eingeführt.
  • Jetzt muss die setAge()Methode davon ausgehen, dass der Anrufer isValidAge()das Alter bereits überprüft hat (eine schreckliche Annahme) oder erneut validiert. Wenn das Alter erneut überprüft wird, muss setAge() immer noch eine Art Fehlerbehandlung bereitgestellt werden, und Sie sind wieder auf dem ersten Platz.

Vorgeschlagenes Design

  • Machen Sie die setAge()Rückkehr bei Erfolg wahr und bei Misserfolg falsch.

  • Überprüfen Sie den Rückgabewert von setAge()und informieren Sie den Benutzer, falls dies fehlgeschlagen ist, dass das Alter ungültig war, nicht mit einer Ausnahme, sondern mit einer normalen Funktion, die dem Benutzer einen Fehler anzeigt.


quelle
Wie soll ich es dann machen? Mit der von mir vorgeschlagenen alternativen Methode oder mit etwas völlig anderem, an das ich nicht gedacht habe? Ist meine Prämisse, dass "die Validierung auf der Business-Ebene erfolgen sollte", auch falsch?
Carlos Campderrós
@ CarlosCampderrós, siehe das Update; Ich habe diese Informationen hinzugefügt, als Sie kommentierten. Ihr ursprüngliches Design hatte die Validierung an der richtigen Stelle, aber es war ein Fehler, Ausnahmen zu verwenden, um diese Validierung durchzuführen.
Die alternative Methode setAgeerzwingt zwar eine erneute Validierung, aber da die Logik im Grunde lautet: "Wenn sie gültig ist, dann setze Alter, sonst Ausnahme auslösen", bringt sie mich nicht zurück zum ersten Punkt.
Carlos Campderrós
2
Ein Problem, das ich sowohl bei der alternativen Methode als auch beim vorgeschlagenen Design sehe, ist, dass sie nicht mehr unterscheiden können, WARUM das Alter ungültig war. Es könnte gemacht werden, um true oder eine Fehlerzeichenfolge zurückzugeben (ja, PHP ist soooo schmutzig), aber dies könnte zu vielen Problemen führen, weil "The entered age is out of bounds" == trueund die Leute immer verwenden sollten ===, so dass dieser Ansatz problematischer wäre als das Problem, das er versucht lösen
Carlos Campderrós
2
Aber dann ist das Codieren der Anwendung wirklich mühsam, denn für alles setAge(), was Sie irgendwo machen, müssen Sie überprüfen, ob es wirklich funktioniert hat. Das Auslösen von Ausnahmen bedeutet, dass Sie sich keine Gedanken darüber machen sollten, ob alles in Ordnung ist. Aus meiner Sicht ist der Versuch, einen ungültigen Wert in ein Attribut / eine Eigenschaft zu setzen, etwas Außergewöhnliches und es lohnt sich, das zu werfen Exception. Dem Modell sollte es egal sein, ob es seine Eingaben von der Datenbank oder vom Benutzer erhält. Es sollte niemals schlechte Eingaben erhalten, daher sehe ich es als legitim an, dort eine Ausnahme auszulösen.
Carlos Campderrós
4

Aus meiner Sicht (ich bin ein Java-Typ) ist es absolut gültig, wie Sie es auf die erste Weise implementiert haben.

Es ist gültig, dass ein Objekt eine Ausnahme auslöst, wenn einige Voraussetzungen nicht erfüllt sind (z. B. leere Zeichenfolge). In Java ist das Konzept der überprüften Ausnahmen für einen solchen Zweck vorgesehen - Ausnahmen, die in der Signatur deklariert werden müssen, um wahrscheinlich ausgelöst zu werden, und der Aufrufer muss diese explizit abfangen. Im Gegensatz dazu können ungeprüfte Ausnahmen (auch bekannt als RuntimeExceptions) jederzeit auftreten, ohne dass eine catch-Klausel im Code definiert werden muss. Während die ersten für wiederherstellbare Fälle verwendet werden (z. B. falsche Benutzereingaben, Dateiname existiert nicht), werden die letzteren für Fälle verwendet, gegen die der Benutzer / Programmierer nichts tun kann (z. B. zu wenig Speicher).

Sie sollten jedoch, wie bereits von @Stephen C erwähnt, Ihre eigenen Ausnahmen definieren und speziell diejenigen abfangen, um andere nicht unbeabsichtigt abzufangen.

Eine andere Möglichkeit wäre jedoch die Verwendung von Datenübertragungsobjekten, die einfach Datencontainer ohne Logik sind. Sie übergeben dann ein solches DTO zur Validierung an einen Validator oder das Modellobjekt selbst und nehmen nur bei Erfolg die Aktualisierungen im Modellobjekt vor. Dieser Ansatz wird häufig verwendet, wenn Präsentationslogik und Anwendungslogik getrennte Ebenen sind (Präsentation ist eine Webseite, Anwendung ein Webservice). Auf diese Weise werden sie physisch getrennt. Wenn Sie jedoch beide auf einer Ebene haben (wie in Ihrem Beispiel), müssen Sie sicherstellen, dass es keine Problemumgehung gibt, um einen Wert ohne Validierung festzulegen.

Andy
quelle
4

Mit meinem Haskell-Hut sind beide Ansätze falsch.

Was konzeptionell passiert, ist, dass Sie zuerst eine Reihe von Bytes haben und nach dem Parsen und Validieren eine Person erstellen können.

Die Person hat bestimmte Invarianten, wie die Vorwahl eines Namens und eines Alters.

Die Möglichkeit, eine Person zu repräsentieren, die nur einen Namen hat, aber kein Alter, möchten Sie unbedingt vermeiden, denn das schafft Vollständigkeit. Strenge Invarianten bedeuten, dass Sie beispielsweise später nicht mehr nach dem Vorhandensein eines Alters suchen müssen.

In meiner Welt wird Person atomar mithilfe eines einzelnen Konstruktors oder einer einzelnen Funktion erstellt. Dieser Konstruktor oder diese Funktion kann erneut die Gültigkeit der Parameter überprüfen, es sollte jedoch keine halbe Person erstellt werden.

Leider machen Java, PHP und andere OO-Sprachen die richtige Option ziemlich ausführlich. In geeigneten Java-APIs werden häufig Builder-Objekte verwendet. In einer solchen API würde das Erstellen einer Person ungefähr so ​​aussehen:

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

oder umso ausführlicher:

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

In diesen Fällen ist es unabhängig davon, wo Ausnahmen ausgelöst werden oder wo eine Validierung stattfindet, unmöglich, eine ungültige Personeninstanz zu empfangen.

user239558
quelle
Alles, was Sie hier getan haben, ist, das Problem in die Builder-Klasse zu verschieben, die Sie nicht wirklich beantwortet haben.
Cypher
2
Ich habe das Problem in der Funktion builder.build () lokalisiert, die atomar ausgeführt wird. Diese Funktion ist eine Liste aller Überprüfungsschritte. Es gibt einen großen Unterschied zwischen diesem Ansatz und Ad-hoc-Ansätzen. Die Builder-Klasse hat keine Invarianten über die einfachen Typen hinaus, während die Person-Klasse starke Invarianten hat. Beim Erstellen korrekter Programme geht es darum, starke Invarianten in Ihren Daten zu erzwingen.
user239558
Es beantwortet die Frage immer noch nicht (zumindest nicht vollständig). Können Sie erläutern, wie einzelne Fehlermeldungen von der Builder-Klasse im Aufrufstapel an die Ansicht übergeben werden?
Cypher
Drei Möglichkeiten: build () kann bestimmte Ausnahmen auslösen, wie im ersten Beispiel des OP. Es kann eine öffentliche Menge <String> validate () geben, die eine Reihe von lesbaren Fehlern zurückgibt. Es gibt ein öffentliches Set <Error> validate () für i18n-fähige Fehler. Der Punkt ist, dass dies während der Konvertierung in ein Personenobjekt geschieht.
user239558
2

In Laienwörtern:

Der erste Ansatz ist der richtige.

Der zweite Ansatz geht davon aus, dass diese Geschäftsklassen nur von diesen Controllern aufgerufen werden und dass sie niemals aus einem anderen Kontext aufgerufen werden.

Geschäftsklassen müssen jedes Mal eine Ausnahme auslösen, wenn eine Geschäftsregel verletzt wird.

Der Controller oder die Präsentationsschicht müssen entscheiden, ob sie ausgelöst werden oder ob sie eigene Validierungen durchführen, um zu verhindern, dass Ausnahmen auftreten.

Denken Sie daran: Ihre Klassen werden möglicherweise in unterschiedlichen Kontexten und von unterschiedlichen Integratoren verwendet. Sie müssen also klug genug sein, um Ausnahmen für schlechte Eingaben auszulösen.

Tulains Córdova
quelle