Feedback on OOP Classes

Tanax · 09-17-2007, 08:58 AM

Hello, I'm new to this forum, and I'm not exactly sure if this is the correct place to post this, but I hope it is.

Anyways, I've done 2 classes, a database class that handles the database querys and general stuff with the database, and a member class that manages the members in the database.

And yea, I know that my comment lines are big.

_database.php

PHP Code:


			
<?php

/**
||||||||||||||||||||||||||||||||||||||||||
|||| @author Tanax
|||| @copyright 2007
||||||||||||||||||||||||||||||||||||||||||
**/

    class database {
          
          private $db = array();
          
          /**
        ||||||||||||||||||||||||||||||||||||||||||
        |||| Our constructor that auto-loads
        ||||||||||||||||||||||||||||||||||||||||||
        **/
          public function __construct($host, $user, $pass) {
                
            $this->db['host'] = $host;
            $this->db['user'] = $user;
            $this->db['pass'] = $pass;
                
            $this->connectdb();
                        
        }
        
        /**
        ||||||||||||||||||||||||||||||||||||||||||
        |||| Connect to db
        ||||||||||||||||||||||||||||||||||||||||||
        **/
        private function connect() {
            
            @mysql_connect($this->db['host'], $this->db['user'], $this->db['pass']) or die('Could not connect to the database!');
                    
        }
        
        /**
        ||||||||||||||||||||||||||||||||||||||||||
        |||| Select db
        ||||||||||||||||||||||||||||||||||||||||||
        **/
        public function select($name) {
            
            $this->db['db'] = $name;
            @mysql_select_db($this->db['db']) or die('This system has not been installed, or something is wrong in the database table!');
            
        }
        
        /**
        ||||||||||||||||||||||||||||||||||||||||||
        |||| Disconnect db
        ||||||||||||||||||||||||||||||||||||||||||
        **/
        public function disconnect() {
            
            mysql_close();
            
        }
        
        /**
        ||||||||||||||||||||||||||||||||||||||||||
        |||| Makes a query
        ||||||||||||||||||||||||||||||||||||||||||
        **/
        public function query($string) {
            
            $result = mysql_query($string);
            
            return $result;
            
        }

       /**
        ||||||||||||||||||||||||||||||||||||||||||
        |||| Fetches a result
        ||||||||||||||||||||||||||||||||||||||||||
        **/

        public function fetch($string) {
            
            $result = mysql_fetch_array($string);
            
            return $result;
            
        }
        
        /**
        ||||||||||||||||||||||||||||||||||||||||||
        |||| Makes a value safe from SQL injection
        ||||||||||||||||||||||||||||||||||||||||||
        **/
        public function makesafe($string) {
            
            $result = mysql_real_escape_string($string);
            
            return $result;
            
        }
        
        /**
        ||||||||||||||||||||||||||||||||||||||||||
        |||| Installs a system, adding tables
        ||||||||||||||||||||||||||||||||||||||||||
        **/
        public function install($system, $tablename) {
            
            if($system == "membersystem") {
                
                $create = "CREATE TABLE `".$tablename."` (
                
                    `u_id` int(11) NOT NULL auto_increment,
                      `username` varchar(25) NOT NULL default '',
                      `registerdate` int(25) NOT NULL default '',
                      `registerip` varchar(32) NOT NULL default '',
                    PRIMARY KEY  (`u_id`)
                
                ) TYPE=MyISAM AUTO_INCREMENT=60";
                
            }
            
            elseif($system == "memberpm") {
                
                $create = "CREATE TABLE `".$tablename."` (
                
                    `id` INT( 11 ) NOT NULL AUTO_INCREMENT PRIMARY KEY,
                    `title` VARCHAR( 255 ) NULL,
                    `message` TEXT NOT NULL,
                    `from` INT( 11 ) NOT NULL ,
                    `to` INT( 11 ) NOT NULL ,
                    `from_viewed` BOOL NOT NULL DEFAULT '0',
                    `to_viewed` BOOL NOT NULL DEFAULT '0',
                    `from_deleted` BOOL NOT NULL DEFAULT '0',
                    `to_deleted` BOOL NOT NULL DEFAULT '0',
                    `from_vdate` DATETIME NULL,
                    `to_vdate` DATETIME NULL,
                    `from_ddate` DATETIME NULL,
                    `to_ddate` DATETIME NULL,
                    `created` DATETIME NOT NULL

                ) ENGINE = MYISAM";
                
            }
            
            $insert = $this->query($create);
            if($insert) {
                    
                $msg = "Table :" .$tablename. " was successfully created!";
                return $msg;
                    
            } else {
                    
                $msg = "Table: " .$tablename. " could not be created!";
                return $msg;
                    
            }
            
        }
        
    }

