Method property input validation

Question

How should I handle the input validation of a method?
Of these two, which one is the more correct way? or there is a better way
This method is called by the constructor and $prodID can be user input or come from the db.

private function fill_data1($prodID)
{
    //Way 1
    filter_var($prodID, FILTER_VALIDATE_INT, array('options'=>array('min_range'=>1, 'max_range'=>1000000)));
    if (is_null($prodID)) {
        return FALSE;
    } elseif ($prodID === FALSE) {
        return FALSE;
    }
    $prod = getArtData($prodID);
    $this->set_id($prod['artID']);
    $this->set_name($prod['artName']);
    $this->set_price($prod['precio']);
}

private function fill_data(2$prodID)
{
    //Way 2
    filter_var($prodID, FILTER_VALIDATE_INT, array('options'=>array('min_range'=>1, 'max_range'=>1000000)));
    if (is_null($prodID) || $prodID === FALSE)
    {
        die('invalid input for prodID (' . $prodID . '). It has to be an integer > 0');
    }
    $prod = getArtData($prodID);
    $this->set_id($prod['artID']);
    $this->set_name($prod['artName']);
    $this->set_price($prod['precio']);
}

David Harkness · Accepted Answer · 2011-02-08 02:10:24Z

Option 3: Use exceptions and put the ID validation as close to the data as possible.

public function getArtData($id) {
    if (!is_int($id) || $id <= 0) {
        throw new InvalidIdentifierException(
                "Article ID $id must be a valid, positive integer.");
    }
    ...
}

The problem with returning false is that you must check for the return value to handle and skip it. If where you handle it (the presentation layer) is several function calls removed from where it's validated (the data layer), you have to check for false at each level. An exception will propagate up to the first function that catches it, bypassing the remaining code in each function along the way.

function displayProductAction(...) {
    $prodID = $request->getParam('prod');
    $form = ...
    try {
        $form->fill_data($prodID)
        $view->form = $form;
    }
    catch (InvalidIdentifierException $e) {
        $view->error = $e->getMessage();
    }
    $view->render();
}

Calling die() causes its own difficulties. It is harder to unit test and it forces you to put the error-display code at the point of failure. What happens when you want to use your code from a web-service?

Samuel Herzog · Accepted Answer · 2011-02-08 02:02:36Z

0

You should not use die() when validating your input.
Your user should see a proper error message and hints on how to give a correct input.

Therefore I suggest you to use the first way.
Depending on your architecture it might make more sense to throw an exception instead of returning false.

answered Feb 8, 2011 at 2:02

Samuel Herzog

3,5991 gold badge24 silver badges21 bronze badges

3 Comments

The Disintegrator Over a year ago

Yes, die is a little harsh, but for sure the user will ser the message (it would be the only thing on the page)

The Disintegrator Over a year ago

The thing is, if the input is invalid. The execution must stop immediately, the next step can be a db write..

The Disintegrator Over a year ago

I hate this comment box, hitting enter submits automatically... Which would be the correct way of using an exception here?

Collectives™ on Stack Overflow

Method property input validation

2 Answers 2

Comments

3 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

3 Comments

Your Answer

Sign up or log in

Post as a guest

Related