Need feedback on my class

[pocobueno1388]

I'm practicing using OOP and would like some feedback on a class I programmed, as well as the use of the class. The class is for a private messaging system.

 

The Class

<?php

class inbox {
   var $user; //The users ID
   
   function __construct($user){
      $this->user = $user;
   }
   
   function set($var, $value){
      $this->$var = $value;
   }

   #################################################################################
   # FUNCTION retrieve
   # DESCRIPTION 
   #    Retrieve all messages from a users inbox from designated folder
   # PARAMETERS
   #    VARCHAR $folder - From which folder to get messages from? [1=inbox, 2=saved]  
   #################################################################################
   
   function retrieve($folder=1){
      
         $folder = (int)$folder;
         
         $query = "SELECT i.messageID, i.subject, i.status, DATE_FORMAT(i.date, '%b %D %l:%i %p') as date, u.username,
                          u.userID
                   FROM inbox i
                   LEFT JOIN users u
                   ON u.userID = i.sender
                   WHERE reciever='$this->user' AND folder=$folder";

         
      if ($result = mysql_query($query))
         return $result;          
      else
         return FALSE;
   }
   
   #################################################################################
   # FUNCTION get_status
   # DESCRIPTION 
   #    Returns word depending on status number (unread, read, replies)
   # PARAMETERS
   #    INT $status - The ID of the message  
   #################################################################################
   
   function get_status($status){
   
      switch($status){
         case 1:
            return "Unread";
         break;
         case 2:
            return "Read";
         break;
         case 3:
            return "Replied";
         break;
      }
         
   }
   
   #################################################################################
   # FUNCTION read_message
   # DESCRIPTION 
   #    Gets message information for specific message
   # PARAMETERS
   #    INT $messageID - The ID of the message
   #################################################################################
   
   function read_message($messageID){
      
      //check if this message belongs to the user trying to view it
      if ($this->check_user($this->user, $messageID)){
   
         $query = "SELECT i.subject, i.body, DATE_FORMAT(i.date, '%b %D %l:%i %p') as date, u.username, u.userID 
                   FROM inbox i
                   LEFT JOIN users u
                   ON u.userID = i.sender
                   WHERE messageID = $messageID";
                   
         if ($result = mysql_query($query)){
            $this->update_status($messageID, 2);
            return $result;
         } else {
            return FALSE;
         }
            
      } else {
         return FALSE;
      }

   }
         
   #################################################################################
   # FUNCTION send_message
   # DESCRIPTION 
   #    Send a message to another users inbox
   # PARAMETERS
   #    INT $to -  Users ID to send message to
   #    VARCHAR $subject - Subject of message
   #    TEXT $body - Body of message 
   #################################################################################    

   function send_message($to, $subject, $body){
   
      require_once 'purifier/HTMLPurifier.standalone.php';
     $purifier = new HTMLPurifier();
     $to = $purifier->purify(mysql_real_escape_string($to));
     $subject = $purifier->purify(mysql_real_escape_string($subject));
     $body = $purifier->purify(mysql_real_escape_string($body));
   
      $query = "INSERT INTO inbox (sender, reciever, subject, body, date)
                  VALUES ('$this->user', '$to', '$subject', '$body', NOW())";
      
      if (mysql_query($query))
         return TRUE;
      else
         return FALSE;

   }
   
   #################################################################################
   # FUNCTION delete
   # DESCRIPTION 
   #    Delete all selected messages from folder
   # PARAMETERS
   #    ARRAY $messages - Array of message IDs to delete  
   #################################################################################
   
   function delete($messages){
      
      $query = "DELETE FROM inbox WHERE messageID IN(" .implode(', ', $messages) . ")";
      
      if (mysql_query($query))
         return TRUE;
      else
         return FALSE;
   }
   
   #################################################################################
   # FUNCTION check_user
   # DESCRIPTION 
   #    Checks if the user is the owner of the message, if not don't let them read
   #    it.
   # PARAMETERS
   #    INT $user - The user's ID logged in
   #    INT $messageID - The ID of the message they are reading  
   #################################################################################
   
   function check_user($user, $messageID){
      
      $query = "SELECT COUNT(*) FROM inbox WHERE messageID='$messageID' AND reciever='$user'";
      
      if ($num_rows = mysql_result(mysql_query($query),  0))
         return $num_rows;
      else
         return FALSE;
         
   }
   
   #################################################################################
   # FUNCTION update_status
   # DESCRIPTION 
   #    Updates the status of a message (read, unread, replies)
   # PARAMETERS
   #    INT $messageID - The ID of the message they are reading  
   #################################################################################
   
   function update_status($messageID, $status){
      
      $query = mysql_query("SELECT status FROM inbox WHERE messageID=$messageID");
      $row = mysql_fetch_assoc($query);
      
      if ($row['status'] != $status){
         if (mysql_query("UPDATE inbox SET status=$status WHERE messageID=$messageID"))
            return TRUE;
         else
            return FALSE;
      }
              
   }
   