?>

So that was the database class, this is the membersclass.

_members.php

PHP Code:


			
<?php

/**
||||||||||||||||||||||||||||||||||||||||||
|||| @author Tanax
|||| @copyright 2007
||||||||||||||||||||||||||||||||||||||||||
**/

    class member {
        
        private $db;
        private $data = array('u_id' => '', 'username' => '', 'password' => '', 'registerdate' => '', 'registerip' => '');
        private $table = array();
        
        /**
        ||||||||||||||||||||||||||||||||||||||||||
        |||| Our constructor that auto-loads
        ||||||||||||||||||||||||||||||||||||||||||
        **/
        public function __construct(database $db, $tablename) {
            
            $this->db = $db;
            $this->table['members'] = $tablename;
            
        }
        
        /**
        ||||||||||||||||||||||||||||||||||||||||||
        |||| Creates, or updates a member
        ||||||||||||||||||||||||||||||||||||||||||
        **/

        public function save();

        $sql = sprintf('INSERT INTO
                    %1$s
                    (username, password, registerdate, registerip)
                VALUES
                    ("%2$s", "%3$s", "%4$s", "%5$s")
                ON DUPLICATE KEY
                UPDATE
                    username = "%2$s",
                    password = "%3$s",
                    registerdate = "%4$s",
                    registerip = "%5$s"',
                $this->table['members'],
                $this->data['username'],
                $this->data['password'],
                $this->data['registerdate'],
                $this->data['registerip']);

        $this->db->query($sql);
        }
                
        /**
        ||||||||||||||||||||||||||||||||||||||||||
        |||| Checks if user is loaded
        ||||||||||||||||||||||||||||||||||||||||||
        **/
        public function isLoaded() {
            
            return isset($this->data['id']);
    
        }
        
        /**
        ||||||||||||||||||||||||||||||||||||||||||
        |||| Loads a user by ID
        ||||||||||||||||||||||||||||||||||||||||||
        **/
        public function load($u_id) {
            
            $sql = printf("SELECT `u_id`, `username`, `password`, `registerdate`, `registerip` FROM `".$this->table['members']."` WHERE `u_id` = %d", $u_id);
            $this->data = $this->db->fetch($sql);    
            
        }
        
        /**
        ||||||||||||||||||||||||||||||||||||||||||
        |||| Searches for a user by name
        ||||||||||||||||||||||||||||||||||||||||||
        **/
        public function find($name) {
        
            $name = $this->db->makesafe($name);
            $sql = printf("SELECT `u_id` FROM `".$this->table['members']."` WHERE `username` = %s", $name);
            $id = $this->db->fetch($sql);

            // If anything was found
            if(isset($id['id'])) {
                
                $this->load($id['id']);
        
            }
        
        }
        
        /**
        ||||||||||||||||||||||||||||||||||||||||||
        |||| Gets ID of loaded user
        ||||||||||||||||||||||||||||||||||||||||||
        **/
        public function getId() {
            
            if(!isset($this->data['u_id'])) {
                
                trigger_error('Tries to get property of not loaded user.', E_USER_NOTICE);
                return false;
        
            }

            return $this->data['u_id'];
    
        }
        
        /**
        ||||||||||||||||||||||||||||||||||||||||||
        |||| Gets name of loaded user
        ||||||||||||||||||||||||||||||||||||||||||
        **/
        public function getName() {
            
            if(!isset($this->data['username'])) {
                
                trigger_error('Tries to get property of not loaded user.', E_USER_NOTICE);
                return false;
        
            }

            return $this->data['username'];
    
        }
        
        /**
        ||||||||||||||||||||||||||||||||||||||||||
        |||| Sets name of user
        ||||||||||||||||||||||||||||||||||||||||||
        **/
        public function setName($name) {
            
            $name = $this->data->makesafe($name);
            $this->data['username'] = (string) $name;
    
        }
        
        /**
        ||||||||||||||||||||||||||||||||||||||||||
        |||| Gets password of loaded user
        ||||||||||||||||||||||||||||||||||||||||||
        **/
        public function getPass() {
            
            if(!isset($this->data['password'])) {
                
                trigger_error('Tries to get property of not loaded user.', E_USER_NOTICE);
                return false;
                
            }
            
            return $this->data['password'];
            
        }
        
        /**
        ||||||||||||||||||||||||||||||||||||||||||
        |||| Sets password of user
        ||||||||||||||||||||||||||||||||||||||||||
        **/
        public function setPass($pass) {
            
            $pass = $this->data->makesafe($pass);
            $this->data['password'] = $pass;
            
        }
        
        /**
        ||||||||||||||||||||||||||||||||||||||||||
        |||| Gets registerdate of loaded user
        ||||||||||||||||||||||||||||||||||||||||||
        **/
        public function getRegdate() {
            
            if(!isset($this->data['registerdate'])) {
                
                trigger_error('Tries to get property of not loaded user.', E_USER_NOTICE);
                return false;
                
            }
            
            return $this->data['registerdate'];
            
        }
        
        /**
        ||||||||||||||||||||||||||||||||||||||||||
        |||| Sets registerdate of user
        ||||||||||||||||||||||||||||||||||||||||||
        **/
        public function setRegdate($date) {
            
            $this->data['registerdate'] = $date;
            
        }
        
        /**
        ||||||||||||||||||||||||||||||||||||||||||
        |||| Gets registerip of loaded user
        ||||||||||||||||||||||||||||||||||||||||||
        **/
        public function getRegip() {
            
            if(!isset($this->data['registerip'])) {
                
                trigger_error('Tries to get property of not loaded user.', E_USER_NOTICE);
                return false;
                
            }
            
            return $this->data['registerip'];
            
        }
        
        /**
        ||||||||||||||||||||||||||||||||||||||||||
        |||| Sets registerip of user
        ||||||||||||||||||||||||||||||||||||||||||
        **/
        public function setRegip($ip) {
            
            $this->data['registerip'] = $ip;
            
        }
    
    }

