From the things I see in your code it looks to me like you are quite new to Object Oriented Programming (OOP), so I will try to keep that in mind during this review.
Basic OOP
The __construct()
method of a class is called only once per object of that class, when it is instantiated like so: $db = new Database()
. Your class contains a field $db_connection
, which default to false. Since the __construct()
method is called only when the object is being instantiated, there is no reason to check if ($this->db_connection != false)
, since $db_connection
will always be false for that object.
Static vs non-static
It seems to me like you are misunderstanding how regular class fields and static class fields work. Lets do a quick example based on your code:
$database1 = new Database();
$database2 = new Database();
I now have two instantiated Database
objects. Lets assume that when I created $database1
, the connection succeeded, so a PDO
object will be assigned to $db_connection
:
// Will print some stuff about the PDO object
print_r($database1->db_connection);
However, when creating $database2
, the $db_connection
field will be set to false for $database2
, because it is in no way related to the $db_connection
field of any other instantiated Database
object.
If you were to make $db_connection
a static field, then they would be shared between all class instances, because it would be a field on the actual class itself, and not on the instantiated objects.
Using fields
What else is weird is that you assign the global variables DB_HOST
, DB_NAME
, DB_PASS
and DB_USER
to some private fields of the class, and then in the constructor you do not use the private fields, but you use the global variables again. This is very unusual and should definitely be removed like so:
$this->db_connection = new PDO('mysql:host='. $this->host .';dbname=' . $this->user .';charset=utf8', $this->name, $this->pass);
Now that we have that out of the way, you should not be setting private fields of a class based on some global variable that might be there. You should change it so they have to be passed to the constructor like so:
public function __construct($host, $user, $password, $name) {
// Use the variables provided as arguments to the constructor here
}
This makes your class less dependent on your specific setup and global variables and allows for easier reuse and testing of your class. If you for whatever reason need to setup a connection to another database, you can reuse the class with different parameters. You can now use it like this:
$database = new Database(DB_HOST, DB_USER, DB_PASS, DB_NAME);
Furthermore, now that these values are passed as arguments to the constructor, you no longer need the fields to store them in, because you only use them in the constructor!
At this stage into the code review, your class would look something like this:
<?php
class database {
public $db_connection = false;
public $logs = array();
public function __construct($host, $user, $pass, $name) {
$this->logs[] = "Attempting to connect to the database.";
try{
$this->db_connection = new PDO('mysql:host='. $host .';dbname=' . $name .';charset=utf8', $user, $pass);
$this->logs[] = "A new database connection has been established.";
}
catch (PDOException $e) {
$this->logs[] = "A new database connection could not be established.";
}
}
}
One small nitpick, you should get into the habit of not ending PHP files with the ?>
closing tag. See this question for an explanation.
Logging with interfaces
One final thing I will just briefly mention: You shouldn't be logging like this. You should be logging to an interface. Consider the interface:
interface Logger {
public function log($data)
}
Now your Database class could take an optional Logger
object in the constructor like so:
public function __construct($host, $user, $pass, $name, Logger $logger = NULL) {
$this->logger = logger;
}
Now you can replace $this->logs[] = "...";
with $this->logger->log("...");
. The beauty of this is that your Database
class no longer cares how the data is being logged, only that it is being logged. You could create a ConsoleLogger
that logs to the console or a FileLogger
, that logs the data to a file. Any of these could be passed to the constructor of a new Database
object, as long as they implement the Logger
interface.
Edit: The Singleton
Since you verified that you attempted to make a singleton, I will talk about singletons for a bit. By definition, a singleton is a class that can only be instantiated once. What does this mean? This means that you should not be able to do this:
$db1 = new DatabaseSingleton();
$db2 = new DatabaseSingleton();
You have to ensure that only one instance of this class is made. You might get sneaky and attempt to do something like this:
// Don't do this!
class DatabaseSingleton() {
private static $instance_count = 0;
public function __construct() {
if (self::$instance_count > 0) {
throw new Exception("This singleton has already been instantiated!");
}
self::$instance_count += 1;
}
}
This would technically do the trick, as a second instantiation of the class would throw an exception. Notice how I have used a static variable to achieve this.
The "singleton" above is actually a very bad implementation and should be avoided. The generally accepted way of creating a singleton in PHP looks something like this:
public class DatabaseSingleton {
private static $instance;
private __construct() {
}
public function getInstance() {
if (empty(self::$instance)) {
self::$instance = new DatabaseSingleton();
}
return self::$instance;
}
}
A few clever tricks have been used here. First of all, I have made the constructor private! This means that the constructor can only be called from within the class itself, which sounds weird, but is the key to singletons. It means you can no longer do this outside of the DatabaseSingleton
class:
// An error occurs because the constructor is private
$db = new DatabaseSingleton();
I have also created a public and static getInstance()
method. This method first checks if the static field self::$instance
already contains something. If it does not, then it instantiates a new DatabaseSingleton
object and stores it in the self::$instance
field. Finally it returns the field. The second time this method is called, the if statement will be skipped because self::$intsance
is no longer empty.
The instance can now be retrieved as follows from anywhere in the code:
$db = DatabaseSingleton::getInstance();
Tada! There you have your singleton. But notice how I said anywhere. This basically means that you now have a global variable, but enclosed in a class. Singletons can sometimes be useful, but you should think real hard about whether or not you really need/want one before using this design pattern.
I hope this little extra section was informative for you!