13 {{ upvoteCount | shortNum }}
13 {{ upvoteCount | shortNum }}

PHP gurus - please critique my OOP programming

by strungout
11 replies
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.

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;
        }
    }
    
}
#critique #gurus #oop #php
  • Profile picture of the author rocketsites
    One thing I noticed is you arent using the PHP5 way of declaring the constructor function. Instead of having function called post(), you should use the php5 way of doing it (assuming you are running php5) which is:

    public function __construct() {

    }

    and you should try to declare the scope of each function (public or private) as well as scope of variables for example protected vs public vs private.
    {{ DiscussionBoard.errors[717172].message }}
  • Profile picture of the author Adaptive
    I'm new to OOP. I'm trying to improve my coding skills & habits as much as possible.
    Good for you!

    I recommend you design just the interface of your objects. Don't yet code all the methods until you're sure that you have the methods organized correctly.

    Let's say that Posts are inside Threads which are inside a Forum.

    Forum.NewThread returns a Thread
    Forum.DeleteThread(ThreadID) removes the reference to the Thread, then deletes the Thread

    Thread.NewPost returns a Post
    Thread.DeletePost(PostID) removes the reference to the Post, then deletes the Post

    Post.ParentThread refers to the thread which contains the Post

    Forum.GetPostByPostID(PostID) returns a Post

    You could have Thread.GetAllPosts which returns all the Posts in the thread, but is this really needed? You should be able to do ForEach Post in Thread to iterate. Not sure if php has foreach iteration.

    You could have Forum.GetAllPosts, but again, better to iterate the threads and then iterate the posts within theforum.

    There are two times that you might want to save a Post: newly created post is populated, or edited post is updated.

    To create a new post,
    newPost = Thread.NewPost
    newPost.User = username
    newPost.Subject = subjectline
    newPost.Body = editfieldcontents

    But the post is just a draft at this point

    Thread.AppendPost(newPost) actually puts the new post into the thread

    For editing the post:
    newPost.Subject = subjectline
    newPost.Body = editfieldcontents
    (user is still the same, no need to update this)
    Put the changes inside a transaction and you'll avoid an inconsistent state when people could see a half-edited post.

    Regards,
    Allen
    Signature

    Success only requires four words. http://www.warriorforum.com/blogs/ad...our-words.html

    {{ DiscussionBoard.errors[717230].message }}
  • Profile picture of the author n7 Studios
    For error handling, consider using PHP5's try ... catch blocks and extending the Exception handler.

    You can also make PHP errors silent, by prepending functions with @, but test the function results to then throw your own errors (for example, you'd do an if (!@mysql_query($query)) { throw new Exception("Database Error"); } - but you can be a lot more sophisticated than that!

    Also don't forget to use a destructor (function __destruct()) to cleanup variables when your class is destroyed / unset.

    What you've done so far looks good though.
    Signature
    THE Web Development Guru - Bespoke WordPress Theme, Plugin + iPhone Apps [HTML5, CSS3, PHP, MySQL].
    {{ DiscussionBoard.errors[717564].message }}
  • Profile picture of the author stevenh512
    If that's your first attempt at OOP I'd say you're definitely on the right track.. or at least you have a better understanding of it than I did when I first started learning OOP.. lol

    You're getting some good advice and tips so far, really try to get your head around what Adaptive said, a lot of OOP concepts are about designing a good interface to your objects so you can hide the implementation (basically, if anyone is using a class you wrote they shouldn't need to know or care about what's under the hood).

    I notice this is your first post, welcome to the WF. This is a more marketing-oriented forum, there are certainly people here who can help you (I know we have quite a few programmers here) and this is a great place to learn, but you might also want to look for forums that are focused more on programming.
    Signature

    This signature intentionally left blank.

    {{ DiscussionBoard.errors[717967].message }}
    • Profile picture of the author strungout
      Thanks guys, i understanding now that i was basically programming in PHP4 rather than PHP5. What you said makes a lot of sense, i just need to get my head around it and ill be sweet

      Originally Posted by stevenh512 View Post

      If that's your first attempt at OOP I'd say you're definitely on the right track.. or at least you have a better understanding of it than I did when I first started learning OOP.. lol

      You're getting some good advice and tips so far, really try to get your head around what Adaptive said, a lot of OOP concepts are about designing a good interface to your objects so you can hide the implementation (basically, if anyone is using a class you wrote they shouldn't need to know or care about what's under the hood).

      I notice this is your first post, welcome to the WF. This is a more marketing-oriented forum, there are certainly people here who can help you (I know we have quite a few programmers here) and this is a great place to learn, but you might also want to look for forums that are focused more on programming.
      Yeah i have a basic understanding of OOP i just need to practice more. Thought i better start off on the right track

      I posted this exact same thread across 2-3 other forums just to get a wide range of opinions. Ill be using this forum for marketing purposes hopefully very soon
      {{ DiscussionBoard.errors[718105].message }}
  • Profile picture of the author Neil Morgan
    If there is functionality that is common to all your objects then you could create a "base" object that contains the common stuff.

    When you create a new object you "extend" it from the "base" object.

    The new object then has all the properties and methods of its ancestor.

    class MyNewObject extends MyBaseObject {
    blah, blah
    }

    Doing it this way means that you can more easily add and maintain properties and functionality that are common to all your objects, typically things like createStamp, updateStamp, deleteByID, getByID and so on.

    Note - if the base object's constructor contains stuff that you need to happen, then the constructor of the new object needs to manually call the constructor of the base object.

    Cheers,

    Neil
    Signature

    Easy email marketing automation without moving your lists.

    {{ DiscussionBoard.errors[718045].message }}
    • Profile picture of the author strungout
      Originally Posted by Neil Morgan View Post

      If there is functionality that is common to all your objects then you could create a "base" object that contains the common stuff.

      When you create a new object you "extend" it from the "base" object.

      The new object then has all the properties and methods of its ancestor.

      class MyNewObject extends MyBaseObject {
      blah, blah
      }

      Doing it this way means that you can more easily add and maintain properties and functionality that are common to all your objects, typically things like createStamp, updateStamp, deleteByID, getByID and so on.

      Note - if the base object's constructor contains stuff that you need to happen, then the constructor of the new object needs to manually call the constructor of the base object.

      Cheers,

      Neil
      Oh yeah i've heard of that before. is it called encapsulation or something? inheritence?

      How would i call a MyBaseObject method from inside MyNewObject? would it be:

      $MyNewObject::someMethod();

      ???
      {{ DiscussionBoard.errors[718119].message }}
      • Profile picture of the author Neil Morgan
        Inheritance.

        From inside the child, you reference properties and methods of the parent like this:

        parent::__construct();

        And to the outside world (ie outside the child), the parent's properties and methods will simply exist are if they were defined within the child and can therefore be referenced in the same way as the child's own properties and methods.

        Cheers,

        Neil
        Signature

        Easy email marketing automation without moving your lists.

        {{ DiscussionBoard.errors[718261].message }}
  • Profile picture of the author Neil Morgan
    Something else I thought of.

    You might want to break the MySQL stuff out into an object of its own for a few reasons:

    1. It's easier to build and also maintain the application when low-level stuff such as database functionality is separate from the "business" side of your app.

    2. Gives you the opportunity to more easily port to a different database platform in the future (if that is important to you).

    3. It prevents errors creeping into your database code which you're repeating all over the place.

    4. You'll have a class that you can use in future projects.

    This idea can obviously be extended to other low-level stuff such as web page layout, file management, data sanitization etc, etc, etc.

    Cheers,

    Neil
    Signature

    Easy email marketing automation without moving your lists.

    {{ DiscussionBoard.errors[718072].message }}
  • Profile picture of the author Plinko
    Something I've found useful is to develop or adapt a Coding Standards Document for myself which defines code formatting, function return standards, etc. If you use one on every project, and improve upon it, you'll notice over time that when you revisit something you can come to expect the same format for each one, which usually I find lessens my re-acquaintance time
    {{ DiscussionBoard.errors[719590].message }}
  • Profile picture of the author yosis
    Excellent tip Plinko. Since I work with frameworks often I make sure I have a good grasp of the coding standards before any real work begins. When I work on my own code I stick fairly close to PEARs coding standards as I think they fit most needs and it means I don't have to reinvent the wheel.

    strungout, well done. I would separate the database functionality from your class and set up the instantiate the data access class from within the Post constructor.
    {{ DiscussionBoard.errors[720168].message }}

Trending Topics