Si el modelo está validando los datos, ¿no debería arrojar excepciones en la entrada incorrecta?

9

Al leer esta pregunta SO , parece que lanzar excepciones para validar la entrada del usuario está mal visto.

Pero, ¿quién debería validar estos datos? En mis aplicaciones, todas las validaciones se realizan en la capa empresarial, porque solo la clase en sí misma sabe qué valores son válidos para cada una de sus propiedades. Si tuviera que copiar las reglas para validar una propiedad al controlador, es posible que las reglas de validación cambien y ahora hay dos lugares donde se debe realizar la modificación.

¿Mi premisa de que la validación debe hacerse en la capa empresarial es incorrecta?

Lo que hago

Entonces mi código generalmente termina así:

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

En el controlador, simplemente paso los datos de entrada al modelo y capturo las excepciones lanzadas para mostrar los errores al usuario:

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

¿Es esta una mala metodología?

Metodo alternativo

¿Debería crear métodos para isValidAge($a)ese retorno verdadero / falso y luego llamarlos desde el controlador?

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

Y el controlador será básicamente el mismo, solo que en lugar de try / catch hay ahora 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);
}

¿Entonces qué debo hacer?

Estoy bastante contento con mi método original, y a mis colegas a quienes se lo mostré en general les ha gustado. A pesar de esto, ¿debería cambiar al método alternativo? ¿O estoy haciendo esto terriblemente mal y debería buscar otra forma?

Carlos Campderrós
fuente
He modificado un poco el código "original" para manejarlo ValidationExceptiony otras excepciones
Carlos Campderrós
2
Un problema al mostrar mensajes de excepción al usuario final es que el modelo de repente necesita saber qué idioma habla el usuario, pero eso es principalmente una preocupación para la Vista.
Bart van Ingen Schenau
@BartvanIngenSchenau buena captura. Mis aplicaciones siempre han sido de un solo idioma, pero es bueno pensar en los problemas de localización que pueden surgir en cualquier implementación.
Carlos Campderrós
Las excepciones para las validaciones son solo una forma elegante de inyectar tipos en el proceso. Puede lograr los mismos resultados devolviendo un objeto que implementa una interfaz de validación como IValidateResults.
Reactgular

Respuestas:

7

El enfoque que he usado en el pasado es poner todas las clases de validación dedicadas a la lógica de validación.

Luego puede inyectar estas clases de validación en su capa de presentación para una validación de entrada temprana. Y nada impide que sus clases Modelo utilicen las mismas clases para imponer la Integridad de los datos.

Siguiendo este enfoque, puede tratar los errores de validación de manera diferente dependiendo de en qué capa ocurran:

  • Si la validación de integridad de datos falla en el modelo, entonces arroje una excepción.
  • Si la validación de entrada del usuario falla en la capa de presentación, muestre una sugerencia útil y retrase la inserción del valor en su modelo.
MetaFight
fuente
Entonces tienes una clase PersonValidatorcon toda la lógica para validar los diferentes atributos de a Person, y la Personclase que depende de esto PersonValidator, ¿verdad? ¿Cuál es la ventaja que ofrece su propuesta sobre el método alternativo que sugerí en la pregunta? Solo veo la capacidad de inyectar diferentes clases de Validación para a Person, pero no puedo pensar en ningún caso en el que esto sea necesario.
Carlos Campderrós
Estoy de acuerdo en que agregar una clase completamente nueva para la validación es exagerado, al menos en este caso relativamente simple. Podría ser útil para un problema con mucha más complejidad.
Bueno, para una aplicación que planea vender a varias personas / compañías, puede tener sentido, porque cada compañía puede tener diferentes reglas para validar lo que es un rango válido para la edad de una Persona. Por lo tanto, es posible útil, pero realmente exagerado para mis necesidades. De todos modos, +1 para ti también
Carlos Campderrós
1
Separar la validación del modelo también tiene sentido desde el punto de vista del acoplamiento y la cohesión. En este escenario simple, puede ser excesivo, pero solo tomará una sola regla de validación de "campo cruzado" para hacer que la clase de Validador separada sea mucho más atractiva.
Seth M.
8

