Validate optional input parameters with if(!empty)

Question

I have a function which accepts four parameters, which are all optional. Once they're received, it validates using if(!empty).

$subject = (empty($params['subject'])) ? '' : addslashes(trim(strip_tags($params['subject'])));
$message = (empty($params['message'])) ? '' : addslashes(htmlentities($params['message']));
$userIdArray = (!empty($params['users']) && is_array($params['users'])) ? array_map('intval', $params['users']) : array();
$messageId = (empty($params['id'])) ? null : intval($params['id']);

$validMessage = true;



$valHelper = new validation();

if(!empty($message)){
    $validMessage = ($valHelper->validateMessageTextLength($message)) ? true : false;
}

if(!empty($subject) && $validMessage){
    $validMessage = ($valHelper->validateMessageSubjectTextLength($subject)) ? true : false;
}

if($messageId && $validMessage){
    $validMessage = ($valHelper->validateMessageId($messageId)) ? true : false; 
}

if ($validMessage) {

    // go to save method 
    $messageDbHelper = new Message_MessageDbHelper();
    $messageDbHelper->saveMessage($subject, $message)   
}  else {
    // show a common error message
}

As you can see, I'm giving a common error message and in a scenario where $message fails in validation then the rest of the if(!empty) statements become an extra overhead.

I need this code to be more optimized. Or is there a better way to do this?

EDIT-

save method is something similar to below

public function saveMessage($subject='', $message=''){
            try {
                $query = 'INSERT INTO message(subject, message) VALUES ("'.$subject.'", "'.$message.'")';
                return $this->dbAdapter->query($query)->rowCount();
            } catch (Exception $ex) {
                throw $ex;
            }
}

Answer 1

I am very concerned about all the mangling that you are doing with addslashes(trim(strip_tags(…))) and addslashes(htmlentities(…)). It's almost certainly wrong, but I can't tell you what would be right without knowing exactly how those variables get used.

Answer 2

$validMessage = true;



$valHelper = new validation();

if(!empty($message)){
    $validMessage = ($valHelper->validateMessageTextLength($message)) ? true : false;
}

if(!empty($subject) && $validMessage){
    $validMessage = ($valHelper->validateMessageSubjectTextLength($subject)) ? true : false;
}

if($messageId && $validMessage){
    $validMessage = ($valHelper->validateMessageId($messageId)) ? true : false; 
}

if ($validMessage) {

You could replace this with

if (parametersValid($message, $subject, $messageID)) {

and define

function parametersValid($message, $subject, $messageID) {
    $validationHelper = new validation();

    if (!empty($message) && !$validationHelper->validateMessageTextLength($message))) {
        return false;
    }

    if (!empty($subject) && !$validationHelper->validateMessageSubjectTextLength($subject)) {
        return false;
    }

    if ($messageId && !$validationHelper->validateMessageId($messageId)) {
        return false; 
    }

    return true;
}

Then you don't need $validMessage, and you don't need to keep checking it. This function returns as soon as it knows the result is false rather than keep doing unnecessary checks.

I wrote out $validationHelper, as I find it more readable that way.

Added more whitespace for the same reason.

We could unify the last if and the final return, but I find it more readable when separate in this case.

    return !$messageId || $validationHelper->validateMessageId($messageId);

You mention the idea of optimizing the code, but you are missing important context. What makes this code suboptimal? What do you do if the message is valid? Invalid? What does the validation do? As is, this is the only optimization that I see, and it's not much of one.

Answer 3

Stylistically, I think you have gone overboard in your use of ternaries. It leads to long lines of code that are hard to read and jamed full of logic steps.

You also have a lot of variable assignement going on that simple doesn't need to happen.

You could REALLY simplify your code to something like:

$valHelper = new validation();

if(empty($params['message'])) {
   // input error condition
   // fail out
}

if(false === $valHelper->validateMessageTextLength($params['message'])) {
   // input error condition
   // fail out
}

// Actually populate $message now
// Use whatever you logic is around transforming $params['message'] to $message
// which seem a bit unclear without more context to this code.
$message = ...; 

if(empty($params['subject'])) {
    // fail
}

// etc. for you other conditions

Or, if you build the right logic into your validation class, you could ideally get to something like

$fields = $valHelper->getValidatedFields($params);

if(false === $fields) {
    // error in input
    // fail out with message to user
}

// work with $fields data

Where you can pass the parameters to a single method and get returned either a false (or structured error return information) or an array of fields that have all been validated (or perhaps even cleaned/sanitized if you want further extend the functionality of the class).