4
\$\begingroup\$

I'm updating a website I made awhile ago from a functional design to an OOP design. There are still a lot of functions below this class that I haven't implemented yet but I'm getting to it (such as getUnsentNotifications, fireUnsentNotifications, and other functions such as sendSMS and sendEmail). Please let me know what I need to improve on.

Basically, this class is a 'model' (I think). I've heard some vague terms about OOP, databases, and what the proper way to design them are.

The purpose of this class is to ease the setting and retrieval of notifications (which can be in the form of an Email or SMS). Unsent notifications are then sent in a queue like system on a separate thread.

require_once 'private/Twilio.php';
require_once 'private/mysql.php';
require_once 'private/server_controls.php';
require_once 'private/user_controls.php';

class Notification {
    private $id = NULL;
    public $sent = FALSE;
    private $type;
    public $subject;
    public $body;
    public $address;
    public $name;

    public function __construct($id=NULL, $sent=FALSE) 
    {
        $this->id = $id;
        $this->sent = $sent;
    }

    public function getType()
    {
        return $this->type;
    }

    public function isSMS()
    {
        return $this->type == "SMS";
    }

    public function isEmail()
    {
        return $this->type == "EMAIL";
    }

    public function clear()
    {
        unset($sent);
        unset($type);
        unset($subject);
        unset($body);
        unset($address);
        unset($name);
    }

    public function setSMS($body, $address)
    {
        $this->clear();
        $this->type="SMS";
        $this->body = $body;
        $this->address = $address;
    }

    public function setEmail($subject, $body, $address, $name)
    {
        $this->clear();
        $this->type="EMAIL";
        $this->subject=$subject;
        $this->body=$body;
        $this->address=$address;
        $this->name=$name;
    }

    public function send()
    {
        if ($this->isSMS()) {
            $result = send_sms_blocking($this->body, $this->address);

            if ($result === TRUE) {
                $this->mark_sent();
                return TRUE;
            } else {
                $this->mark_sent();
                $this->error = "SMS could not be sent";
                return FALSE;
            }

        } elseif ($this->isEmail()) {
            $result = send_email_blocking($this->subject, $this->body, $this->address, $this->name);

            if ($result === TRUE) {
                $this->mark_sent();
                return TRUE;
            } else {
                $this->mark_sent();
                $this->error = $result;
                return FALSE;
            }
        }
    }

    public function mark_sent()
    {
        $this->sent = TRUE;
        $this->push();
    }

    public function push()
    {
        if ($this->id !== NULL)
            $stmt = mycon()->prepare("UPDATE notifications SET type=?, subject=?, body=?, address=?, name=?, sent=? WHERE ID=?");
        else
            $stmt = mycon()->prepare("INSERT INTO notifications (type, subject, body, address, name, sent) VALUES (?,?,?,?,?,?)");

        if (!$stmt) {
            return FALSE;
        }
        if ($this->id !== NULL)
            $stmt->bind_param("sssssii", $this->type, $this->subject, $this->body, $this->address, $this->name, $this->sent, $this->id);
        else
            $stmt->bind_param("sssssi", $this->type, $this->subject, $this->body, $this->address, $this->name, $this->sent);

        $result = $stmt->execute();
        if ($id === NULL)
            $this->id = $stmt->insert_id;

        $stmt->close();
        return $result;
    }
}

function getUnsentNotifications()
{
    $stmt = mycon()->prepare("SELECT ID, type, subject, body, address, name, sent FROM notifications WHERE sent=FALSE");
    if (!$stmt or !$stmt->execute())
        return FALSE;


    $result = $stmt->get_result();
    $unsent = array();
    while ($info = $result->fetch_assoc()) {
        if ($info["type"] == "SMS") {
            $notification = new Notification($info["ID"], $info["sent"]);
            $notification->setSMS($info["body"], $info["address"]);
            $unsent[] = $notification;
        } else {
            $notification = new Notification($info["ID"], $info["sent"]);
            $notification->setEmail($info["subject"], $info["body"], $info["address"], $info["name"]);
            $unsent[] = $notification;
        }
    }
    return $unsent;
}

function sendSMS($body, $to)
{
    $notification = new Notification();
    $notification->setSMS($body, $to);
    return $notification->push();
}

function sendEmail($subject, $message, $email, $name=NULL)
{
    $notification = new Notification();
    $notification->setEmail($subject, $message, $email, $name);
    return $notification->push();
}