Estoy bastante contento con mi método original, y a mis colegas a quienes se lo mostré en general les ha gustado. A pesar de esto, ¿debería cambiar al método alternativo? ¿O estoy haciendo esto terriblemente mal y debería buscar otra forma?

Si usted y sus colegas están contentos con esto, no veo la necesidad urgente de cambiar.

Lo único que es cuestionable desde una perspectiva pragmática es que estás tirando en Exceptionlugar de algo más específico. El problema es que si detecta Exception, puede terminar detectando excepciones que no tienen nada que ver con la validación de la entrada del usuario.


Ahora hay muchas personas que dicen cosas como "las excepciones solo deben usarse para cosas excepcionales, y XYZ no es excepcional". (Por ejemplo, la respuesta de @ dann1111 ... donde etiqueta los errores del usuario como "perfectamente normales").

Mi respuesta a eso es que no hay un criterio objetivo para decidir si algo ("XY Z") es excepcional o no. Es una medida subjetiva . (El hecho de que cualquier programa necesite verificar errores en la entrada del usuario no hace que los errores de ocurrencia sean "normales". De hecho, "normal" carece de sentido desde un punto de vista objetivo).

Hay un grano de verdad en ese mantra. En algunos idiomas (o más exactamente, algunas implementaciones de lenguaje ) la creación, lanzamiento y / o captura de excepciones es significativamente más costosa que los condicionales simples. Pero si lo mira desde esa perspectiva, debe comparar el costo de crear / lanzar / atrapar con el costo de las pruebas adicionales que podría necesitar realizar si evitaba usar excepciones. Y la "ecuación" tiene que tener en cuenta la probabilidad de que deba lanzarse la excepción.

El otro argumento contra las excepciones es que pueden dificultar la comprensión del código. Pero la otra cara es que cuando se usan adecuadamente, pueden hacer que el código sea ​​más fácil de entender.


En resumen, la decisión de usar o no usar excepciones debe hacerse después de sopesar los méritos ... y NO sobre la base de algún dogma simplista.

Stephen C
fuente
Buen punto sobre el genérico Exceptionarrojado / atrapado. Realmente lanzo alguna subclase propia Exception, y el código de los setters generalmente no hace nada que pueda lanzar otra excepción.
Carlos Campderrós
He modificado un poco el código "original" para manejar ValidationException y otras excepciones / cc @ dan1111
Carlos Campderrós
1
+1, preferiría tener una ValidationException descriptiva que volver a la edad oscura de tener que verificar el valor de retorno de cada llamada al método. Código más simple = potencialmente menos errores.
Heinzi
2
@ dan1111 - Si bien respeto tu derecho a tener una opinión, nada en tu comentario es otra cosa que opinión. No existe una conexión lógica entre la "normalidad" de la validación y el mecanismo para manejar los errores de validación. Todo lo que estás haciendo es recitar el dogma.
Stephen C
@StephenC, después de reflexionar, siento que expongo mi caso con demasiada fuerza. Estoy de acuerdo en que es más una preferencia personal.
6

En mi opinión, es útil distinguir entre los errores de la aplicación y los errores del usuario , y solo usar excepciones para los primeros.

  • Las excepciones están destinadas a cubrir cosas que impiden que su programa se ejecute correctamente .

    Son sucesos inesperados que le impiden continuar, y su diseño refleja esto: interrumpen la ejecución normal y saltan a un lugar que permite el manejo de errores.

  • Los errores del usuario, como la entrada no válida, son perfectamente normales (desde la perspectiva de su programa) y su aplicación no debe tratarlos como inesperados .

    Si el usuario ingresa el valor incorrecto y usted muestra un mensaje de error, ¿su programa "falló" o tuvo algún error? No. Su aplicación fue exitosa: dado un cierto tipo de entrada, produjo la salida correcta en esa situación.

    El manejo de los errores del usuario, debido a que es parte de la ejecución normal, debería ser parte del flujo normal de su programa, en lugar de manejarse saltando con una excepción.