   #################################################################################
   # FUNCTION save
   # DESCRIPTION 
   #    Saves a message to users "saved" folder
   # PARAMETERS
   #    INT $messageID - The ID of the message they want to save 
   #################################################################################
   
   function save($messageID){
   
      $messageID = (int)$messageID;
      
      $query = "UPDATE inbox SET folder=2 WHERE messageID=$messageID";
      
      if (mysql_query($query)) return TRUE;
      else return FALSE;
      
   }               
   
}
   
?>

 

Using the class

<?php
include 'header.php';
include 'lib/inbox.class.php';

//=============== READ MESSAGE ===================================//

if (isset($_GET['messageID'])){

   echo '<table width="50%" align="center"><td><a href="inbox.php"><<< Back to Inbox</a></td></table>';

   $messageID = (int)$_GET['messageID'];
   $inbox = new inbox($sid);
   
   if ($result = $inbox->read_message($messageID)){
      $row = mysql_fetch_assoc($result);
      
print<<<HERE
   <form action="{$_SERVER['PHP_SELF']}?userID={$row['userID']}" method="post">
         <table id="mytable" cellspacing=0>
         <tr>
            <th colspan=2><b>{$row['subject']}</b></th>
         </tr>
         <tr>
            <td class="color"><a href="profile.php?userID={$row['userID']}">{$row['username']}</a></td>
            <td class="color" align="center">{$row['date']}</td>
         </tr>
         <tr>
HERE;
            echo '<td class="read-message" colspan=2>' . nl2br($row['body']) . '</td>';
print<<<HERE
         </tr>
         <tr>
            <td class="color" align="center" colspan=2>
               <input class="send-btn" type="submit" name="reply" value="Reply">
               <input class="send-btn" type="submit" name="save" value="Save">
            </td>
         </tr>
         </table>
         
         <input type="hidden" name="send_to" value="{$row['userID']}">
         <input type="hidden" name="subject" value="{$row['subject']}">
         <input type="hidden" name="messageID" value=$messageID>
         <input type="hidden" name="body" 
         value="\n\n\n--------------Player #{$row['userID']} said-------------------\n{$row['body']}">
   <form>
         
HERE;

   } else {
      $error[] = "This isn't your message!";
      error($error);
   }
      

//================ COMPOSE ============================================//
} else if ($_GET['action'] == 'compose' || isset($_POST['reply']) || isset($_POST['save'])){

   //If they save a message
   if (isset($_POST['save'])){
      $inbox = new inbox($sid);
      if ($inbox->save($_POST['messageID'])){
         msg("Message Saved");
      } else {
         $error[] = "There was an error saving your message";
      }
   }

   //if they press "send"
   if (isset($_POST['compose'])){
         
      if ((empty($_POST['send_to'])) || (empty($_POST['subject'])) || (empty($_POST['body'])))
         $error[] = "Please fill out all fields!";
         
      if (!is_numeric($_POST['send_to'])) $error[] = "Please enter a numeric player ID";
         
      if(!empty($error)){ 
         error($error);   
      } else {
      
         $inbox = new inbox($sid);
         
         if (isset($_POST['update'])) $inbox->update_status($_POST['update'], 3);
         
         if ($inbox->send_message($_POST['send_to'], $_POST['subject'], $_POST['body'])){
            msg("Message Sent");
         } else {
            $error[] = "There was an issue sending the message, please contact an administrator.";
         }
   
     }
      
   }
   
   //figure out who the message is to for the user  
   if (isset($_GET['userID'])) $send_to = (int)$_GET['userID'];
   else if (isset($_POST['send_to'])) $send_to = (int)$_POST['send_to'];
   else $send_to = "";
   
print<<<HERE

<table width="50%" align="center"><td><a href="inbox.php"><<< Back to Inbox</a></td></table>

<form action="{$_SERVER['PHP_SELF']}?action=compose" method="post">
   <table id="mytable" cellspacing="0">
   <tr>
      <th colspan=2>Compose</th>
   </tr>
   <tr>
      <td class="color"><b>Player #</b></td> 
      <td><input type="text" name="send_to" maxlength=11 size=11 value="$send_to"></td></td>
   </tr>
   <tr>
      <td class="color"><b>Subject</b></td> 
      <td><input type="text" name="subject" maxlength=25 size=25 value="{$_POST['subject']}"></td></td>
   </tr>
   <tr>
      <td class="color"><b>Body</b></td>
      <td><textarea rows=10 cols=50 name="body">{$_POST['body']}</textarea></td>
   </tr>
   <tr>
      <td class="color" align="center" colspan=2><input type="submit" name="compose" class="send-btn" value="Send"></td>
   </table>

HERE;

if (isset($_POST['reply'])) echo "<input type='hidden' name='update' value='{$_POST['messageID']}'>";

echo '</form>';
   
   
//================ INBOX ==============================================//
} else {

   $inbox = new inbox($sid);

   //=== Delete selected messages ===//
   if (isset($_POST['delete']) && !empty($_POST['del'])){
      if ($inbox->delete($_POST['del'])){ 
         msg("Messages Deleted");
      } else {
         $error[] = "Message(s) could not be deleted, please contact an administrator.";
         error($error);
      }
      
   } else if (isset($_POST['delete']) && empty($_POST['del'])) {
      $error[] = "You didn't select any messages to delete!";
      error($error);
   }

?>
<h2>Inbox Demo</h2>
<a href="inbox.php?action=compose">Compose</a> || 

<?php
if (isset($_GET['folder']) && $_GET['folder'] == 2)
   echo '<a href="inbox.php">Inbox</a>';
else
   echo '<a href="inbox.php?folder=2">Saved</a>';
?>

<p>

<form method="post">
<script type="text/javascript" src="http://www.shawnolson.net/scripts/public_smo_scripts.js"></script>

   <table id="mytable" cellspacing="0">
   <tr>
     <th><input type="checkbox" name="checkall" onclick="checkUncheckAll(this);"/></th>
     <th>Subject</th>
     <th>Sender</th>
     <th>Time</th>
     <th>Status</th>
   </tr>
   
<?php

   $folder = (isset($_GET['folder'])) ? (int)$_GET['folder'] : 1;
   $result = $inbox->retrieve($folder);
   
   if (mysql_num_rows($result) > 0){
   
      while ($row = mysql_fetch_assoc($result)){
   
         echo '<tr>'
              ."<td class='color'><input type='checkbox' name='del[]' value='{$row['messageID']}'></td>"
              ."<td><a href='{$_SERVER['PHP_SELF']}?messageID={$row['messageID']}'>{$row['subject']}</a></td>"
              ."<td class='color'><a href='profile.php?user={$row['userID']}'>{$row['username']}</a></td>"
              ."<td>{$row['date']}</td>"
              ."<td class='color'>" . $inbox->get_status($row['status']) . "</td>"
            .'</tr>';
            
      }
      
   } else {
      echo '<td colspan=5 align="center">No Messages</td>';
   }
   
echo '</table>';

print<<<HERE
   <table align="center" width="48%">
      <td><input type="submit" name="delete" value="Delete"></td>
   </table>
   
   </form>
HERE;

}
?>

 

