After a quick glance at your code, it reminded me of this review I posted a while back this review I posted a while back. Much like that piece of code, a couple of (broadly similar) issues jumped out:
After a quick glance at your code, it reminded me of this review I posted a while back. Much like that piece of code, a couple of (broadly similar) issues jumped out:
After a quick glance at your code, it reminded me of this review I posted a while back. Much like that piece of code, a couple of (broadly similar) issues jumped out:
If you want to avoid lengthy code at all costs, then perhaps you should consider learning another language than PHP. Perl, for example, allows you to write obfuscated code a lot easier. Mind you, PHP has a couple of tricks up its sleeve, as this "Hello world" demonstrates (source):
If you want to avoid lengthy code at all costs, then perhaps you should consider learning another language than PHP. Perl, for example, allows you to write obfuscated code a lot easier. Mind you, PHP has a couple of tricks up its sleeve, as this "Hello world" demonstrates (source):
Update 1
As you requested, some more details on the second issue I mentioned (about the dbConnect
method being flawed). Your code, as it stands now expects the user to write something like:
$obj = new dbConfig();
$obj->host = '127.0.0.1';
$obj->username = 'user';
$obj->password = 'pass';
$obj->dab = 'dbname';
$obj->dbConnect();
But what if, because most devs are lazy, someone writes:
$db = new DbConfig();//classes should start with an UpperCase
//some code
someFunction($db);
//where someFunction looks like:
function someFunction(DbConfig $db)
{
$db->dbConnect();//<== ERROR! host, user, pass... nothing is set
}
The instance could be created in a different file as the line that is calling dbConnect
, so debugging this kind of code is hellish. You could re-write each piece of code that calls dbConnect
to make sure all of the required properties have been set correctly, but what if they aren't? And what is "correct"? That's something the instantiating code should know, not the code that calls dbConnect
. Luckily, you can use a constructor to ensure all parameters are known:
class DbConnect
{
/**
* @var string
*/
protected $host = null;
/**
* @var string
*/
protected $username = null;
/**
* @var string
*/
protected $pass = null;//null for no pass
/**
* @var string
*/
protected $dbName = null;
/**
* @var mysqli
*/
protected $conn = null;
public function __construct($host, $user, $pass = null, $db = null)
{
$this->host = $host;
$this->user = $user;
$this->pass = $pass
$this->dbName = $db;
}
}
Now, whenever an instance is created, the host and user must be passed to the constructor, if not, PHP will throw up a fatal error. So you'll have to use the class like so:
$db = new DbConnect('127.0.0.1', 'root', '', 'chatapp');
This reduces the users' code (where your class is being used), and makes the instances behave more predictably.
You may have noticed that I've changed the visibility from public
to protected
. That's because public
properties can be reassigned on the go, without any validation on their new value whatsoever. That's dangerous. Instead, I suggest you add some getters and setters for the properties that you want to expose (like the database):
public function setDbName($name)
{
if (!is_string($name))
{//dbName MUST be a string, of course, so if it isn't, notify the user
throw new InvalidArgumentException(
sprintf(
'%s expects name argument to be a string, instead saw "%s"',
__METHOD__,
gettype($name)
)
);
}
$this->dbName = $name;
return $this;
}
Using this method, you can also check if the instance is currently holding an active db connection, clean up whatever needs to be cleaned up (closing cursors, freeing results, ...) and (re)connect or select the new db on that connection.
On the user side, using these setters is as simple as:
$instance->setDbName('foobar');
//but:
$instance->setDbName(array());
//InvalidArgumentException
//message: DbConnect::setDbName expects name argument to be a string, instead saw "array"
Given that your class clearly tries to abstract the nitty-gritty of the rather messy mysqli
API away from the user (which I can understand), it's also important that the user needn't worry about closing the connection in time. When your instance of DbConnect
goes, then so must the DB connection itself go. For that, a simple desctructor would do the job just fine:
public function __destruct()
{
if ($this->conn instanceof mysqli)
this->conn->close();//or mysqli_close($this->conn);
$this->conn = null;//optional
}
Now you can rest assured that, in case someone writes this:
$db = new DbConnect('127.0.0.1', 'root');
$db->dbConnect();
//...code here
$db = null;//or unset($db), or $db just goes out of scope
The connection will be closed automatically. Naturally, for all of these changes to take effect, you'll have to refactor your dbConnect
method somewhat, but that's something I'll try to cover in the next update.
Update 1
As you requested, some more details on the second issue I mentioned (about the dbConnect
method being flawed). Your code, as it stands now expects the user to write something like:
$obj = new dbConfig();
$obj->host = '127.0.0.1';
$obj->username = 'user';
$obj->password = 'pass';
$obj->dab = 'dbname';
$obj->dbConnect();
But what if, because most devs are lazy, someone writes:
$db = new DbConfig();//classes should start with an UpperCase
//some code
someFunction($db);
//where someFunction looks like:
function someFunction(DbConfig $db)
{
$db->dbConnect();//<== ERROR! host, user, pass... nothing is set
}
The instance could be created in a different file as the line that is calling dbConnect
, so debugging this kind of code is hellish. You could re-write each piece of code that calls dbConnect
to make sure all of the required properties have been set correctly, but what if they aren't? And what is "correct"? That's something the instantiating code should know, not the code that calls dbConnect
. Luckily, you can use a constructor to ensure all parameters are known:
class DbConnect
{
/**
* @var string
*/
protected $host = null;
/**
* @var string
*/
protected $username = null;
/**
* @var string
*/
protected $pass = null;//null for no pass
/**
* @var string
*/
protected $dbName = null;
/**
* @var mysqli
*/
protected $conn = null;
public function __construct($host, $user, $pass = null, $db = null)
{
$this->host = $host;
$this->user = $user;
$this->pass = $pass
$this->dbName = $db;
}
}
Now, whenever an instance is created, the host and user must be passed to the constructor, if not, PHP will throw up a fatal error. So you'll have to use the class like so:
$db = new DbConnect('127.0.0.1', 'root', '', 'chatapp');
This reduces the users' code (where your class is being used), and makes the instances behave more predictably.
You may have noticed that I've changed the visibility from public
to protected
. That's because public
properties can be reassigned on the go, without any validation on their new value whatsoever. That's dangerous. Instead, I suggest you add some getters and setters for the properties that you want to expose (like the database):
public function setDbName($name)
{
if (!is_string($name))
{//dbName MUST be a string, of course, so if it isn't, notify the user
throw new InvalidArgumentException(
sprintf(
'%s expects name argument to be a string, instead saw "%s"',
__METHOD__,
gettype($name)
)
);
}
$this->dbName = $name;
return $this;
}
Using this method, you can also check if the instance is currently holding an active db connection, clean up whatever needs to be cleaned up (closing cursors, freeing results, ...) and (re)connect or select the new db on that connection.
On the user side, using these setters is as simple as:
$instance->setDbName('foobar');
//but:
$instance->setDbName(array());
//InvalidArgumentException
//message: DbConnect::setDbName expects name argument to be a string, instead saw "array"
Given that your class clearly tries to abstract the nitty-gritty of the rather messy mysqli
API away from the user (which I can understand), it's also important that the user needn't worry about closing the connection in time. When your instance of DbConnect
goes, then so must the DB connection itself go. For that, a simple desctructor would do the job just fine:
public function __destruct()
{
if ($this->conn instanceof mysqli)
this->conn->close();//or mysqli_close($this->conn);
$this->conn = null;//optional
}
Now you can rest assured that, in case someone writes this:
$db = new DbConnect('127.0.0.1', 'root');
$db->dbConnect();
//...code here
$db = null;//or unset($db), or $db just goes out of scope
The connection will be closed automatically. Naturally, for all of these changes to take effect, you'll have to refactor your dbConnect
method somewhat, but that's something I'll try to cover in the next update.