Por supuesto, es posible usar excepciones para otro propósito que no sea el previsto, pero hacerlo confunde el paradigma y se arriesga a comportamientos incorrectos cuando se producen esos errores.

Su código original es problemático:

  • La persona que llama del setAge()método debe saber demasiado sobre el manejo de errores internos del método: la persona que llama necesita saber que se produce una excepción cuando la edad no es válida, y que no se pueden generar otras excepciones dentro del método . Esta suposición podría romperse más adelante si agrega una funcionalidad adicional dentro setAge().
  • Si la persona que llama no capta excepciones, la excepción de edad inválida se trataría más tarde de otra manera, muy probablemente opaca. O incluso causar un bloqueo de excepción no controlado. No es un buen comportamiento para los datos no válidos que se ingresan.

El código alternativo también tiene problemas:

  • Se isValidAge()ha introducido un método adicional, posiblemente innecesario .
  • Ahora, el setAge()método tiene que suponer que la persona que llamó ya marcó isValidAge()(una suposición terrible) o validar la edad nuevamente. Si vuelve a validar la antigüedad, setAge() aún debe proporcionar algún tipo de manejo de errores, y volverá al punto de partida nuevamente.

Diseño sugerido

  • Haga que el setAge()retorno sea verdadero en caso de éxito y falso en caso de fracaso.

  • Verifique el valor de retorno setAge()y, si falla, informe al usuario que la edad no era válida, no con una excepción, sino con una función normal que muestra un error al usuario.


fuente
Entonces, ¿cómo debo hacerlo? ¿Con el método alternativo que propuse o con algo totalmente diferente en lo que no he pensado? Además, ¿es falsa mi premisa de que "la validación debe hacerse en la capa empresarial"?
Carlos Campderrós
@ CarlosCampderrós, ver la actualización; Estaba agregando esa información como comentaste. Su diseño original tenía validación en el lugar correcto, pero fue un error usar excepciones para realizar esa validación.
El método alternativo obliga setAgea validar de nuevo, pero como la lógica es básicamente "si es válida, establezca la edad y la excepción de lanzamiento" no me lleva de vuelta al punto de partida.
Carlos Campderrós
2
Un problema que veo tanto con el método alternativo como con el diseño sugerido es que pierden la capacidad de distinguir POR QUÉ la edad no era válida. Se podría hacer que devuelva verdadero o una cadena de error (sí, php está muuuuy sucio), pero esto podría generar muchos problemas, porque las "The entered age is out of bounds" == truepersonas siempre deberían usar ===, por lo que este enfoque sería más problemático que el problema que intenta resolver
Carlos Campderrós
2
Pero codificar la aplicación es realmente agotador porque por cada cosa setAge()que haces en cualquier lugar, debes verificar que realmente funcionó. Lanzar excepciones significa que no debe preocuparse por recordar que todo salió bien. Tal como lo veo, tratar de establecer un valor no válido en un atributo / propiedad es algo excepcional y, por lo tanto, vale la pena lanzar el Exception. Al modelo no debería importarle si obtiene su entrada de la base de datos o del usuario. Nunca debería recibir una entrada incorrecta, por lo que veo que es legítimo lanzar una excepción allí.
Carlos Campderrós
4

Desde mi punto de vista (soy un chico de Java) es totalmente válido cómo lo implementó la primera vez.

Es válido que un objeto arroje una Excepción cuando no se cumplen algunas condiciones previas (por ejemplo, cadena vacía). En Java, el concepto de excepciones comprobadas está destinado a tal propósito: excepciones que deben declararse en la firma para que se puedan lanzar de manera propagable, y la persona que llama explícitamente necesita capturarlas. En contraste, las excepciones no verificadas (también conocidas como RuntimeExceptions) pueden ocurrir en cualquier momento sin la necesidad de definir una cláusula catch en el código. Mientras que los primeros se usan para casos recuperables (por ejemplo, entrada incorrecta del usuario, el nombre de archivo no existe), los segundos se usan para casos en los que el usuario / programador no puede hacer nada (por ejemplo, sin memoria).

