SitePoint Sponsor |
|
User Tag List
Results 1 to 10 of 10
-
Apr 21, 2009, 06:02 #1
- 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; } } }
-
Apr 21, 2009, 06:20 #2
- 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; } } } ?>
-
Apr 21, 2009, 06:30 #3
- 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!
-
Apr 21, 2009, 08:23 #4
- 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.)
-
Apr 21, 2009, 08:35 #5
- Join Date
- Feb 2008
- Location
- london, UK
- Posts
- 51
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
-
Apr 21, 2009, 09:16 #6
- Join Date
- Jul 2006
- Location
- Augusta, Georgia, United States
- Posts
- 4,194
- Mentioned
- 17 Post(s)
- Tagged
- 5 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;
}
}
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>';
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
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.
-
Apr 21, 2009, 16:01 #7
- 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);
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();
-
Apr 21, 2009, 16:13 #8
- Join Date
- Jul 2006
- Location
- Augusta, Georgia, United States
- Posts
- 4,194
- Mentioned
- 17 Post(s)
- Tagged
- 5 Thread(s)
PHP Code:$post_45 = new Post(45);
$post_45->get();
PHP Code:$post_45 = new Post(45);
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);
PHP Code:$post = new Post(array(
'title'=>'my post'
,'created'=>time()
,'content'=>'blah blah blah...'
));
-
Apr 21, 2009, 16:21 #9
- Join Date
- Jul 2006
- Location
- Augusta, Georgia, United States
- Posts
- 4,194
- Mentioned
- 17 Post(s)
- Tagged
- 5 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.
-
Apr 22, 2009, 06:12 #10
- 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