First PHP class for review/advice

Discussion in 'Web Design and Development' started by Cabbit, Apr 20, 2009.

  1. Cabbit macrumors 68020

    Cabbit

    Joined:
    Jan 30, 2006
    Location:
    Scotland
    #1
    I have made a basic class that can take values from a form post, these can be in any combination and to any table and it will post the new record to the database.

    Note: i have some validation functions i have not yet added.

    the call on the php side
    PHP:
    /************************* Post record *******************************/
        // Posts the record to the database
        
    $post = new newrecord;
        
            
    // Tbl Name
            
    $post->tbl_name "forum-posts";

            
    // Send variables
            
    $post->postid $_POST['postid']; // this is only used if it is something like a comment that relates to another object
             
    $post->subject $_POST['subject'];
            
    $post->body $_POST['body'];

            
    // Display output
             
    $post->PostToDatabase();
    the class
    PHP:
    <?php
    /************************************************************************************
                                     Jerry Roy
                                     KittenbunnyCore
                                     Database Class: Add new record 
    ************************************************************************************/

    // Prevent direct access to file //
    if(eregi(basename(__FILE__),$_SERVER['REQUEST_URI']))
    die(
    '<h1>Forbidden</h1><p>Direct access prohibited.</p>');

    // Class deals with the following components

    // --- Tbl --- //
    // tbl_name

    // --- DATA --- //
    // PostID (relates for forum replys)
    // Subject
    // Body
    // Date
    // Poster

    class newrecord 
    {
        
    // Variables
        
    var $tbl_name;
        var 
    $postid;
        var 
    $subject;
        var 
    $body;
        var 
    $date;
        var 
    $poster;

        
    // Validate the Tbl_name
        
    function tbl_name() 
        {
            if (isset(
    $this->tbl_name))
            {
                 return 
    ' `'.$this->tbl_name.'`';
            }
      }

        
    // Validate the PostID
        
    function ValidatePostID() 
        {
            if (isset(
    $this->postid))
            {
                    
    $valid_postid $this->postid;
                 return 
    $valid_postid;
            }
      }

        
    // Validate the subject
        
    function ValidateSubject() 
        {
            if (isset(
    $this->subject))
            {
                 return 
    $this->subject;
            }
      }

        
    // Validate the Body
        
    function ValidateBody() 
        {
            if (isset(
    $this->body))
            {
                 return 
    $this->body;
            }
      }
        
        
    // Validate the Date
        
    function ValidateDate() 
        {
            if (isset(
    $this->date))
            {
                 return 
    $this->date;
            }
      }
        
        
    // Validate the Poster
        
    function ValidatePoster() 
        {
            if (isset(
    $this->poster))
            {
                 return 
    $this->poster;
            }
        }

        
    // Make the sql string
        
    function MakeSQL()
        {
            
            
    // Varibles
            
            // Feilds
            
    $field['0'] = '`post_id`';
            
    $field['1'] = '`subject`';
            
    $field['2'] = '`body`';
            
    $field['3'] = '`date`';
            
    $field['4'] = '`poster`';

            
            
    // Values
            
    $value['0'] = $this->ValidatePostID();
            
    $value['1'] = $this->ValidateSubject();
            
    $value['2'] = $this->ValidateBody();
            
    $value['3'] = $this->ValidateDate();
            
    $value['4'] = $this->ValidatePoster();
            
            
    //******************** INSERT string *******************************//

            // Build the array
            
    $x 0;
            foreach (
    $field as $key)
            {
                if (isset(
    $value[$x]))
                {
                    
    $strArray[$x] = $field[$x];
                }
                
    $x++;
            }
            
            
    // Build the array
            
            
    $queryStr ''
            
    $andStr ', '
            
            
    // Build the string
            
    foreach($strArray as $str
            { 
                if(!empty(
    $str)) 
                { 
                    
    $queryStr $queryStr $str $andStr
                } 
            }     
                    
            
    // Remove last $andStr added to str 
            
    $queryStr substr($queryStr0strlen($queryStr)-strlen($andStr));
            
            
    // Build the query
            
            // Start the query
            
    $start_return '$sql = sprintf("INSERT INTO '.$this->tbl_name().'';
            
            
    // tables
            
    $field_return ' ('.$queryStr.') ';
            
    //******************** INSERT string *******************************//

            //******************** Value string *******************************//
            
            // Values
            // Build the array
            
    $x 0;
            foreach (
    $field as $key)
            {
                if (isset(
    $value[$x]))
                {
                    
    $strArray[$x] = '%s';
                }
                
    $x++;
            }
            
    $queryStr ''
            
    $andStr ', '
            
            
    // Build the string
            
    foreach($strArray as $str
            { 
                if(!empty(
    $str)) 
                { 
                    
    $queryStr $queryStr $str $andStr
                } 
            }     
            
            
    // Remove last $andStr added to str 
            
    $queryStr substr($queryStr0strlen($queryStr)-strlen($andStr));
            
    $values_return 'VALUES ('.$queryStr.')", ';
            
    //******************** Value string *******************************//
            
            //******************** Data String ********************************//
            
            
    $x 0;
            foreach (
    $field as $key)
            {
                if (isset(
    $value[$x]))
                {
                    
    $strArray[$x] = mysql_real_escape_string($value[$x]);
                }
                
    $x++;
            }
            
            
    // Build the array
            
            
    $queryStr ''
            
    $andStr ', '
            
            
    // Build the string
            
    foreach($strArray as $str
            { 
                if(!empty(
    $str)) 
                { 
                    
    $queryStr $queryStr $str $andStr
                } 
            }     
                    
            
    // Remove last $andStr added to str 
            
    $queryStr substr($queryStr0strlen($queryStr)-strlen($andStr));     
            
    $input_return $queryStr;
            
            
            
    //******************** Data String ********************************//
            
            //Close
            
    $close_return ');';
            
            
            
    // Build up the SQL String
            
    return $start_return $field_return $values_return $input_return $close_return;
         
        }
        
        
    // Return the results
      
    function PostToDatabase() 
        {
             
    $query mysql_query($this->MakeSQL());
                if(!
    $query)
                {
                    echo 
    "There was a error entering the data into the database, please report this issue to the site admin.";
                }
                else
                {
                    echo 
    "The post was successful";
                }
           }

    }
     
    ?>
     
  2. Mr.nix macrumors member

    Joined:
    Jan 30, 2007
    #2
    I understand you're just starting out with classes but this is pretty bad. For starters I would just looks at some of the simple PHP. Read up on how foreach loops work.
     
  3. Cromulent macrumors 603

    Cromulent

    Joined:
    Oct 2, 2006
    Location:
    The Land of Hope and Glory
    #3
    You shouldn't use the var keyword before variables anymore really, also you haven't stated which of your variables or functions are public or private (or protected).

    Maybe do some reading on object orientated programming methodologies?
     
  4. SrWebDeveloper macrumors 68000

    SrWebDeveloper

    Joined:
    Dec 7, 2007
    Location:
    Alexandria, VA, USA
    #4
    Here's some honest feedback...

    First, bravo and a golf clap for you for taking the time to write your solution in OOP format, meaning using a PHP class. I also see nothing obviously wrong with the class and it's basic purpose, to add a new record to the database and in the future validate various parsed fields, etc. Great job using mysql_real_escape_string for SQL injection protection.

    My comment is more on a general nature about what classes are best used for, and how you might improve the concept of using OOP for things like this.

    First off, the main reason classes are used is for portability and scalability of the code, in my experience. By no means the only reasons, but those two rank up there in my book. Portability refers to the class being shared by other developers, including yourself, for other projects and even in some cases other platforms. Integrating a series of functions to get a task done with minimal amount of code, and using objects to plugin your class into the code of others. Scalability refers to easily adding and extending existing functionality of the class - including the power of overriding functions. This means another author, or yourself, could add in child classes with functions that override your main class, saving alot of code by re-writing your class due to a minor behavior change.

    In your example, the class saves some code by handling the validation of passed arguments and then handling 100% of the database insertion. But I want you to ask an honest question of yourself, was a class necessary to accomplish that? Why not just add a few functions at the bottom of your source or include them in some simple .inc file or whatever? In your case, both would serve equally well.

    So what I'm saying is, when writing a class - try to see it not as just a way of condensing code. Specifically, I will suggest the following improvements so the class has more of the two things every class should have, portability and scalability:


    * Add more set functions which include database name, host, username, password, and MD5 functions for using encrypted values of password.

    * Make output optional, or return as a string or even an array which includes MySQL error status along with a success or failure. Your project calling the class should handle HTML output, allow the class to do the "inner" workings.

    *Add a function for debugging or class introspection. PHP has great commands to list class functions and internal variables, plus add some timing values to see how long it takes for the class to run.


    Get the idea?

    Such changes means you can re-use the class for numerous other projects and not be so locked into a proprietary use i.e. this one specific forum, that one specific database. Imagine someone using your class to post, you want them to be able to customize these things. Even if it's you, later on!

    However, again, I wish to compliment you for being a brave soul and posting code using OOP. No matter what I said here, I respect your efforts and recognize the work that went into it. I encourage to keep going at it, and making more complex classes and start building an internal library. You might even add a few to phpclasses.org or maybe a hacker's area for the forum you're running. To the end user, once a class has all the proper functions, they just include your class file, initiate the object and write a few lines of code.

    My goal in writing this is to see if you can note the conceptual difference, the idealogical way of looking at OOP, between what your current class does and its potential. I know this is a small project so do not feel the need to explain, justify or defend -- consider this a discussion of concept. From a coding perspective, you're doing great!

    Bravo!!! :) :) :)

    -jim
     
  5. savar macrumors 68000

    savar

    Joined:
    Jun 6, 2003
    Location:
    District of Columbia
    #5
    For a noob this isn't bad, but it's far from being production quality.

    A few comments:

    1) Standardize on a naming convention for variables, classes, and methods (I suggest the standard which is used by Zend Framework: http://framework.zend.com/manual/en/coding-standard.html)

    2) You have way too many comments.

    3) You don't have enough comments.

    What I mean by #2 and #3 is that your comments are in the wrong places. You have several places where there's one or two lines of comment for just one line of code.

    On the other hand, you don't have enough comments on the class itself, the methods, or the instance variables. Take a look at PhpDocumentor (http://www.phpdoc.org/).

    4) Read "Code Complete". This is an excellent book that will teach you many programming principles that you won't learn out of a "Learn PHP in 24 Hours" or "PHP for Dummies" books. Books like that can be useful for novices, but they are insulting to anybody who designs software professionally.

    5) Keep trying... the fact that you're asking for people to review your code is a VERY GOOD THING. It reflects your interest in quality and that immediately sets you apart from most people who teach themselves a programming language.
     
  6. SrWebDeveloper macrumors 68000

    SrWebDeveloper

    Joined:
    Dec 7, 2007
    Location:
    Alexandria, VA, USA
    #6
    Great suggestions! Just wanted to add that the var keyword is technically deprecated in favor of private/public (and protected) declaration types -- for those running PHP5. And if you run PHP5 (Zend-2), check out the new constructor and destructor naming conventions too, i.e. __construct() and __destruct() instead of naming a function the same name as the class as in PHP4 as the constructor. The __destruct() is called automatically when the object is released.

    -jim
     
  7. sbauer macrumors member

    Joined:
    Feb 7, 2009
    Location:
    Baltimore, MD
    #7
    Not a bad first attempt. I have a few pieces of advice.

    One of the main goals of OOP is to reduce software maintenance costs by effectively reusing code, and writing maintainable code. When all of these factors are in place, your code will be much easier to read, and it won't be duplicated throughout your project. When I look at your class, I feel like it needs to be cleaned up.

    - First of all, the intentions of that class are not very clear to me. What is a newrecord? How do you recall that newrecord? If you recall a record and store it into an instance of a newrecord, it's not a "new record" anymore, and doesn't really make sense. If you're storing previously recorded data in a different class (ex. oldrecord), I feel a lot of duplication coming on.

    - Your class is doing way too much. It's validating. It's communicating with a database. It's also displaying information. The more responsibilities you give to a class, the more difficult it is to maintain. A class' scope should be limited. Your class' scope is stretched across three distinct layers: presentation (echo 'whatever'), business (validation) and data (mysql communication). If your class was designed to handle business logic, that's fine. Let it do that responsibility. The moment you ask it to create database connections and display data is the moment where things start to get crazy. ( http://en.wikipedia.org/wiki/Single_responsibility_principle - Single Responsibility Principle)

    - Your class is a leaky abstraction, and seems very awkward to use. As an example, we're going to use your class to save contact information to a database. Our site has a contact form with a few form fields that allow us to collect contact information from our customer. After setting up our form (our presentation layer), we used your class (business layer) and initiated our fields. Here's the problem, though. Why should our presentation ever know what table name to use? Another part of the system should know that, not the presentation layer. If we ever decided to refactor that table, or code, we would need to modify every page that references the table. That may include the contact form, the user's contact status page, the admin form to display all contacts currently in the database, an admin form used to display contact details, and a form designed to delete/update contacts that have been dealt with.

    If I were to improve your code, I would create a separate class called forum posts. In addition, I would also create one for thread, user and whatever else is needed. The abstractions would then understand what it needed, and wouldn't have to be so general.

    Hope that helps.
     
  8. Cabbit thread starter macrumors 68020

    Cabbit

    Joined:
    Jan 30, 2006
    Location:
    Scotland
    #8
    Thanks for all the advise guys i am currently looking into it i had someone on another forum rewrite the class in php 5 code but it does not function as the original though it seems to be a better solution and i am poking though it to see what all the new things are.

    From what i can see it does everything i want it to but does not work if all the fields are not provided and adds a extra , .

    Code:
    You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'postid`,`subject`,`body`,`date`,`poster``) VALUES('','test subject','test body',' at line 1
    
    A few things i am looking at are what functionality public and private provides and how these are used, also extends for using two classes.
    What the heck return implode does that sounds rather scary.

    Though over all he was just able to make it do the same thing as what mine done with less code. I would really like to know what the term OOP means. And am interested to learn about the uses of classes and why one would not make them hugh do everythings. I like huge do everythings they are easy to remember. ^_^

    Note: its deliberately broken for me to work out how it works so i can fix it.
    PHP:
    <?php
    /************************************************************************************
                                     Jerry Roy
                                     KittenbunnyCore
                                     Filename: newrecord.class.php
                                     Database Class: Add new record 
    ************************************************************************************/

    // Prevent direct access to file //
    if(eregi(basename(__FILE__),$_SERVER['REQUEST_URI']))
    die(
    '<h1>Forbidden</h1><p>Direct access prohibited.</p>');

    class 
    newrecord
    {
       
    // Variables
       
    private $tbl_name;
       private 
    $fields = array(
          
    'postid'    => null,
          
    'subject'   => null,
          
    'body'      => null,
          
    'date'      => null,
          
    'poster'    => null
       
    );

       
    /**
        * Set the table name
        * @param string $name
        * @return bool
        */
       
    public function set_tbl_name($name)
       {
          
    $this->tbl_name trim($name);
          return (bool)
    $this->tbl_name// false if set to empty string
       
    }

       
    /**
        * Set $fields values
        * Uses the __set() "magic" method
        * @param string $key
        * @param mixed $value
        * @return bool
        */
       
    public function __set($key$value)
       {
          if(
    array_key_exists($key$this->fields))
          {
             
    $this->fields[$key] = trim($value);
             return 
    true;
          }
          else
          {
             
    user_error("Invalid field '$key'");
             return 
    false;
          }
       }

       
    /**
        * Do the insert
        * @return bool
        */
       
    public function PostToDatabase()
       {
          
    $sql $this->MakeSQL();
          
    $query mysql_query($sql);
          if(!
    $query)
          {
             
    error_log(mysql_error()."\n$sql");
          }
          return (bool)
    $query;
       }

       
    /**
        * Create insert SQL string
        * @return string
        */
       
    private function MakeSQL()
       {
          
    $sql sprintf(
             
    "INSERT INTO `%s` (`%s`) VALUES(%s)",
             
    $this->tbl_name,
             
    $this->field_list(),
             
    $this->value_list()
          );
          
    $query mysql_query($sql);
          if(!
    mysql_query($sql))
            {
                echo 
    mysql_error();
            }
       }

       
    /**
        * Create list of fields for use in SQL
        * @return string
        */
       
    private function field_list()
       {
          
    $fields = array();
          foreach(
    $this->fields as $key => $val)
          {
             
    $fields[] = "`$key`";
          }
          return 
    implode(','$fields);
       }

       
    /**
        * Create list of values for use in insert SQL
        * @return string
        */
       
    private function value_list()
       {
          
    $values = array();
          foreach(
    $this->fields as $value)
          {
             if(
    is_numeric($value))
             {
                
    $values[] = $value;
             }
             else
             {
                
    $values[] = "'".mysql_real_escape_string($value)."'";
             }
          }
          return 
    implode(','$values);
       }
    }
     
    ?>
     
  9. SrWebDeveloper macrumors 68000

    SrWebDeveloper

    Joined:
    Dec 7, 2007
    Location:
    Alexandria, VA, USA
    #9
    OOP = Object Oriented Programming and the other respondents covered the concepts and best practices in a professional environment very well. Be it you're a hobbyist or pro, following those guidelines is the key to quality OOP, and it'll make you a better coder overall.

    Now, about the MySQL error, you'll notice it returns a portion of the query relevant to the error. Might be as simple as an single quote before postid but a tick following it, or an extra tick after the field named poster is located in the query? Or maybe you need to add a space after VALUES and before the next parenthesis that follows? Or all of the above!

    If those suggestions don't work what I often do to analyze the query is add some debugging code to output a fully parsed/formatted string which contains that query exactly as MySQL would see it. Then if I still can't figure it out, i.e. some wacky MySQL syntax thing going on, I copy/paste and run the query within phpMyAdmin or whatever interface is handy to really see what's going on as you have convenient access to the table structure, error codes, etc. Just an informal suggestion, to each their own.

    -jim
     
  10. Cabbit thread starter macrumors 68020

    Cabbit

    Joined:
    Jan 30, 2006
    Location:
    Scotland
    #10
    Thanks i think i am starting to get this. But it is gona take some head re muddling, before PHP i used Darkbasic. I am rather a fan of line by line programing due to well only doing that.

    Perhaps this OOP will help me muddle my head around ruby and ruby on rails. Would love to be able to make fancy apps with sproutcore.
     
  11. Cromulent macrumors 603

    Cromulent

    Joined:
    Oct 2, 2006
    Location:
    The Land of Hope and Glory
    #11
    You might find Sequel Pro useful. It is an open source MySQL GUI based database tool. It basically lets you manage the whole database and run queries on it. They seem to be developing it pretty quickly, it is just missing user management and a couple of other esoteric things before I can dump Navicat for it.

    Edit : If you do download it, make sure you get version 0.9.5 RC1.
     
  12. SrWebDeveloper macrumors 68000

    SrWebDeveloper

    Joined:
    Dec 7, 2007
    Location:
    Alexandria, VA, USA
    #12
    Why?

    Respectfully, that's a release candidate (RC), which means it's not technically their most stable release and in wide beta. I'm sure it's a decent version and I know you'll vouch for it, but when recommending software - it is safer to always suggest the last stable release. As of this writing that would be 0.9.4 (build 392). Actually, recommending an RC is fine so long as you inform the users of what they're getting. We have a lot of novices here who might not know what RC means, so when you say "make sure you get [it]" and don't justify why or disclaim, it's not a good thing sometimes.

    -jim
     
  13. Cromulent macrumors 603

    Cromulent

    Joined:
    Oct 2, 2006
    Location:
    The Land of Hope and Glory
    #13
    In most cases I would agree with you but in this instance I do not as this version fixes several important bugs from the old stable version. Therefore I recommend the release candidate over the stable version.
     
  14. Mr.nix macrumors member

    Joined:
    Jan 30, 2007
    #14
    Just for a little comparison, here is the exact same thing in Zend Framework.

    PHP:
    <?php

    class Model_Post extends Model_Basic
    {
        public function 
    create(array $post)
        {
            
    $parser = new BBCode();
            
            
    $this->db->insert('post', array(
                
    'topicid'        => $post['topicid'],
                
    'userid'         => 1,#(int)$this->user->userid,
                
    'title'          => $post['title'],
                
    'message'        => $post['message'],
                
    'message_parsed' => $parser->Parse($data['message']),
                
    'posted'         => time()
            ));
            
            return 
    $this->db->lastInsertId();
        }
     
  15. SrWebDeveloper macrumors 68000

    SrWebDeveloper

    Joined:
    Dec 7, 2007
    Location:
    Alexandria, VA, USA
    #15
    @Cromulent - thanks for explaining, that information was left out of your original response, we aint pyschic here.

    @Mr Nix:

    Cool code!

    -jim
     
  16. Cabbit thread starter macrumors 68020

    Cabbit

    Joined:
    Jan 30, 2006
    Location:
    Scotland
    #16
    One thing i am missing here is why do i need Sequel pro to develop PHP code? I do have it for database administration, and i also have phpmyadmin but none of these really benefit PHP development as i already know how to form a SQL query so i don't really need the SQL to copy and paste the issue is in the extra "," that the code has added in that i will resolve.

    And Mr.nix that does indeed look like some very clean code and i will take a look at the code in the zend framework.
     
  17. SrWebDeveloper macrumors 68000

    SrWebDeveloper

    Joined:
    Dec 7, 2007
    Location:
    Alexandria, VA, USA
    #17
    I take it you didn't even visit its web site! It's for DB management, nobody here said it helps with PHP.

    I've tried it, it's great. The main reason I suggested phpMyAdmin originally was it's web based and is accessible anywhere so it's commonly suggested in this forum for professional developers remotely managing databases for clients. Not because its the only tool worth using.

    You don't "need" it, it was suggested to you as a tool to assist in doing exactly what you described, with additional features you might find useful if you wish to check it out, such as:

    • It's a desktop app on Mac, extremely fast, extremely easy, friendly GUI
    • Much better for importing and exporting, no memory limits or PHP upload maxsize issues, also a ton faster too
    • Editing tables and creating tables is much, much faster
    • The search/filter feature is just what I'd want, much better and easier than phpMyAdmin - and a useful feature, too
    • Easy to store queries and not have to do special setup for that in phpMyAdmin for that and bookmarks, plus tons faster
    Just another tool in the toolbox to help you streamline your time and efforts, and it's free, so a great recommendation by Mr. Nix.

    -jim
     
  18. Mr.nix macrumors member

    Joined:
    Jan 30, 2007
    #18
    I didn't recommend it, but I do use it!
     
  19. SrWebDeveloper macrumors 68000

    SrWebDeveloper

    Joined:
    Dec 7, 2007
    Location:
    Alexandria, VA, USA
    #19
    Oops! Credit goes to Cromulent, sorry! ;) But thanks for the additional endorsement on the product!
     
  20. Cabbit thread starter macrumors 68020

    Cabbit

    Joined:
    Jan 30, 2006
    Location:
    Scotland
    #20
    Ah i see it just looked like a very out of the blue recommendation to me.
     
  21. sbauer macrumors member

    Joined:
    Feb 7, 2009
    Location:
    Baltimore, MD
    #21
    Eh. Not a fan.
     
  22. Mr.nix macrumors member

    Joined:
    Jan 30, 2007
    #22
    Not a fan of what? A simple insert?
     
  23. sbauer macrumors member

    Joined:
    Feb 7, 2009
    Location:
    Baltimore, MD
    #23
    Yes, that's it. I'm not a fan of an insert...

    I'm not a fan of the code.
     
  24. SrWebDeveloper macrumors 68000

    SrWebDeveloper

    Joined:
    Dec 7, 2007
    Location:
    Alexandria, VA, USA
  25. sbauer macrumors member

    Joined:
    Feb 7, 2009
    Location:
    Baltimore, MD
    #25
    Honestly, I haven't used PHP in eight years. As a result, I'm sure I'm a little rusty. I have no issues writing the class in a different language, though.

    I'll comment on why I don't like certain aspects. Some are small (personal preference), while others are larger.

    - I don't like class names that contain an underscore.
    - Is this really a model class? It feels like a class designed after a data access pattern, such as gateway or mapper.

    The class Model_Post feels empty. There are no properties, and no methods (outside of the create). Not only that, but you have to pass in an array/hash of data and magic strings to insert something into the database. You're not even using anything related to a model. You're essentially creating a class to turn an array of data into a database record. If you had passed in an instance of post into the create method, you could have used a property, or method, to set the id. Since objects are passed by reference in PHP5 (like Java, C#, etc..), you wouldn't have had to return a value.

    - I also don't like having the word model in the class name. Post should be good enough. To me it's just being redundant. You might as well put class in there too.

    - BBCode class needs a new name. Based off of it's parse method, I'm assuming it's a parser. It could have been a writer, parser, or a custom bbcode element implementation. In a larger system, that may not be as clear.
     

Share This Page