function fireNotifications()
{
    foreach(getUnsentNotifications() as $notification) {
        if (!$notification->send()) {
            create_alert(2, $notification->isSMS(), NULL, $notification->address . " - " . $notification->error);
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ What code do you have in send_sms_blocking and send_email_blocking Kindly show that code too. \$\endgroup\$ – Alankar More Feb 15 '16 at 7:47
1
\$\begingroup\$

Below are some improvements that you can do.

  • Separating the logic of sending SMS and Email
  • Do not call the push() function from the mark_sent The purpose of mark_sent function should be set the type = TRUE that's it. As per the coding standards PSR 2.0 use camel case for specifying the function name. as mark_sent will be markSent
  • While specifying the hard coded values in the if conditions always place the hard coded value at left hand side. as if(NULL === $id) it will avoid by mistakenly initialization of variables.
  • Try to write reusable code.

Main class Notification.php : This is base class.

  // Your necessary required files list will go here

abstract class Notification
{
    /**
     * 
     * var @integer
     */
    protected $id;

    /**
     * 
     * var @string
     */
    protected $body;

    /**
     * 
     * var @string
     */ 
    public $subject;

    /**
     * 
     * var @string
     */ 
    public $to;

    /**
     * 
     * var @boolean
     */ 
    public $sent = FALSE;


    /**
     * 
     * var @string
     */     
    public $type = 'SMS';


    /**
     * This is abstract function which definition can be provided by 
     * the child classes depends upon their need they can modified this 
     * function
     * 
     */ 
    abstract protected function send();

    /**
     * Marking notification as sent. 
     * 
     */
    public function markAsSent()
    {
        $this->sent = TRUE;
    }

    /**
     * Get all notifications 
     *
     * @return array
     */ 
    protected function getNotifications()
    {
        $unsent = array();
        $stmt = mycon()->prepare("SELECT ID, type, subject, body, address, name, sent FROM notifications WHERE sent=FALSE");
        if (!$stmt or !$stmt->execute()) {
            return $unsent;
        }

        $result = $stmt->get_result();

        while ($info = $result->fetch_assoc()) {
            $notification = null;
            if ("SMS" == $info['type']) {
                $notification = new SMSNotification($info['body'], $info['address']);
            } else {
                $notification = new EmailNotification($info["subject"], $info["body"], $info["address"], $info["name"]);
            }

            $notification->id = $info['id'];
            $notification->sent = $info['sent'];
            $unsent[] = $notification;
        }

            return $unsent; 
    }


    /**
     * Pushing data into the database if the id is assigned then updating 
     * record.
     * 
     * @return mixed boolean | integer
     */ 
    public function push()
    {
        $stmt = mycon()->prepare("INSERT INTO notifications (type, subject, body, address, name, sent) VALUES (?,?,?,?,?,?)");
        if (NULL !== $this->id) {
            $stmt = mycon()->prepare("UPDATE notifications SET type=?, subject=?, body=?, address=?, name=?, sent=? WHERE ID=?");
        } 

        if (!$stmt) {
            return FALSE;
        }

        if (NULL !== $this->id) {
            $stmt->bind_param("sssssii", $this->type, $this->subject, $this->body, $this->address, $this->name, $this->sent, $this->id);
        } else {
            $stmt->bind_param("sssssi", $this->type, $this->subject, $this->body, $this->address, $this->name, $this->sent);
        }

        $result = $stmt->execute();
        if (NULL === $id) {
            $this->id = $stmt->insert_id;
        }

        $stmt->close();

        return $result;
    }

}

Class for sending SMS (SMSNotification.php) : Inherits from the Notification class

/**
* This class inherits abstract class Notification and it will provide the 
* definition for the send() function to describe how the SMS will be sent.
*
*/

// your required files list will be placed here
require 'PATH/FOR/Notification.php';

class SMSNotification extends Notification
{

    /**
     * Initialization
     * 
     * @params $body string | null
     * @params $to string | null 
     */
    public function __construct($body = null, $to = null)   
    {
        $this->body = $body;
        $this->to =$to;
    }

    /**
    * As this is the abstract function in base class we have to provide the 
    * actual definition for this class in sub class.
    * 
    * @return boolean
    */
    public function send()
    {
        $result = send_sms_blocking($this->body, $this->to);
            if (TRUE === $result) {
                $this->markAsSent();
                $this->push();
                return TRUE;
            } 

            $this->error = "SMS could not be sent";
            return FALSE;
    }
}

Class for sending Email (EmailNotification.php) : Inherits from Notification

/**
* This class inherits abstract class Notification and it will provide the 
* definition for the send() function to describe how the Email will be sent.
*
*/

// your required files list will be placed here
require 'PATH/FOR/Notification.php';

class EmailNotification extends Notification
{
    /**
    *  Name of the person to which this email is going to be send.
    *
    * var @string
    */
    public $name;

    /**
     * {inherit}
     * 
     * @var string
     */ 
    public $type = 'EMAIL';

    /**
     * Initialition of object
     * 
     */ 
    public function __construct($subject = null, $body = null, $to = null, $name = null)    
    {
        $this->subject = $subject;
        $this->body = $body;
        $this->to =$to;
        $this->name = $name;
    }

    /**
    * As this is the abstract function in base class we have to provide the 
    * actual definition for this class in sub class.
    *
    * @return boolean
    */
    protected function send()
    {
        $result = send_email_blocking($this->subject, $this->body, $this->to, $this->name);

        if ($result === TRUE) {
            $this->markAsSent();
            $this->push();
            return TRUE;
        } 

        $this->error = "Email could not be sent";
        return FALSE;
    }
}

NOTE: - I don't understand your below code: which you had wrote in your send function

if ($result === TRUE) {
       $this->mark_sent();
       return TRUE;
  } else {
    $this->mark_sent();
    $this->error = $result;
    return FALSE;
}

You have called $this->mark_sent() from if as well as from else part too. If you want to call this function in every condition you can put above the if.

  • I have renamed your variable $address as $to to be more specific (But its not big change)

  • Always try to use the Object oriented concepts.

  • In this code you can use getter/ setter methods which can be access outside the class.

  • Additionally you can write the database class where you can put your queries related stuff that we have written in the Notification class.

I hope this will help you to understand how we can separate the logic and write reusable code.

\$\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.