SitePoint Sponsor

User Tag List

Results 1 to 10 of 10
  1. #1
    SitePoint Enthusiast
    Join Date
    Feb 2008
    Location
    london, UK
    Posts
    51
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)

    PHP gurus - please critique my OOP programming

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

    I知 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知 making, and any code improvements I can make to optimise the way I code.

    I知 looking to implement some sort of error catch (which I致e attempted) but need guidance in this area.

    Code:
    <?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;
    		}
    	}
    	
    }

  2. #2
    SitePoint Addict
    Join Date
    Mar 2005
    Posts
    319
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    ill just post mine instead so you get an idea how others do it

    Code PHP:
    <?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;
    			}
    	}
     
    }
     
    ?>

  3. #3
    SitePoint Enthusiast
    Join Date
    Feb 2008
    Location
    london, UK
    Posts
    51
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    cheers man, ill take a look at that later on!

  4. #4
    SitePoint Wizard
    Join Date
    Mar 2008
    Posts
    1,149
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    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.)

  5. #5
    SitePoint Enthusiast
    Join Date
    Feb 2008
    Location
    london, UK
    Posts
    51
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by sk89q View Post
    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

  6. #6
    SitePoint Wizard bronze trophy
    Join Date
    Jul 2006
    Location
    Augusta, Georgia, United States
    Posts
    4,151
    Mentioned
    16 Post(s)
    Tagged
    3 Thread(s)
    Basic idea behind having the object store individual row data.

    PHP Code:
    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:
    PHP Code:
    $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
    HTML Code:
    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.

  7. #7
    SitePoint Wizard
    Join Date
    Mar 2008
    Posts
    1,149
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    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:
    PHP Code:
    $post->get(45);
    $post->delete(46); 
    PHP Code:
    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:
    PHP Code:
    $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.

  8. #8
    SitePoint Wizard bronze trophy
    Join Date
    Jul 2006
    Location
    Augusta, Georgia, United States
    Posts
    4,151
    Mentioned
    16 Post(s)
    Tagged
    3 Thread(s)
    PHP Code:
    $post_45 = new Post(45);
    $post_45->get(); 
    You might as well:

    PHP Code:
    $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.

    PHP Code:
    $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.

    PHP Code:
    $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.

  9. #9
    SitePoint Wizard bronze trophy
    Join Date
    Jul 2006
    Location
    Augusta, Georgia, United States
    Posts
    4,151
    Mentioned
    16 Post(s)
    Tagged
    3 Thread(s)
    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.

  10. #10
    SitePoint Enthusiast
    Join Date
    Feb 2008
    Location
    london, UK
    Posts
    51
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Cheers guys. A lot of helpful information
    Last edited by strungoutyeh; Apr 23, 2009 at 01:23.


Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •