Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am trying to learn OOP using PHP5 and I wrote a simple MySQL connection class. Please take a look and give me some feedback on better practices, critical errors, and any other feedback you can provide.

Note: in the verifyDatabaseConnection method, I used an @ symbol to curb the error I was receiving.

<?php
class Mysql
{
        private $user;
        private $pass;
        private $data;
        private $host;

        public function __construct($user,$pass,$data,$host)
        {
                $this->user = $user;
                $this->pass = $pass;
                $this->data = $data;
                $this->host = $host;
                $this->verifyNullFields();
        }
        private function verifyNullFields()
        {
                if($this->user == NULL)
                {
                        print('mysql error : username is null');
                }
                if($this->data == NULL)
                {
                        print('mysql error : database name is null');
                }
                else if($this->host == NULL)
                {
                        print('mysql error : host name is null');
                }
                else
                {
                        $this->verifyDatabaseConnection();
                }
        }
        private function verifyDatabaseConnection()
        {

                $link = @mysql_connect($this->host,$this->user,$this->pass);
                if(!$link)
                {
                        die('mysql error : databse connection issue');
                }
                else
                {
                        $this->verifyDatabaseExist();
                }
        }
        private function verifyDatabaseExist()
        {
                $db = mysql_select_db($this->data);
                if(!$db)
                {
                        die('mysql error : database selection issue');
                }
        }

}
?>

<?php
$m = new Mysql("root","","test","localhost");
 ?>
share|improve this question

migrated from stackoverflow.com May 9 '11 at 10:27

This question came from our site for professional and enthusiast programmers.

    
i don't think this is a site for people to critique your code. this is a place to ask questions –  d'o-o'b May 8 '11 at 17:01
    
@ Jimmy C, thanks I will check that out. –  Anonymous May 8 '11 at 17:04
    
@ d'o-o'b, I guess I could of simply asked "Is this the proper way to create a MySQL connection class?" Thanks for the heads up though :-) –  Anonymous May 8 '11 at 17:05
add comment

3 Answers 3

  • Don't print anything or call die() from this class because it may breaks caller's code/page. For example, you may want to show customized error page when error occurs, but all text printed by your code will break and stops this. One point is return boolean, as already said. But I personally prefer using exceptions for that (and even more -- I create special excepton which extends standard Exception class. This approach allows to differentiate one exception from another.).

  • Get rid of chained method calls. Don't call verifyDatabaseConnection() from verifyNullFields(), verifyDatabaseExist() from verifyDatabaseConnection() and so on. Your method should do only one thing as declared in their name.

  • Rename data member to something more meaningful.

  • At your opinion: you may replace $this->user == NULL) check to is_null($this->user)

  • Also, when you will need to create similar class for PostgreSQL database (or abother DB) you should extract base class

share|improve this answer
    
I am going to create a new thread for everyone to look at. But, before I do is there a way for me to do it in this current thread? Thanks- –  Mike May 13 '11 at 1:34
add comment

Nothing seems too amiss (the fact that you're suppressing errors via $link = @mysql_connect is OK, as you're explicitly checking the $link variable afterwards.)

That said I'm really not sure why you wouldn't use MySQLi or (better still) PDO in this day and age if you're starting a new project.

Also, they fact that you're simply outputting an error message may not be the most useful approach. (You may want to return a boolean true/false as well, etc. although I'm not sure what use this would be within your constructor.)

share|improve this answer
    
Hmm, I will read up on MySQLi and PDO. I have heard of MySQLi, but never heard of PDO. I will reading up on PDO that right now. Thank you :-) –  Anonymous May 8 '11 at 17:08
    
I guess error messages would lead to attacks huh, lol. Boolean would be a much better approach. The OOP book I am reading mentioned that good practice is to always have a construct, but that could be directed more towards High Level Programming, is it good practice in PHP? –  Anonymous May 8 '11 at 17:25
    
@user738910 The constructor is fine, but the problem is that it's effectively blocking any return from your verify methods. (i.e.: It can't pass it back up to whatever called the object.) As such, you might want to call the verify method in the same scope that you created the object. –  middaparka May 8 '11 at 17:37
add comment

In the question title, you ask us to judge your "first OOP attempt", and again in the body, "I am trying to learn OOP".

Well, the code example cannot be considered OOP in my eyes. You are merely grouping related functionality into a class.

OOP is a programming paradigm. It is a mindset about how you code and structure your program. It is about how to use abstractions to solve complex business rules, how to use abstractions to allow dependencies to be replaced, e.g. for testability, or for easy migration to new platforms.

OOP is much more than just grouping code in classes.

share|improve this answer
    
Hey Pete, thanks for your input. I understand what your're saying and the more I've been reading and learning I can see A TON of people write classes that are just organized functions, but since it's wrapped in class they call it OOP. Other than WIKI, what are some resources you recommend me reading and or learning videos that could help? Thanks again :) –  Mike Jan 13 '13 at 20:50
    
Hey. The book that really made me understand what OOP is about is, 'Design Patterns'. It is an old book though with examples in C++ and SmallTalk, so maybe not the easiest to read. But it probably should be on every developers bookshelf. Otherwise, I could recommend, 'Domain Driven Design', which talks about modelling domain concepts, and perhaps after that 'Analysis Patterns' which contains examples about how you could model domain concepts in well-known domains (also an older book). –  Pete Jan 14 '13 at 8:05
    
But those book recommendations are just based on what is on my bookshelf. Each programmer has his or her own favorites. –  Pete Jan 14 '13 at 8:09
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.