?>

So I'm just wondering if you see any errors, or if you see anything that could be done better/safer.

codefreek · 09-17-2007, 09:02 AM

its nice keep up the good work.
ps: i will check if i see any bugs
and report them to you :)

Tanax · 09-17-2007, 09:04 AM

Thanks :)

And btw, I know that the members system is really simple, but I will try to make a PM system for it later, and also maybe create a template class when I have the knowledge for it :)

Wildhoney · 09-17-2007, 10:07 AM

Aside from the enormous comments! I've never really been a big fan of the following code:

PHP Code:


			
$this->data = $this->db->query("SELECT `u_id`, `username`, `password`, `registerdate`, `registerip` FROM `MEMBER_TABLE` WHERE `u_id` = '".$u_id."'")->fetch();

Where you concatenate a variable in to the SQL statement itself. Especially when it's an ID and you've placed quotes around it. I know many individuals take precaution here and place the quotes around to prevent any SQL errors for if someone should manage to inject a string into it - but there should be absolutely no way they should get the chance to.

I've always been a huge advocate of sprintf. You may enjoy reading the p if you haven't done so already.

All in all though your code looks pretty clean to me. Easy to understand as well which is always a big plus!

Tanax · 09-17-2007, 10:42 AM

PHP Code:


			
$u_id = $this->db->makesafe($u_id);
$sql = sprintf("SELECT `u_id`, `username`, `password`, `registerdate`, `registerip` FROM `MEMBER_TABLE` WHERE `u_id` = %d", $u_id);
$this->data = $this->db->query($sql)->fetch();

