Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

Reading this SO question it seems that throwing exceptions for validating user input is frowned upon.

But who should validate this data? In my applications, all validations are done in the business layer, because only the class itself really knows which values are valid for each one of its properties. If I were to copy the rules for validating a property to the controller, it is possible that the validation rules change and now there are two places where the modification should be made.

Is my premise that validation should be done on the business layer wrong?

What I do

So my code usually ends up like this:

<?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 the controller, I just pass the input data to the model, and catch thrown exceptions to show the error(s) to the user:

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

Is this a bad methodology?

Alternate method

Should maybe I create methods for isValidAge($a) that return true/false and then call them from the controller?

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

And the controller will be basically the same, just instead of try/catch there are now 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);
}

So, what should I do?

I'm pretty happy with my original method, and my colleagues to whom I have showed it in general have liked it. Despite this, should I change to the alternate method? Or am I doing this terribly wrong and I should look for another way?

share|improve this question
    
I've modified the "original" code a bit to handle ValidationException and other exceptions –  Carlos Campderrós Jun 12 '13 at 10:46
2  
One problem with showing exception messages to the end-user is that the model suddenly needs to know what language the user speaks, but that is mostly a concern for the View. –  Bart van Ingen Schenau Jun 12 '13 at 11:18
    
@BartvanIngenSchenau good catch. My applications always have been single-language, but it's good to think in localization problems that may arise in any implementation. –  Carlos Campderrós Jun 12 '13 at 11:49
    
Exceptions for validations is just a fancy way of injecting types into the process. You can achieve the same results by return an object that implements a validation interface like IValidateResults. –  Mathew Foscarini Jun 2 at 23:05

6 Answers 6

up vote 6 down vote accepted

The approach I've used in the past is to put all validation logic dedicated Validation classes.

You can then inject these Validation classes into your Presentation Layer for early input validation. And, nothing prevents your Model classes from using the very same classes to enforce Data Integrity.

Following this approach you can then treat Validation Errors differently depending on which layer they happen in:

  • If Data Integrity Validation fails in the Model, then throw an Exception.
  • If User Input Validation fails in the Presentation Layer, then display a helpful tip and delay pushing the value to your Model.
share|improve this answer
    
So you have class PersonValidator with all the logic to validate the different attributes of a Person, and the Person class that depends on this PersonValidator, right? What's the advantage your proposal offers over the alternate method I suggested in the question? I only see the capacity to inject different Validation classes for a Person, but I can't think of any case where this would be needed. –  Carlos Campderrós Jun 12 '13 at 11:40
    
I agree that adding a whole new class for validation is overkill, at least in this relatively simple case. It could be useful for a problem with a lot more complexity. –  dan1111 Jun 12 '13 at 11:42
    
Well, for an application that you plan to sell to multiple people/companies it may made sense have it, because each company may have different rules for validating what is a valid range for a Person's age. So it is possible useful, but really overkill for my needs. Anyway, +1 for you too –  Carlos Campderrós Jun 12 '13 at 11:48
1  
Seperating the validation from the model makes sense from a coupling and cohesion standpoint as well. In this simple scenario it may be overkill, but it will only take a single "cross field" validation rule to make the seperate Validator class much more appealing. –  Seth M. Jun 12 '13 at 11:55

I'm pretty happy with my original method, and my colleagues to whom I have showed it in general have liked it. Despite this, should I change to the alternate method? Or am I doing this terribly wrong and I should look for another way?

If you and your colleagues are happy with it, I see no pressing need to change.

The only thing that is questionable from a pragmatic perspective is that you are throwing Exception rather than something more specific. The problem is that if you catch Exception, you may end up catching exceptions that have nothing to do with validation of user input.


