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 playing with different ways to do database interaction in PHP, and one of the ideas I have been playing with is connecting to the DB in the constructor and disconnecting in the destructor. This is the code from my Database class.

function __construct()
{
  $this->link = mysql_connect($this->server.':'.$this->port, $this->username);
  if(!$this->link)
    die('Could not connect: '.mysql_error());

  if(!mysql_select_db($this->database, $this->link))
    die('Could not select database: '.mysql_error());
}    

function __destruct()
{
  if(mysql_close($this->link))
    $this->link = null; 
}

This works well, my only reservation is that if I need to connect several to hit the database several times it will do multiple connections and disconnects. If I do that a lot I can see, maybe, potential problems. Is this a concern or is there a better way to do this? And is my code even up to snuff in general?

share|improve this question

5 Answers 5

up vote 10 down vote accepted

From your question I infer that you're thinking of having several instances of the DB class. If so I'd suggest abstracting the connection out to another class and holding a reference to the same connection in each DB instance.

You could then set your connection up as a singleton and thus only connect & disconnect once.

Apologies in advance if I've missed anything here - my PHP is far from fluent.

share|improve this answer
    
Yes. I will have multiple instances of the database class. I just wasn't sure on a good approach to make for abstracting away the connection, or if I even should. –  percent20 Jan 19 '11 at 21:14
    
The thing is I've been stung in the past by using more than one connection when I could've shared the connection so I'd say "abstract it unless you can think of a very good reason not to" –  LRE Jan 19 '11 at 21:58

You could use MySQLi (PHP extension) which is class based by default instead of MySQL. It is very easy to set up multiple connections. You are, however, required to know the connection you are querying always.


Congrats with the first question.

share|improve this answer

You might also look into the built-in php command mysql_pconnect(). This differs from mysql_connect in that it opens a persistent connection to the DB the first time it is called, and each subsequent time, it checks to see if an existing connection to that database exists and uses that connection instead. You should then remove the mysql_close command from the destructor, as they will persist between page loads.

The php manual page: http://php.net/manual/en/function.mysql-pconnect.php

share|improve this answer
    
I read about that, and it sounds very enticing. However, how do you close the connection? in the docs it says the mysql_close() doesn't work. Do you just rely on mysql server to close it when it has been inactive for so long. I'd like to know that once all the work is done that the connection is closed. –  percent20 Jan 19 '11 at 21:19
    
You don't close the connection explicitly. It eventually gets closed by the php internals after a certain time has elapsed without use. This eliminates all of the overhead involved with repeatedly opening and closing connections, and allows individual page requests to be much faster. –  Wade Tandy Jan 19 '11 at 21:22
    
Okay, i'll do some playing with this too. Thanks. –  percent20 Jan 19 '11 at 21:26

use an abstraction library like Pear MDB2 for your database connection.

This abstracts all the connection logic away from your code, so should ever change your database (mysql to SQLite,etc) you won't have to change your code.

http://pear.php.net/manual/en/package.database.mdb2.php

share|improve this answer
3  
The last stable release was in 2007... So either it's good to the point where it doesn't need updating (doubt it, since there's a beta in the works), or it's just plain inactive (more likely) There are tons of better abstraction layers (IMHO) that are more actively developed than this... –  ircmaxell Jan 19 '11 at 21:42
1  
i haven't used PHP in a while,so please feel free to add it in this thread or create a new response :) my point is that a much better solution is to abstract the connection/query logic away via a library, more so than a specific library. –  cbrulak Jan 19 '11 at 21:46

I don't think it makes any difference in regards to connecting to the database within the construction or within a connect method, what i do think you need to change is those die commands.

using die causes the script to halt and send 1 little message to the user, who would think this is rubbish, and never visit your site again :( :(

What you should be doing is catching your errors, and redirecting to a static page where you can show a very nice message to the user, fully apologising for the technical issues your having.

You can also have an box that says, Enter your email address and we will email you when were back on line, you get the idea.

as for the code I would go down the lines of:

class Database
{
    public function __construct($autoconnect = false)
    {
        //Here you would 'globalize' your config and set it locally as a reference.
        if($autoconnect === true)
        {
             $this->connect();
        }
    }

    public function connect()
    {
        if($this->connected() === false)
        {
             $result = $this->driver->sendCommand("connect");
             if($result === true)
             {
                  $this->setConnectionState("active");
                  $this->setConnectionResource($this->driver->sendCommand("get_resource"));
             }else
             {
                 throw new DatabaseConnectionError($this->driver->sendCommand("getDriverError"));
             }
        }
    }
}

This gives you more functionality in the long run as every action is decidable within your APP, nothing is auto fired on default.

you can simple use try,catch blocks to maintain your error reporting.

share|improve this answer
    
+1 for the use of try/catch. Also I have experienced issues with failures thrown in the constructor: you do not have an instance of the object to implement whatever fallback mechanism would make sense, which in my case was using large parts of code present in the class... Without a valid instance, you cannot call instance methods, and I had to make some static methods, which was less than ideal. –  Eric Bréchemier Jan 22 '11 at 9:29
    
Thanks, But I'me struggling to understand you, Can you please rephrase your comment please. –  RobertPitt Jan 22 '11 at 12:33
    
Sure :) Throwing exceptions is fine as long as you add code to try and catch them. Avoid code that throws exceptions in a constructor though, this could lead to some issues that I have experienced: in case of failure, you have no object created, and you might miss that in your catch block, if you want to reuse behaviors defined for this object. I would rather recommend to create the connection in a regular method, e.g. init() or connect(), called on the object after it has been created, not directly in the constructor. –  Eric Bréchemier Jan 22 '11 at 17:12

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.