Something like that?

And thanks for your comment :)
About my comment blocks, I use PHPDesigner 2007, and when I use regular //commentlines, they go grey and I can barely see them. /**commentblock**/ is orange, which is alot easier to see.

But you didn't see any php errors? :)

EDIT: Actually to check the id is rather pointless, because it will always be an integer, however, the search for name would be more important to use this feature.

PHP Code:


			
public function load($u_id) {
            
            $this->data = $this->query("SELECT `u_id`, `username`, `password`, `registerdate`, `registerip` FROM `MEMBER_TABLE` WHERE `u_id` = '".$u_id."'")->fetch();    
            
        }
    
        public function find($name) {
        
            $name = $this->data->makesafe($name);
            $id = $this->query("SELECT `u_id` FROM `MEMBER_TABLE` WHERE `username` = '" . $name )->fetch();

            // If anything was found
            if(isset($id['id'])) {
                
                $this->load($id['id']);
        
            }

As you see, loading the id comes from the query from the finding. So the users have no way of writing it in manually.

So it would look something like this:

PHP Code:


			
public function find($name) {
        
            $name = $this->data->makesafe($name);
            $sql = printf("SELECT `u_id` FROM `MEMBER_TABLE` WHERE `username` = %s", $name);
            $id = $this->query($sql)->fetch();

            // If anything was found
            if(isset($id['id'])) {
                
                $this->load($id['id']);
        
            }
        
        }

Better? :P

Salathe · 09-17-2007, 03:10 PM

I've got a whole bunch of things to comment on, so bear with me because it might turn out to be quite a long post! Note that the line numbers mentioned are taken from a direct copy/paste of the code in your first post.

Database Class

Lines 23-25: Constants
It would be better to keep things relevant to the database class within the scope of said class. Define the host, user, password, etc as private member variables rather than as global constants. It avoids any potential problem with overlapping constant names, and also prevents any other class from getting access to those precious connection credentials.
Lines 42, 53, 65: Method/function names
There's no need to append "db" to the end of the function names. We're within a class called "database", it should be obvious already what the functions do! I'd suggest using just connect/disconnect (merging the select function into connect).
Line 42 & many other places: Constants not used properly
When connecting to the database and in many other places throughout both classes you try to make use of your constants, but incorrectly. If you do define('MY_CONSTANT', 'My Constant Rocks!') and attempt to echo it using: echo "MY_CONSTANT" what do you think will be echoed? The actual answer is MY_CONSTANT and not My Constant Rocks!. However if you were to write: echo MY_CONSTANT then the result would be as expected. All over your two classes you make this mistake.
Line 102: database::define function
What's the point of this, really? It is just as easy to use PHP's define method and see the comments above about using global constants at all.
Line 149: Session
Why are you mixing sessions and database activity? They should remain completely separate (as far as your classes are concerned). Also the comment is factually incorrect: it does not create a session at all.

Member Class

The whole concept here I think could do with a little improvement. I'd like to see a Member object which simple contains that member's credentials (the $data variable in your class). This class is an interface between the database and a member -- what if you wanted to retrieve the member's details from an altogether different source instead of the MySQL database? You'd have to rewrite the whole class again, essentially. Anyway, on to specific points within the class as it stands at the moment:
Line 20: Constructor
Perhaps it might be an idea in this case to add a type hint for the $db property: __construct(database $db, $tablename)
Line 23: Define
More defining of global constants, this time the name (MEMBER_TABLE) doesn't even follow the DB_ convention.
Line 38: Update query
I'm a fan of the UPDATE TABLE (...) VALUES (...) syntax and I feel it would be neater in this case.
Line 47: Syntax error
You have a typo: $this-data['password'] is missing the >. Also note that the SQL query built up on this line would also contain a syntax error since there is a missing closing bracket at the end of the VALUES list.
Line 61: parent::fetch();
The member class isn't a child of any other class so you cannot use parent
Line 70: strip
What is the purpose of this function?
SQL Queries
Many times you're online looking for one row to be returned from the database. Save your DBMS some time and effort and tell it to only return a single row using LIMIT 0,1.
Fetching results
I don't see anywhere where you use mysql_fetch_* to bring back results from the database. Surely a logical place would be in your fetch function but alas, that's not the case.

Something I need to ask is, have you actually tried using your code on your own server (localhost?) at all? The first thing that I did was copy/paste your two classes into files and tried to execute them -- syntax errors show up right away (assuming you have error tracking/reporting turned on on your development machine).

As for the grey comment lines in PHP Designer, can you not go into the settings and change the colour to something more readable (like the orange)? On the topic of comments, it's nice to see that you've at least thought about adding comments to your code but to be honest they're really not all that useful. For example, commenting the database constructor with "Our constructor that auto-loads" is just stating the blindly obvious. More useful would be to comment on what the constructor actually does "Defines global constants for the class to use and attempts to connect to the database." Most of the other comments follow in a similar vein; basically restating the associated function's name.

P.S. Sorry for such a long post!

Tanax · 09-17-2007, 03:22 PM

I have not tried it, because my server on localhost can't use classes o_O
I have no idea why, but ohwell.

I'll read the rest of the post, it's long as you said, but I'm glad someone had comments about my code! So thanks in advance, and I'll try to edit the things you mentioned :)