Now there are many people who say things like "exceptions should only be used for exceptional things, and X Y Z is not exceptional". (For example, @dann1111's Answer ... where he labels user errors as "perfectly normal".)

My response to that is that there is no objective criterion for deciding whether something ("X Y Z") is exceptional or not. It is a subjective measure. (The fact that any program needs to check for errors in user input does not make the occurrence errors "normal". In fact, "normal" is largely meaningless from an objective standpoint.)

There is a grain of truth in that mantra. In some languages (or more accurately, some language implementations) exception creation, throwing and/or catching is significantly more expensive than simple conditionals. But if you look at it from that perspective, you need to compare the cost of create/throw/catch against the cost of the extra tests that you might need to perform if you avoided using exceptions. And the "equation" has to take into account the probability that the exception needs to be thrown.

The other argument against exceptions is that they can make code harder to understand. But the flip-side is that when they are used appropriately, they can make code easier to understand.


In short - the decision to use or not use exceptions should be made after weighing up the merits ... and NOT on the basis of some simplistic dogma.

share|improve this answer
    
Good point about the generic Exception being thrown/catched. I really throw some own subclass of Exception, and the code of the setters usually does nothing that could throw another exception. –  Carlos Campderrós Jun 12 '13 at 10:09
    
I've modified the "original" code a bit to handle ValidationException and other exceptions /cc @dan1111 –  Carlos Campderrós Jun 12 '13 at 10:47
1  
+1, I'd much rather have a descriptive ValidationException than going back to the dark ages of having to check the return value of every method call. Simpler code = potentially less errors. –  Heinzi Jun 12 '13 at 12:01
1  
@dan1111 - While I respect your right to have an opinion, nothing in your comment is anything other than opinion. There is no logical connection between the "normalness" of validation and the mechanism for handling validation errors. All you are doing is reciting the dogma. –  Stephen C Jun 12 '13 at 15:45
    
@StephenC, upon reflection I feel that I did state my case too strongly. I agree that it is more of a personal preference. –  dan1111 Jun 13 '13 at 15:40

In my opinion, it is useful to distinguish between application errors and user errors, and only use exceptions for the former.

  • Exceptions are intended to cover things that prevent your program from executing properly.

    They are unexpected occurrences that prevent you from continuing, and their design reflects this: they break normal execution and jump to a place that allows error handling.

  • User errors like invalid input are perfectly normal (from your program's perspective) and should not be treated as unexpected by your application.

    If the user enters the wrong value and you display an error message, did your program "fail" or have an error in any way? No. Your application was successful--given a certain kind of input, it produced the correct output in that situation.

    Handling user errors, because it is part of normal execution, should be part of your normal program flow, rather than handled by jumping out with an exception.

Of course it is possible to use exceptions for other than their intended purpose, but doing so confuses the paradigm and risks incorrect behavior when those errors occur.

Your original code is problematic:

  • The caller of the setAge() method must know way too much about the method's internal error handling: the caller needs to know that an exception is thrown when the age is invalid, and that no other exceptions could be thrown within the method. This assumption could be broken later if you added additional functionality within setAge().
  • If the caller does not catch exceptions, the invalid age exception would later be handled in some other, most likely opaque, way. Or even cause an unhandled exception crash. Not good behavior for invalid data being entered.

The alternative code also has problems:

  • An extra, possibly unnecessary method isValidAge() has been introduced.
  • Now the setAge() method has to assume that the caller already checked isValidAge() (a terrible assumption) or validate the age again. If it validates the age again, setAge() still has to provide some sort of error handling, and you are back to square one again.

Suggested design

  • Make setAge() return true on success and false on failure.

  • Check the return value of setAge() and if it failed, inform the user that the age was invalid, not with an exception, but with a normal function that displays an error to the user.

share|improve this answer
    
Then how should I do it? With the alternate method I proposed or with something totally different that I haven't thought of? Also, is my premise that "validation should be done on the business layer" false? –  Carlos Campderrós Jun 12 '13 at 10:11
    
@CarlosCampderrós, see the update; I was adding that information as you commented. Your original design had validation in the correct place, but it was a mistake to use exceptions to perform that validation. –  dan1111 Jun 12 '13 at 10:24
    
The alternate method does force the setAge to validate again, but as the logic is basically "if it is valid then set age else throw exception" it does not take me back to square one. –  Carlos Campderrós Jun 12 '13 at 10:35
1  
One problem I see both with the alternate method and the suggested design is that they lose the ability to distinguish WHY the age was invalid. It could be made to return true or an error string (yeah, php is soooo dirty), but this could lead to a lot of problems, because "The entered age is out of bounds" == true and people should always use ===, so this approach would be more problematic than the problem it tries to solve –  Carlos Campderrós Jun 12 '13 at 10:38
1  
But then coding the application is really tiresome because for every setAge() that you make anywhere, you have to check that it really worked. Throwing exceptions means that you shouldn't worry about remembering to check everything went fine. As I see it, trying to set an invalid value into a attribute/property is something exceptional and then, worth throwing the Exception. The model shouldn't care if it is getting its input from the database or from the user. It should never receive bad input, so I see it is legitimate to throw an exception there. –  Carlos Campderrós Jun 12 '13 at 11:45

From my point of view (I'm a Java guy) it is totally valid how you implemented it the first way.

It is valid that an object throws an Exception when some preconditions are not met (e.g. empty-string). In Java the concept of checked exceptions is inteded to such a purpose - exceptions that must be declared in the signature to be propably thrown, and the caller explicitly needs to catch those. In contrast, unchecked exceptions (aka RuntimeExceptions), may happen any time without the need to define a catch-clause in code. While the first are used for recoverable cases (e.g. wrong user input, filename does not exist), the latter are used for cases the user/programmer can't do anything about (e.g. Out-Of-Memory).

You should though, as already mentioned by @Stephen C, define your own exceptions and catch specifically those to not catch others unintentionally.

Another way, however, would be to use Data Transfer Objects which are simply data-containers without any logic. You then handover such a DTO to a validator or the Model-Object itself for validation, and only if successfull, make the updates in the Model-Object. This approach is often used when presentation logic and application logic are seperated tiers (presentation is a webpage, appplication a webservice). This way they are physically separated, but if you have both on one tier (as in your example), you must ensure that there will be no workaround to set a value without validation.

share|improve this answer

In layman's words:

The first approach is the correct one.

The second approach assumes that those business classes will only be called by those controllers, and that they will never be called from other context.

Business classes must throw an Exception everytime a business rule is violated.

Controller or presentation layer must decide whether it throws them or it does it's own validations to prevent the exceptions from happening.

Remember: your classes will potentially be used in different contexts and by different integrators. So they must be smart enough to throw exceptions to bad inputs.

share|improve this answer

With my Haskell hat on, both approaches are wrong.

What happens conceptually is that you first have a bunch of bytes, and after parsing and validating, you can then construct a Person.

The Person has certain invariants, such as the precense of a name and an age.

Being able to represent a Person that only has a name, but no age is something you want to avoid at all costs, because that is what creates compleity. Strict invariants means you do not need to check for the presence of an age later on for example.

So in my world, Person is created atomically by using a single constructor or function. That constructor or function can again check the validity of the parameters, but no half-Person should be constructed.

Unfortunately, Java, PHP and other OO languages make the correct option pretty verbose. In proper Java APIs, builder objects are often used. In such an API, creating a person would look something like this:

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

or the more verbose:

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 these cases, no matter where exceptions are thrown or where validation happens, it is impossible to receive a Person instance that is invalid.

share|improve this answer
    
All you've done here is move the problem to the Builder class, which you haven't really answered. –  Cypher Jul 9 at 23:58
    
I have localized the problem to the builder.build() function which is executed atomically. That function is a list of all the verification steps. There is a huge difference between this approach and ad-hoc approaches. The Builder class has no invariants beyond the simple types, while the Person class has strong invariants. Building correct programs is all about enforcing strong invariants in your data. –  user239558 Jul 10 at 20:52
    
It still doesn't answer the question (at least not completely). Could you elaborate on how individual error messages are passed from the Builder class up the call stack to the View? –  Cypher Jul 10 at 22:11
    
Three possibilities: build() can throw specific exceptions, like in the OP's first example. There can be a public Set<String> validate() that returns a set of human readable errors. There an be a public Set<Error> validate() for i18n-ready errors. The point is that this happens during conversion to a Person object. –  user239558 Jul 15 at 14:30

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.