Concerns

 

Initiating class multiple times through script

I am initiating a new class for each separate part of my script. Should I be only initiating the class at the top of the script and just change the information inside the class with my set() function (inside class) as I go along with the script, or am I doing it the best way?

 

Not setting enough variables in class

As you can see in my class I only set the variable $user for the class. Should I be setting much more variables, such as for the messageID, body, subject, send to, etc? I just put those as the parameters of the functions, and that takes care of it.

 

Those are the only concerns that immediately jumped out at me. So if anyone has any suggestions or comments at all, I would really appreciate hearing them. Thanks

[ignace]

why don't you just use overloading to set your properties? (http://be2.php.net/manual/en/language.oop5.overloading.php)

so that you are able to do:

 

<?php

class Inbox
{
    private $_properties = array();
    public function __set($key, $value)
    {
         $this->_properties[$key] = $value;
    }
    public function __get($key)
    {
         if (array_key_exists($key, $this->_properties)) {
             return $this->_properties[$key];
         }
    }
}

$inbox = new Inbox(1);
$inbox->foo = bar;

?>

 

if you want to use only one instance of the class, but don't want to be bothered with scopes, then use the Factory Pattern (http://en.wikipedia.org/wiki/Factory_method_pattern) implementation is something like:

 

<?php

class Inbox
{
    private static $_instance;
    
    public function getInstance()
    {
        if (self::$_instance === null) {
            self::$_instance = new self();
        }
        return self::$_instance;
    }
}

$inbox = Inbox::getInstance();
$inbox->set('foo', $bar);
unset($inbox);

function newScope() {
    $inbox = Inbox::getInstance();
    echo $inbox->get('foo'); // returns $bar
}

?>

 

if you combine both techniques you can do:

 

<?php

class Inbox
{
    private static $_instance;
    private $_properties = array();
    
    public function getInstance()
    {
        if (self::$_instance === null) {
            self::$_instance = new self();
        }
        return self::$_instance;
    }
    public function __set($key, $value)
    {
         $this->_properties[$key] = $value;
    }
    public function __get($key)
    {
         if (array_key_exists($key, $this->_properties)) {
             return $this->_properties[$key];
         }
         return null;
    }
}

$inbox = Inbox::getInstance();
$inbox->foo = 'bar';
$inbox->hello = 'world';
unset($inbox);