Tanax · 09-17-2007, 04:32 PM

Quote:

Originally Posted by Salathe

Database Class

Lines 23-25: Constants DONE
It would be better to keep things relevant to the database class within the scope of said class. Define the host, user, password, etc as private member variables rather than as global constants. It avoids any potential problem with overlapping constant names, and also prevents any other class from getting access to those precious connection credentials.
Lines 42, 53, 65: Method/function names DONE
There's no need to append "db" to the end of the function names. We're within a class called "database", it should be obvious already what the functions do! I'd suggest using just connect/disconnect (merging the select function into connect).
Line 42 & many other places: Constants not used properly DONE
When connecting to the database and in many other places throughout both classes you try to make use of your constants, but incorrectly. If you do define('MY_CONSTANT', 'My Constant Rocks!') and attempt to echo it using: echo "MY_CONSTANT" what do you think will be echoed? The actual answer is MY_CONSTANT and not My Constant Rocks!. However if you were to write: echo MY_CONSTANT then the result would be as expected. All over your two classes you make this mistake.
Line 102: database::define function DONE
What's the point of this, really? It is just as easy to use PHP's define method and see the comments above about using global constants at all.
Line 149: Session DONE
Why are you mixing sessions and database activity? They should remain completely separate (as far as your classes are concerned). Also the comment is factually incorrect: it does not create a session at all.

Member Class

The whole concept here I think could do with a little improvement. I'd like to see a Member object which simple contains that member's credentials (the $data variable in your class). This class is an interface between the database and a member -- what if you wanted to retrieve the member's details from an altogether different source instead of the MySQL database? You'd have to rewrite the whole class again, essentially. Anyway, on to specific points within the class as it stands at the moment:
Line 20: Constructor DONE Why? :P
Perhaps it might be an idea in this case to add a type hint for the $db property: __construct(database $db, $tablename)
Line 23: Define DONE
More defining of global constants, this time the name (MEMBER_TABLE) doesn't even follow the DB_ convention.
Line 38: Update query DONE I think..
I'm a fan of the UPDATE TABLE (...) VALUES (...) syntax and I feel it would be neater in this case.
Line 47: Syntax error DONE
You have a typo: $this-data['password'] is missing the >. Also note that the SQL query built up on this line would also contain a syntax error since there is a missing closing bracket at the end of the VALUES list.
Line 61: parent::fetch(); NOT DONE
The member class isn't a child of any other class so you cannot use parent
Line 70: strip NOT DONE
What is the purpose of this function?
SQL Queries NOT DONE
Many times you're online looking for one row to be returned from the database. Save your DBMS some time and effort and tell it to only return a single row using LIMIT 0,1.
Fetching results NOT DONE
I don't see anywhere where you use mysql_fetch_* to bring back results from the database. Surely a logical place would be in your fetch function but alas, that's not the case.

About the fetch, how would that function look? If I were to use it without a variable in it?

