PHP gurus - please critique my OOP programming

[strungoutyeh]

Hello all
I'm new to OOP. I'm trying to improve my coding skills & habits as much as possible.

I’m giving you a basic example of a dummy class to manage posts that has methods Save, Delete, Fetch All, fetch by id. I want you to point out any mistakes I’m making, and any code improvements I can make to optimise the way I code.

I’m looking to implement some sort of error catch (which I’ve attempted) but need guidance in this area.

<?php


class Post {

	var $error = NULL;

	
	function Post() {
	
	}


	
	function save($title, $post_date, $content, $categories, $status, $url) {
		if(get_magic_quotes_gpc()) {
			$title = stripslashes($title);
			$contact = stripslashes($contact);
		}
		
		//check required fields aren't empty
		$this->isEmpty($title);
		$this->isEmpty($post_date);
		$this->isEmpty($content);
		
		$name = mysql_real_escape_string($title);
		$email = mysql_real_escape_string($content);
		
		$query = "INSERT INTO ".TBL_POSTS."(title, post_date, content, date_created, status, url) VALUES('$title', '$post_date', '$content', '$date_created', '$status', '$url')";
		$result = mysql_query($query);
		$data = mysql_fetch_object($result);
		
		
	}
	
	
	
	function delete($id) {
		$query = "DELETE FROM ".TBL_POSTS." WHERE id = '$id";
		$result = mysql_query($query);
		if (!$result) {
			$this->error = 1;
		}
	}
	
	
	
	function isEmpty($field) {
		if ($field == NULL or $field == '') {
			$this->error = 1;
		}
	}
	
	
	
	function fetchAll() {
		$query = "SELECT * FROM ".TBL_POSTS;
		$result = mysql_query($query);
		$data = mysql_fetch_array($result);
		if (mysql_num_rows($result) == 0) {
			$this->error = 1;
		} else {
			return $data;
		}
	}
	
	
	
	function fetchAllPosted() {
		$query = "SELECT * FROM ".TBL_POSTS." WHERE post_date <= '$today'";
		$result = mysql_query($query);
		$data = mysql_fetch_array($result);
		if (mysql_num_rows($data) == 0) {
			$this->error = 1;
		} else {
			return $data;
		}
	}
	
	
	
	function get($id) {
		$query = "SELECT * FROM ".TBL_POSTS." WHERE id = '$id";
		$result = mysql_query($query);
		$data = mysql_fetch_array($result);
		if (mysql_num_rows($data) == 0) {
			$this->error = 1;
		} else {
			return $data;
		}
	}
	
	
	function getByUrl($url) {
		$query = "SELECT * FROM ".TBL_POSTS." WHERE url = '$url";
		$result = mysql_query($query);
		$data = mysql_fetch_array($result);
		if (mysql_num_rows($data) == 0) {
			$this->error = 1;
			return false;
		} else {
			return $data;
		}
	}
	
}

[azz0r_wugg]

ill just post mine instead so you get an idea how others do it

<?php
 
 
class Post {
 
 
	public $id;
	public $post;
	public $userId;
	public $threadId;
	public $Attachments;
	public $User;
	public $ip;
	public $updated;
	public $created;
 
