At first glance
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:
- Wrapping procedural style code in a class with some methods does not qualify as OOP.
- Your one and only method (
dbConnect) is fundamentally flawed and unsafe. Not in the least because it relies on the user (the code that creates an instance of your class) to set the properties manually before calling the method. What if these properties aren't set? create a __construct method (constructor) that requires the user to pass the connection params to the class when an instance is created, much in the same way that mysqli or PDO require these params to be passed. - There is some sanitation of values used in your query, but you really ought to look into prepared statements
- Methods don't
echo, and they certainly don't die. If they encounter a problem, they throw new RuntimeException, or some other exception that accurately describes the problem (InvalidArgumentException, BadMethodCallException, ...). - Your
dbConnect method just does too much. Its name suggests it merely connects, when in reality, it echoes, creates tables and never closes the DB connection. Calling dbConnecttwice is possible, your code doesn't take that into account. - Coding standards are important. PHP doesn't have any official standards (yet), but the PHP-FIG standards are adopted by all major players (Symfony, Zend, Apple, Composer, CakePHP, ...). You'd do well following them, too
md5, as a hash, is no longer secure. It first started showing weaknesses back in 1998, was deemed unsafe about ten years ago, and back in 2010, a single-block collision was found. At the very least, use sha1, but even so: PHP supports blowfish, sha256 and sha512 just as easily.- When hashing passwords, you'd also do well using salt. Don't just hash the password as-is. Add some random string that only you know. Even so, PHP has a very useful, easy to use and helpful function called
password_hash. Use it. - You seem to be confused as to the point of OOP. In PHP, and many other languages, OOP isn't used to reduce the size of your code base (look at Java, or even C++). It's about making a sizable code base more manageable, more structured. Yes, when you first start writing OO code, you'll find yourself writing a lot more code than you were used to. The benefits are that, after a while, you'll find yourself re-using bits of code that you had written for another project. OOP enable (or at least facilitates) abstraction, separation of concerns and force developers to think more about what task should go where in the application. If you want to write OO code, don't go looking for ways to reduce the amount of code you write, look for ways to improve the overall structure of your project, and optimize what component has access to what functionality at any given time.
I'll be going into all of these issues in detail, each time updating this answer, but for now, I'd urge you to check out the links I provided here, and google some OOP terms like the SOLID principles
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 dbConnectmethod 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.