PHP Code:


			
public function fetch() {



}

with that structure.

About LIMIT 0,1... why? :S I gather the row where `u_id` = $u_id so I only get 1 row anyways..

Wildhoney · 09-17-2007, 04:56 PM

Quote:

Originally Posted by Tanax

About LIMIT 0,1... why? :S I gather the row where `u_id` = $u_id so I only get 1 row anyways..

To be honest, omitting the LIMIT 0, 1 in this case would not matter if the ID is a primary key that is set to unique. MySQL knows it's unique and it knows to find only 1. However, if you were acquiring the record via a non-unique index then LIMIT 0, 1 would prevent MySQL from trawling through your entire table looking for others that match your criteria. Therefore speeding up the query considerably.

Tanax · 09-17-2007, 04:58 PM

I see, well it's the primary key, dunno if it's unique however...
thanks for the heads up!

Salathe · 09-17-2007, 05:20 PM

MySQL knows that it's unique, but does a programmer glancing over the code?... ;)

Tanax · 09-17-2007, 05:44 PM

Haha true :P

but anyways, got any ideas for the fetch function?

Haris · 09-17-2007, 06:22 PM

PHP Code:


			
          /**

        ||||||||||||||||||||||||||||||||||||||||||

        |||| Our constructor that auto-loads

        ||||||||||||||||||||||||||||||||||||||||||

        **/

          private function __construct($host, $user, $pass) {

            

            

            if(strlen($host) > 0 && strlen($user) > 0 && strlen($pass) > 0) {

                

                $this->db['host'] = $host;

                $this->db['user'] = $user;

                $this->db['pass'] = $pass;

                

                $this->connectdb();

                

            } else {

                

                $this->connectdb();

                

            }

                        

        }

I think this could be further made shorter into the following because even though it doesn't pass the strlen function, it still tries to connect to the database.

PHP Code:


			
          /**

        ||||||||||||||||||||||||||||||||||||||||||

        |||| Our constructor that auto-loads

        ||||||||||||||||||||||||||||||||||||||||||

        **/

          private function __construct($host, $user, $pass) {

                

                $this->db['host'] = $host;

                $this->db['user'] = $user;

                $this->db['pass'] = $pass;



                $this->connectdb();

                        

        }

Tanax · 09-17-2007, 06:25 PM

Yea, I thought about that.. but I wasn't sure.. so if no arguements are given when calling the function(creating the object), it will give mysql error since the script cannot connect to db.

Salathe · 09-17-2007, 06:39 PM

If no arguments are passed to the constructor, an error will be raised because the arguments are not optional.

At the moment if you don't want to pass along any host/user/password you have to create an instance of the object like new database('', '', ''). If you want to be able to write new database() with no arguments then you need to change the function to:

public function __construct($host='', $user='', $pass='') {

Tanax · 09-17-2007, 07:08 PM

Aha! :D

Damn I've tried to come up with this fetch function for like.. 40mins now, and I still can't figure out how to do it :S

Haris · 09-17-2007, 09:10 PM

Syntax error:

PHP Code:


			
          private db = array();

should be

PHP Code:


			
          private $db = array();

Tanax · 09-17-2007, 09:19 PM

Haha, thank you! :P
Such a lil >.< xD

Haris · 09-18-2007, 08:11 PM

Quote:

Originally Posted by Tanax

Haha true :P

but anyways, got any ideas for the fetch function?

PHP Code:


			
        /*

         * Summary:     Fetches the result from the datbase in an array

         * Parameters:  SQL Statement

         * Return:      Returns an array of strings that corresponds to the fetched rows

         */

        

        public function fetch($query){

            $aResult = mysql_fetch_array($query);

            return $aResult;

        }

Tanax · 09-18-2007, 08:28 PM

so if I were to do this:

PHP Code:


			
$sql = printf("SELECT `u_id`, `username`, `password`, `registerdate`, `registerip` FROM `".$this->table['members']."` WHERE `u_id` = %d", $u_id);
$this->data = $this->db->fetch($sql);

it would mean that $data['username'] would be the value of the username row?