	public $autoLoad;
 
 
	function __construct($id, $post=NULL, $userId, $threadId, $ip=NULL, $updated, $created, $username=NULL, $autoLoad=array()) {
		$this->id 				= $id;
		$this->post 			= stripslashes($post);
		$this->userId 			= $userId;
		$this->username			= !is_null($username) ? $username : NULL;
		$this->threadId			= $threadId;
		$this->ip				= $ip;
		$this->updated 			= $updated;
		$this->created			= $created;
		$this->autoLoad			= $autoLoad;
		$this->AutoLoad($this->autoLoad);
	}
 
 
	public static function CleanPosts($posts, $smileArray, $keywords=NULL) {
 
		if (is_array($posts)) {
			foreach($posts as $post) { 
				$post->post = nl2br($post->post);
				$post->post	= Forum_Code::ReturnFinalString($post->post,$post->User->username);
				$post->post = Smile::FindAndReplace($post->post, $smileArray);
				$post->User->Reputation = isset($post->User->Reputation) ? Reputation::Display($post->User->Reputation) : Reputation::Display(0);
 
				if (!is_null($keywords)) { 
					$post->post = preg_replace("/$keywords/i", '<span class="highlightString">'.$keywords.'</span>', $post->post);
				}
			}
		return $posts;
		}
	}
 
 
	public static function CleanPost($post, $smileArray) {
 
		$post->post = nl2br($post->post);
		$username = isset($post->User->username) ? $post->User->username : $post->username;
		$post->post	= Forum_Code::ReturnFinalString($post->post, $username);
		$post->post = Smile::FindAndReplace($post->post, $smileArray);
		$post->User->Reputation = isset($post->User->Reputation) ? Reputation::Display($post->User->Reputation) : Reputation::Display(0);
		return $post;
 
	}
 
 
	public static function Load($id, $autoLoad=array()) {
 
		try {
			$db = Database::GetInstance();
			$query = $db->query("SELECT p.*, u.username FROM Post p JOIN User u ON p.user_id = u.id  WHERE p.id = $id ") or die($db->error);
 
					if ($query && $query->num_rows > 0) {
						$q = $query->fetch_object();
 
						if ($Object = new self($q->id, $q->post, $q->user_id, $q->thread_id, $q->ip, $q->updated, $q->created, $q->username, $autoLoad)) {
							return $Object;
						}
 
					} else {
						throw new Exception_Application('Could not load postId '.$id);
					}
 
		} catch(Exception_Database $e) {
			throw new Exception_Application($e->getMessage(), $e->getCode());
		}
	}
 
 
	public function AutoLoad($autoLoad) {
 
		if (is_array($autoLoad)) {
			foreach($autoLoad as $load):
				switch($load):
					case 'attachments':
						$this->LoadAttachments();
					break;
					case 'is_first_post':
						$this->LoadIsFirstPost();
					break;
				endswitch;
			endforeach;
		} else if ($autoLoad === true) {
			$this->LoadAttachments();
			$this->LoadIsFirstPost();
		}
	}
 
 
	public function LoadAttachments() { 
		$this->Attachments = Attachment::LoadAllByPostId($this->id);
	}
 
 
	public function LoadIsFirstPost() {
		$db = Database::GetInstance();
		$query = $db->query("SELECT id FROM Post WHERE thread_id = ".$this->threadId." ORDER BY id ASC LIMIT 1");
		$result = $query->fetch_object();
		$this->isFirstPost = ($result->id == $this->id) ? true : false;
	}
 
 
	public function Delete() {
 
		try {
			$db = Database::GetInstance();
			$result = $db->query("DELETE FROM Post WHERE id = ".$this->id);
 
				if (!$result) {
					return false;
				} else if ($db->affected_rows >= 0) {
					Thread::RemovePost($this->threadId);
					User::RemovePost($this->userId);
					return true;
				} else {
					return false;
				}
 
		} catch(Exception_Database $e) {
			throw new Exception_Application($e->getMessage(), $e->getCode());
		}		
	}
 
 
	public function DeleteByThreadId($threadId) {
 
		try {
			$db = Database::GetInstance();
			$query = $db->query("SELECT id, user_id FROM Post WHERE thread_id = ".$threadId);
 
				while($q = $query->fetch_object()) {
					User::RemovePost($q->user_id);
				}
 
			return $db->query("DELETE FROM Post WHERE thread_id = ".$threadId);
 
		} catch(Exception_Database $e) {
			throw new Exception_Application($e->getMessage(), $e->getCode());
		}		
	}
 
 
	public function Save() {
 
		try {
			$db = Database::GetInstance();
			$post = Forum_Code::Prepare_Post($this->post);
			$result = $db->query("UPDATE Post SET post = ".$db->safe($this->post).", user_id = ".$this->userId.", thread_id = ".$this->threadId.", ip = '".$this->ip."', updated = ".time()." WHERE id = ".$this->id);
 
				if (!$result) {
					return false;
				} else if ($db->affected_rows >= 0) {
					return true;
				} else {
					return false;
				}
 
		} catch(Exception_Database $e) {
			throw new Exception_Application($e->getMessage(), $e->getCode());
		}		
	}
 
 
	public static function NewPostsCount($userId, $timestamp) {
 
		try {
			$db = Database::GetInstance();
			$query = $db->query("SELECT p.id  
			FROM Post p 
			INNER JOIN Thread t ON t.id = p.thread_id
			INNER JOIN Forum f  ON f.id = t.forum_id
 				WHERE ((f.require_access = 1 AND EXISTS (SELECT 1 FROM User_Data u WHERE u.user_id = $userId AND u.value = f.id AND u.key = 'ForumAccess' )) 
   			OR f.require_access = 0 )
			AND p.created > $timestamp ");
 
				if ($query) {
					return $query->num_rows;
				}
 
		} catch(Exception_Database $e) {
			throw new Exception_Application($e->getMessage(), $e->getCode());
		}
	}
 
 
	public static function ReturnByUserId($userId, $limit, $autoLoad=array()) {
	global $User;
 
		try {
			$db = Database::GetInstance();
			$query 	= $db->query("SELECT p.*, 
			t.id as t_id, t.title as t_title, t.forum_id as t_forum_id, t.group_id as t_group_id, t.icon_id as t_icon_id, t.views as t_views, t.posts as t_posts, t.locked as t_locked, t.sticky as t_sticky   
			FROM  Post p 
			JOIN Thread t ON t.id = p.thread_id
			JOIN Forum f  ON f.id = t.forum_id
			WHERE ((f.require_access = 1 AND EXISTS (SELECT 1 FROM User_Data u WHERE u.user_id = $User->id AND u.value = f.id AND u.key = 'ForumAccess' )) 
			OR f.require_access = 0) 
			AND p.user_id = $userId 
			ORDER BY p.updated DESC LIMIT $limit");
 
				if ($query && $query->num_rows > 0) {
				$posts = array();
					while ($q = $query->fetch_object()) {
						$Object = new self($q->id, $q->post, $q->user_id, $q->thread_id, $q->ip, $q->updated, $q->created, NULL, $autoLoad);
						$Object->Thread = new Thread($q->t_id, $q->t_title, $q->t_forum_id, $q->t_group_id, $q->t_icon_id, $q->t_views, $q->t_posts, $q->t_locked, $q->t_sticky);
						$posts[] = $Object;
					}
				return $posts;
				}
			return false;
 
		} catch(Exception_Database $e) {
			throw new Exception_Application($e->getMessage(), $e->getCode());
		}		
	}
 
 
	public static function LoadAllByThreadId($id, $from, $to, $autoLoad=array()) {
 
		try {
			$db = Database::GetInstance();
			$query = $db->query("SELECT p.*, u.username as u_username, u.postcount as u_postcount, u.last_seen as u_last_seen, u.created as u_created, u.status as u_status,  (SELECT COUNT(*) FROM Attachment WHERE post_id = p.id) as attachmentCount 
			FROM Post p 
			JOIN User u ON p.user_id = u.id 
			WHERE p.thread_id = $id 
			ORDER BY p.id ASC 
			LIMIT $from, $to") or die($db->error);
 
				if ($query && $query->num_rows > 0) {
					$results = array();
						while ($q = $query->fetch_object()) {
							$Object = new self($q->id, $q->post, $q->user_id, $q->thread_id, $q->ip, $q->updated, $q->created, $q->u_username, $autoLoad);
							$Object->User = new User($q->user_id, $q->u_username, NULL, NULL, $q->u_status, $q->u_postcount, NULl, NULL, NULL, $q->u_last_seen, $q->u_created, $autoLoad);
							if ($q->attachmentCount > 0 && in_array('attachments', $autoLoad)) {
								$Object->Attachments = Attachment::LoadAllByPostId($q->id);
							}					
							$posts[] = $Object;
						}
					return $posts;
				}
 
			return false;
 
		} catch(Exception_Database $e) {
			throw new Exception_Application($e->getMessage(), $e->getCode());
		}		
	}
 
 
	public static function LoadLastPostInThread($id, $autoLoad=array()) {
	global $User;
 
		try {
			$db = Database::GetInstance();
			$query = $db->query("SELECT p.*, u.username as u_username, u.postcount as u_postcount, u.last_seen as u_last_seen, u.created as u_created, u.status as u_status,  (SELECT COUNT(*) FROM Attachment WHERE post_id = p.id) as attachmentCount 
			FROM Post p 
			JOIN User u ON p.user_id = u.id 
			WHERE p.id = $id 
			ORDER BY p.id DESC 
			LIMIT 1") or die($db->error);
 
				if ($query && $query->num_rows > 0) {
					$results = array();
						while ($q = $query->fetch_object()) {
							$Object = new self($q->id, $q->post, $q->user_id, $q->thread_id, $q->ip, $q->updated, $q->created, $q->u_username, $autoLoad);
							$Object->User = new User($q->user_id, $q->u_username, NULL, NULL, $q->u_status, $q->u_postcount, NULl, NULL, NULL, $q->u_last_seen, $q->u_created, $autoLoad);
							if ($q->attachmentCount > 0 && in_array('attachments', $autoLoad)) {
								$Object->Attachments = Attachment::LoadAllByPostId($q->id);
							}					
							return $Object;
						}
				}
			return false;
 
		} catch(Exception_Database $e) {
			throw new Exception_Application($e->getMessage(), $e->getCode());
		}		
	}
 
 
	public static function Create($body, $userId, $threadId, $ip) {
 
		try {
			$db 		= Database::GetInstance();
			$post 		= Forum_Code::Prepare_Post($body);
			$time		= time();
			$return 	= $db->query("INSERT INTO Post (post, user_id, thread_id, ip, updated, created) VALUES (".$db->safe($body).", $userId, $threadId, '$ip', $time, $time ) ");
			$id 		= $db->insert_id;
 
				if ($id > 0) {
					User::AddPost($userId);
					Thread::AddPost($threadId);
					return $id;
				} else {
					return false;
				}
 
		} catch(Exception_Database $e) {
			throw new Exception_Application($e->getMessage(), $e->getCode());
		}		
	}
 
 
	public function GetPageNumber($perPage) {
 
		$db = Database::GetInstance();
		$query = $db->query("SELECT * FROM Post WHERE created < ".$this->created." AND thread_id = ".$this->threadId);
		$count = $query->num_rows;
 
			if ($perPage == $count) {
				return 2;
			} else {
				$page = ceil($count / $perPage);
				return ($page == 0) ? 1 : $page;
			}
	}
 
}
 
?>

[strungoutyeh]

cheers man, ill take a look at that later on!

[sk89q]

You're using classes as namespaces and not as actual objects. If you want to write OOP code, then you would make the Post class actually represent a single individual post.

If you don't understand, then take it this way: never return an array from a function. Return an object. The object will take the place of that array. You can use arrays privately within an object, but it should never leave the object. (We're only avoiding arrays for the sake of the lesson.)

[strungoutyeh]

You're using classes as namespaces and not as actual objects. If you want to write OOP code, then you would make the Post class actually represent a single individual post.

If you don't understand, then take it this way: never return an array from a function. Return an object. The object will take the place of that array. You can use arrays privately within an object, but it should never leave the object. (We're only avoiding arrays for the sake of the lesson.)

Yes, i understand what you mean. I understand the concept of OOP but i don't know how to write for it yet - i think a couple of tutorials have led me astray.

Thanks for the help

[oddz]

Basic idea behind having the object store individual row data.

class Post {

    protected $data;

    public function __construct() {
    
        $this->data = array();
    
    }
    
    
    public function setProperty($name,$value) {
        
        if($this->hasProperty($name)===true) {
        
            array_unshift($this->data[$name],$value);
                
        } else {
                
            $this->data[$name] = array($value);
            
        }
        
    }
    
    public function getProperty($name,$index=0) {
    
        if($this->hasProperty($name)===true) {
        
            if(array_key_exists($index,$this->data[$name])) {
            
                return $this->data[$name][$index];
            
            } else {
                
                throw new Exception('Index '.$index.' does not exists for property '.$name.'.');
            
            }
        
        } else {
        
            throw new Exception('Property '.$name.' does not exists.');
        
        }
    
    }
    
    public function hasProperty($name) {
    
        return array_key_exists($name,$this->data)?true:false;
    
    }


} 

simple case:

$post = new Post();

$post->setProperty('title','My Fun Post');
$post->setProperty('title','Didn\'t like the first title.');
$post->setProperty('title','Changed again');
$post->setProperty('title','I\'ve decided the title should be this');

echo '<pre>',print_r($post),'</pre>'; 

object dump

Post Object
(
    [data:protected] => Array
        (
            [title] => Array
                (
                    [0] => I've decided the title should be this
                    [1] => Changed again
                    [2] => Didn't like the first title.
                    [3] => My Fun Post
                )

        )

)
1

Its necessary to store the history so that the the system can recall which properties have changed for saves. This also makes it possible to revert to the original if the object is representative of a actual row in a database table. You can also add the magical __set and __get methods which most people do.

Putting that aside your implementation seems a little to basic to really be useful. Furthermore, you'll come to realize that rewriting all that code for each entity really sucks. Its much better to abstract common functionality so that rewriting the find,save and delete isn't necessary for every entity of the database.

[sk89q]

azz0r_wugg's class is kind of what you want (he's also using his class as a namespace...), and so is oddz's class (although his is very generic).

In your original code, you have to pass around the $id, so there's no real difference in methodology between these two snippets of code:

$post->get(45);
$post->delete(46); 

get_post(45);
delete_post(46); 

$post (or Post) is now a psuedo-namespace, because you don't have to have a get_post function, a delete_post function, etc. foul the global namespace.

However, the OOP way is like this:

$post_45 = new Post(45);
$post_45->get();
$post_46 = new Post(46);
$post_46->delete(); 

Now you don't have to pass around an $id. $post_45 wholely represents only post #45, and any methods you perform on it will automatically do it on post #45.

[oddz]

$post_45 = new Post(45);
$post_45->get(); 

You might as well:

$post_45 = new Post(45); 

Where this will run something like: SELECT x,y,z FROM posts WHERE pk = 45. Then it will automatically fill that object with properties x,y and z. If the record isn't found you would throw a exception. Doing this of course means your going more the active record route.

In an y instance calling ->get() afterwards just seems like unnecessary code unless your going with a gateway.

$userGateway = new UserGateway($db);
$user = $userGateway->get(45); 

Something you can also think about is the ability to fill the object upon creation if its a new entity.

$post = new Post(array(
  'title'=>'my post'
  ,'created'=>time()
  ,'content'=>'blah blah blah...'
)); 

You just check whether the argument is a array or not. if its a array then your only creating a in memory entity. Otherwise, you fetch the one in the database and create its in memory domain representation.

[oddz]

I would also highly recommend using polymorphism and inheritance here. There are certain repetitive tasks that be encapsulated in one method. Doing so cuts down on code and makes extendability and management much easier down the line.

[strungoutyeh]

Cheers guys. A lot of helpful information