// $inbox does not longer exist

function newScope() {
     $box = Inbox::getInstance();
     echo $box->foo . $box->hello;
}

newScope(); // barworld

?>

[keeB]

That __get, __set stuff is so wrong. If the man wants to learn general OOP and not be completely confused if he ever wants to learn other languages, I suggest we help him the right way.

 

Also, generic get/set methods like that are extremely hard to decode and SO lazy.

 

Please read the following article on one of the most intelligent OOP designers out there:

http://www.javaworld.com/javaworld/jw-09-2003/jw-0905-toolbox.html

 

His book is equally amazing.

 

[pocobueno1388]

ignace - Wow, I am completely confused by that code. I can't even comprehend anything in the links provided, hah.

 

keeB - Okay, I understand how the get/set functions are wrong. I never had to use them in my code, so I will just get rid of that altogether. So the way I'm doing it is correct then (initiating a new instance of the class for each part of my script)? If I think about it, I guess it really doesn't matter that I'm creating a new instance as there would always be one being created regardless.

 

So do you guys see anything wrong with not setting many variables in the class? I mean, I just pass those ones through the parameters of the functions, and so far it hasn't caused me any problems. Then again, should functions have parameters if they are inside a class? Because if I did have those variables defined in the class, it would seem a little pointless to have parameters. I'm just confused on which way is correct....

[keeB]

poco -- There's one line in the article that should really stand out to you

 

Paraphrase:

Don't ask an object for information, rather ask the object to do something with information.

 

Passing objects which do something is much more reliable than passing a variable.

[pocobueno1388]

I'm not exactly sure what to take from that =/ Are you saying that my class does too much asking for information instead of doing stuff with it? I may be way off on that one

 

I don't know if I'm going to understand what your saying unless you actually show me with my code.

[keeB]

If you're going for a completely OO centric way, this is what you do (please remember this is a prototype, i am not going to write the actual code)

 

Writing the rest right now

[keeB]

<?php

interface MailReaderDao {

        public function retrieve($user, $folder);

}


public class MessageList {
        #container for messages
        private $items = array();

        public function add(Message $item) {
                $this->items[] = $item;
        }


        public function read($index) {
                return $this->items[$index];
        }

        public function readBySubject($subject) {
                # find a message by subject
        }
}

public class Message {
        public function __construct($raw) {
                # parse the message object and provide 
        }

        #read should take a parameter like where to draw itself

        public function display($where=null) {
                if (where=null) {
                        #write to stdout,
                        print $this->subject;
                        print $this->sender;
                } else {
                        # who knows, probably not necessary, you may not be writing to anywhere other than a 
                        #browser, but this is for the sake of using a pattern like MVC.
                }

        }
}


public class DatabaseMailReader implements MailReaderDao {

        private $db; # stores the database connection

        public function __contruct(DatabaseConnection $db) {$this->db = $db}


        #must define retrieve() method
        public function retrieve($user, $folder) {
                #return a messagelist object
                #db->query is an object which returns all results as an, normally you would
                #implement a factory (or something along those lines) which returns the MessageList 
                #for you. We can pretend that this method would be plugged in to said Factory.
                $q = "select subject, sender from inbox where userid = " . $user->userId;
                $result = $db->query($q);
                $list = new MessageList();
                foreach ($result as $message) {
                        $mObj = new Message($message);
                       $list->add($mObj);
                }

                return $list;
        }


}


?>

[keeB]

As you can see, I have broken down each part of the messaging system to encapsulate it in to different 'parts'

 

These parts usually have a single or really simple piece of the puzzle, and it is the cooperation between these that give you the ultimate flexibility.

 

We have templatized most of the messaging system for common goals which can be re-used and 'plugged' in elsewhere. Say you want your inbox to support UNIX file based MailDirs, an XML file, whatever. Your client code shouldn't care *where* this information is coming from or really what to do with it other than to tell it when to *do* something (like display itself.) This method of design allows for that flexibility.

 

 

[pocobueno1388]

Well, I must be way too much of an amateur still when it comes to OOP, because I'm not understanding at all. I'm not going to make you sit here and explain everything to me, it's obvious that I have much more research to do on the topic. Thank you so much for your feedback, I will refer back to it after I learn a bit more.

[keeB]

Feedback: Great entry in to the OOP world

 

My last word of advice on this topic.

 

You can't Do It Right from the beginning. Your class is fantastic. If you'll ever want to: reuse it, modify it , extend it,maintain it.. you'll then come to understand some of the limitations of your implementation and come to understand the reason I laid out the object above.

 

Truth is-- you won't understand this unless you have to go through it (or unless you're formally taught and given feedback.)

 

I've had to completely tear projects apart because I designed it poorly from the beginning. Thinking about design from the start and potential problems help alleviate this from the beginning