2
\$\begingroup\$

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;
            }
}
\$\endgroup\$
  • \$\begingroup\$ What do you mean by "something similar to"? And what is dbAdapter? \$\endgroup\$ – 200_success Oct 5 '16 at 19:44
  • \$\begingroup\$ where is this alleged "function" you speak of? \$\endgroup\$ – I wrestled a bear once. Aug 2 '17 at 2:14
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ $subject comes from a text field and $message from a text editor where it contain html tags. so these variables needs to store in the database \$\endgroup\$ – user27 Oct 5 '16 at 19:02
  • \$\begingroup\$ If you're going to save those variables to a database, then you should be doing none of that mangling. But I wouldn't be able to show you exactly how it should be done unless you also include the database insertion call in the question. \$\endgroup\$ – 200_success Oct 5 '16 at 19:05
  • \$\begingroup\$ please check my edit \$\endgroup\$ – user27 Oct 5 '16 at 19:42
  • 3
    \$\begingroup\$ You shouldn't be saving mangled data in DB, instead you should use htmlentities when you are printing them. Once you will want to do some operations with those data in DB and it will bite you in the arse... I used to do similar thing, but I got burned and also educated about this at work. \$\endgroup\$ – MiChAeLoKGB Dec 6 '16 at 14:49
0
\$\begingroup\$
$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.

\$\endgroup\$
  • \$\begingroup\$ as i mention, in a scenario where $message fails in validation then the rest of the if(!empty) statements become an extra overhead. this is the reason which makes my code suboptimal. \$\endgroup\$ – user27 Oct 5 '16 at 18:30
  • \$\begingroup\$ i can use your solution but what if i need to give a proper error message (such as "message subject length exceed" ) rather than giving a common message? Also considering your function parametersValid(), i need to know that is it a good practice to have multiple exit points inside a function? \$\endgroup\$ – user27 Oct 5 '16 at 18:48
0
\$\begingroup\$

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).

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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