Am I doing this right with OOP PHP ?

Question

Hello, I'm now in the process of rewriting one of my script (contact script with ajax) with OOP PHP after I read a lot about its advantages.

The script became longer but I think this is good in oop. I have read a lot of articles about how to code php using oop , but it's still confusing to me.

The Code

First look at this part of the script:

/*
 * Validate the data that was given by the user
 */
public function isDataVaild() {
        if (array_filter($_POST, array($this, '_isDataEmpty'))) {
            $this->_error('Please fill all the required info');
            return false;
        }

        if (!filter_var($_POST['email'], FILTER_VALIDATE_EMAIL)) {
            $this->_error('Please use a vaild email');
            return false;
        }

        if (!isset($_SESSION['captcha']) || $_SESSION['captcha'] != $_POST['captcha']) {
            $this->_error('Plese make sure you to enter the correct answer to the spam question');
            return false;
        }
        return true;
}

/*
 * Check is the values are empty or not
 */
protected function _isDataEmpty($val) {
    return(empty(trim($val)));
}

/*
 * Check if there is seesion in not valid or if it does not pass the exploit test
 */
public function isThereExploit () {
        if(array_filter($_POST, array($this, '_validateExploit')) || !$this->_isSessionValid()) {
            if($this->_ajax) {
                $this->_error('Exploit Detected');
            } else {
                $this->_error("<strong style='color: red'>Warning</strong>: An Exploitation attempt has been detected!");
            }
            return false;
        }
        return true;
}

/*
 * Test to see if the values have an exploit
 */
protected function _validateExploit($val) {
    $exploitPattrens = array('content-type', 'to:', 'bcc:', 'cc:', 'document.cookie', 'document.write', 'onclick', 'onload', '\n', '\r', '\t', '%0A', '%0D', '%08', '%09');

    foreach ($exploitPattrens as $exploit) {
        if (strpos($val, $exploit) !== false){
            return true;
        }
    }
    return false;
}

/*
 * Check if the session is vaild for this user
 */
protected  function _isSessionValid() {
    return ($_POST['token'] == $_SESSION['token']);
}

/*
 * Make some sanitizing to the givin value
 */
protected function _clean(&$variable) {
    $variable = trim(filter_var($variable, FILTER_SANITIZE_STRING));
    return $variable;
}

/*
 * Make the message ready to be sent by removing extra data and fixing the rest
 */
protected function _cleanMessage() {   
    foreach ($_POST as $key => &$val) {
        if ($key == 'email') {
            $val = strtolower($val);
        }
        if ($key == 'captcha' || $key == 'token') {
            unset($_POST[$key]);
        }
        $this->_clean($val);
    }
    return $_POST;
}

/*
 * Make the message after checking if the data is vaild and clean
 */
private function _makeMessage() {

    if(!$this->_ajax) {
        if(!$this->isDataVaild()) {
            return;
        }
    }

    if(!$this->isThereExploit()) {
        return;
    }

    $messageEntries = $this->_cleanMessage();

    $message_start = "<div dir='rtl' style='padding: 50px 0 100px;background: #eeeeee; font-family: Arial, Helvetica, sans-serif;'><h1 align='center' style='font-size: 24px; font-weight: bold;color: #989898;margin-bottom: 35px'>New Message</h1><table width='600' align='center' border='1' style='border-collapse: collapse; border: 1px solid #dddddd;font-size: 16px;' cellpadding='14' cellspacing='2'>";

    $message_end = "</table><p style='margin:0;color:#CACACA;font-size:10px;padding-top:20px;text-align:center;'><a style='color:#CACACA;text-decoration:none;' href='http://coolcontact.co.cc'&gt;coolContact v1.2</a> - Developed &amp; Designed by Maher Salam, &copy; <a style='color:#CACACA;text-decoration:none;' href='http://coolworlds.net'&gt;coolworlds.net&lt;/a&gt;&lt;/p&gt;&lt;/div&gt;";

    $this->_message .= $message_start;

   foreach ($messageEntries as $id => $entrie) {

        $this->_message .= "<tr valign='top' bgcolor='#ffffff'><td width='90' align='left' style='color: #989898;'><b>" . $id . '</b></td><td>' . nl2br($entrie) . '</td></tr>';
        $this->_messagePlein .= $id . ': ' . nl2br($entrie) . '\r\n';
    }

    $this->_message .= $message_end;
}

/*
 * Send the message and return true if it worked
 */
public function send() {
        $this->_makeMessage();

        require 'class.phpmailer-lite.php';
        $mail = new PHPMailerLite();
        $mail->Mailer = 'mail';
        $mail->CharSet = 'UTF-8';

        $mail->SetFrom($this->_senderEmail, $this->_senderName);
        $mail->AddAddress($this->_recieverEmail);

        $mail->Subject = $this->_messageTitle;
        $mail->IsHTML(true);

        $mail->Body = $this->_message;
        $mail->AltBody = $this->_messagePleins;

        $mail->Send();

        return true;
}

I know this might be a lot of code to read, but I wanted to give you the whole picture :)

The Problem

Is there a better way to rewrite some of these functions (like makeMessage())? And how about performance?

Thanks in advance.

Answer 1

First of all I want to say: Good choice to go object oriented! It's a better way of programming. But...

It looks to me most of your methods are static methods. You need to bundle all methods that have something in common and save the relevant information as class attributes. This looks like a set of functions that are accidently related to eachother in this case, bundled in a class.

Besides, it is wrong to assume there are several keys in your global $_POST variable available. Classes are meant to be reusable. Using these kind of assumptions makes them not.

Besides that, some strange things in your code:

protected function _clean(&$variable) {
    $variable = trim(filter_var($variable, FILTER_SANITIZE_STRING));
    return $variable;
}

Why do you give the $variable as a reference if you also return the same variable?

According to the documentation of empty:

empty() only checks variables as anything else will result in a parse error. 
In other words, the following will not work: empty(trim($name)). 

this will not work properly:

protected function _isDataEmpty($val) {
    return(empty(trim($val)));
}

Again, it is a good try, but you will need to have a better look at the meaning of object oriented programming.

• Differences between statis and non-static
• Class attributes and the way to access them
• When making a method protected and when private

Once you do understand the basics of OOP you will find out there is a lot more in the wonderful world of OOP!

Good luck!