Sin embargo, como ya mencionó @Stephen C, debe definir sus propias excepciones y capturar específicamente aquellas para no atrapar a otros sin querer.

Sin embargo, otra forma sería utilizar objetos de transferencia de datos que son simplemente contenedores de datos sin ninguna lógica. Luego transfiere dicho DTO a un validador o al Modelo-Objeto para su validación, y solo si tiene éxito, realice las actualizaciones en el Modelo-Objeto. Este enfoque se usa a menudo cuando la lógica de presentación y la lógica de aplicación son niveles separados (la presentación es una página web, la aplicación es un servicio web). De esta manera, se separan físicamente, pero si tiene ambos en un nivel (como en su ejemplo), debe asegurarse de que no habrá una solución alternativa para establecer un valor sin validación.

Andy
fuente
4

Con mi sombrero Haskell puesto, ambos enfoques están equivocados.

Lo que sucede conceptualmente es que primero tienes un montón de bytes, y después de analizar y validar, puedes construir una Persona.

La Persona tiene ciertos invariantes, como el precedencia de un nombre y una edad.

Ser capaz de representar a una Persona que solo tiene un nombre, pero que no tiene edad, es algo que desea evitar a toda costa, porque eso es lo que crea la totalidad. Los invariantes estrictos significan que no es necesario verificar la presencia de una edad posterior, por ejemplo.

Entonces, en mi mundo, Person se crea atómicamente usando un solo constructor o función. Ese constructor o función puede verificar nuevamente la validez de los parámetros, pero no se debe construir una media persona.

Desafortunadamente, Java, PHP y otros lenguajes OO hacen que la opción correcta sea bastante detallada. En las API de Java adecuadas, a menudo se usan los objetos del generador. En tal API, crear una persona se vería así:

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

o el más detallado:

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

En estos casos, no importa dónde se produzcan excepciones o donde se realice la validación, es imposible recibir una instancia de Persona que no sea válida.

user239558
fuente
Todo lo que has hecho aquí es mover el problema a la clase Builder, que realmente no has respondido.
Cypher
2
He localizado el problema en la función builder.build () que se ejecuta atómicamente. Esa función es una lista de todos los pasos de verificación. Hay una gran diferencia entre este enfoque y los enfoques ad-hoc. La clase Builder no tiene invariantes más allá de los tipos simples, mientras que la clase Person tiene invariantes fuertes. La construcción de programas correctos se trata de hacer cumplir fuertes invariantes en sus datos.
user239558
Todavía no responde la pregunta (al menos no completamente). ¿Podría explicar cómo se pasan los mensajes de error individuales desde la clase Builder hasta la pila de llamadas a la vista?
Cypher
Tres posibilidades: build () puede lanzar excepciones específicas, como en el primer ejemplo del OP. Puede haber un Set <String> validate () público que devuelva un conjunto de errores legibles por humanos. Puede haber un Set <Error> público validate () para errores listos para i18n. El punto es que esto sucede durante la conversión a un objeto Persona.
user239558
2

En palabras simples:

El primer enfoque es el correcto.

El segundo enfoque supone que esas clases de negocios solo serán llamadas por esos controladores, y que nunca serán llamadas desde otro contexto.

Las clases de negocios deben lanzar una excepción cada vez que se infringe una regla de negocios.

El controlador o la capa de presentación deben decidir si los arroja o si hace sus propias validaciones para evitar que ocurran las excepciones.

Recuerde: sus clases serán potencialmente utilizadas en diferentes contextos y por diferentes integradores. Por lo tanto, deben ser lo suficientemente inteligentes como para lanzar excepciones a las entradas incorrectas.

Tulains Córdova
fuente