Skip to main content
Code Review

Return to Answer

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

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:

replaced http://codegolf.stackexchange.com/ with https://codegolf.stackexchange.com/
Source Link

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 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):

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):

added 4631 characters in body
Source Link

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.

added 1455 characters in body
Source Link
Loading
Source Link
Loading
lang-php

AltStyle によって変換されたページ (->オリジナル) /