patternphpMinor
mysqli wrapper class
Viewed 0 times
mysqliwrapperclass
Problem
I made the following class to wrap
```
conn = mysqli_connect($host, $user, $pass, $db);
if(mysqli_connect_errno()) $this->error = mysqli_connect_error();
}
public function __destruct() {
if($this->stmt) mysqli_stmt_close($this->stmt);
mysqli_close($this->conn);
}
/** Executes a prepared SQL query on the database
*
* The second argument (optional) is the types list, followed by the parameters
e.g. query('SELECT FROM table WHERE id = ? OR name = ?', 'is', 5, 'Joe')
*
* @param string $query is the sql statement to execute
* @return mixed false on error,
* true on successful select, or
* the number of affected rows
*/
public function query($query) {
// create a new prepared statement
$stmt = mysqli_prepare($this->conn, $query);
if(!$stmt) {
$this->error = mysqli_error($this->conn);
return false;
}
// apply the arguments if any exist
if(func_num_args() > 2) {
$args = array_slice(func_get_args(), 1);
$refs = array();
foreach($args as $key=>&$value) $refs[$key] = &$value;
call_user_func_array(array($stmt, 'bind_param'), $refs);
}
// run the query
$stmt->execute();
if($stmt->errno) {
$this->error = mysqli_error($this->conn);
return false;
}
// if the query was not a select, return the number of rows affected
if($stmt->affected_rows > -1) {
$rows = $stmt->affected_rows;
mysqli_stmt_close($stmt);
return $rows;
}
// close any previous statement
if($this->stmt) mysqli_stmt_close($this->stmt);
$this->stmt = $st
mysqli for PHP using prepared statements. It seems to work well, but I was hoping to get opinions (on overall structure, performance, usage, etc.). Thanks for the insight.```
conn = mysqli_connect($host, $user, $pass, $db);
if(mysqli_connect_errno()) $this->error = mysqli_connect_error();
}
public function __destruct() {
if($this->stmt) mysqli_stmt_close($this->stmt);
mysqli_close($this->conn);
}
/** Executes a prepared SQL query on the database
*
* The second argument (optional) is the types list, followed by the parameters
e.g. query('SELECT FROM table WHERE id = ? OR name = ?', 'is', 5, 'Joe')
*
* @param string $query is the sql statement to execute
* @return mixed false on error,
* true on successful select, or
* the number of affected rows
*/
public function query($query) {
// create a new prepared statement
$stmt = mysqli_prepare($this->conn, $query);
if(!$stmt) {
$this->error = mysqli_error($this->conn);
return false;
}
// apply the arguments if any exist
if(func_num_args() > 2) {
$args = array_slice(func_get_args(), 1);
$refs = array();
foreach($args as $key=>&$value) $refs[$key] = &$value;
call_user_func_array(array($stmt, 'bind_param'), $refs);
}
// run the query
$stmt->execute();
if($stmt->errno) {
$this->error = mysqli_error($this->conn);
return false;
}
// if the query was not a select, return the number of rows affected
if($stmt->affected_rows > -1) {
$rows = $stmt->affected_rows;
mysqli_stmt_close($stmt);
return $rows;
}
// close any previous statement
if($this->stmt) mysqli_stmt_close($this->stmt);
$this->stmt = $st
Solution
Instead of assigning your properties null or empty values from the start, just define them with no values. If they aren't initialized, then they will resolve to null. Although, from what I can tell, you aren't doing much checking of these properties anyways, so I don't understand why you had default values anyways. You really should be verifying these properties before you use them.
You should always specify the access level of your methods and properties (public, private, protected). You did fairly well with your properties, but then you ignore your methods. Sure, right now PHP defaults to making each of these methods public by default, but this will be deprecated before too much longer. Plan for the future and define them properly. Besides, this is just good coding practice.
Always, always, always use braces on your statement. In languages such as Python, where braces don't exist, this is fine, but in PHP this is wrong. Even though PHP allows this syntax, even they say it is wrong, because it CAN cause issues with the code. Not to mention problems with reading the code. Its two characters, and doesn't hurt anything to add them, but it could cause all kinds of problems by not adding them. You do this quite a bit, so I wont point out each case.
When you have a lot to say in the comments, don't use single line comments, use block comments. Or better yet, since you seem to be documenting the behavior of a method here, you should be using PHPDoc comments.
You lied to us in the documentation and promised us a third and, at the very least, fourth parameter in your
SR tells me that this method should only be focused with querying the database and returning the results, after all, that's what you called it
You have potentially violated the DRY principle because each of the tasks I listed above, and each of the others I didn't, could potentially be reused in this class. Instead they are hardcoded into the one method and therefore their potential is being wasted.
Here's another DRY violator. If you have two methods performing the same task, either A) get rid of one of them, or B) have one of them point to the other. This way if you ever decided to change which array you were fetching from, you would only have to do so once. This is one of the key benefits of DRY.
Ternary functions, unless nested (which they should never be), don't require parenthesis. So, this could be a stylistic point, but figured I'd let you know.
private
$conn,
$stmt,
$arr,
$eof=true,
$error
;You should always specify the access level of your methods and properties (public, private, protected). You did fairly well with your properties, but then you ignore your methods. Sure, right now PHP defaults to making each of these methods public by default, but this will be deprecated before too much longer. Plan for the future and define them properly. Besides, this is just good coding practice.
Always, always, always use braces on your statement. In languages such as Python, where braces don't exist, this is fine, but in PHP this is wrong. Even though PHP allows this syntax, even they say it is wrong, because it CAN cause issues with the code. Not to mention problems with reading the code. Its two characters, and doesn't hurt anything to add them, but it could cause all kinds of problems by not adding them. You do this quite a bit, so I wont point out each case.
if(mysqli_connect_errno()) { $this->error = mysqli_connect_error(); }When you have a lot to say in the comments, don't use single line comments, use block comments. Or better yet, since you seem to be documenting the behavior of a method here, you should be using PHPDoc comments.
/* returns false on error, true on successful select, number of affected rows otherwise
at minimum, query is the sql statement to execute
if there are query parameters, the second argument is is the types list, followed by the parameters
e.g. query('SELECT * FROM table WHERE id = ?', 'i', 5)*/
//OR PHPDoc Comments
/** Queries database
*
* The second argument is is the types list, followed by the parameters
*
* e.g. query('SELECT * FROM table WHERE id = ?', 'i', 5)
*
* @param string $query At minimum, query is the sql statement to execute if there are query parameters
*
* @return mixed false on error, true on successful select, number of affected rows otherwise
*/You lied to us in the documentation and promised us a third and, at the very least, fourth parameter in your
query() method. But they are no where in sight. I do see that you have decided to use func_get_args() in there, but that's a big no-no (at least for something so simple). If you want to pass an unspecified number of parameters, do so as an array. That's all you are really getting with func_get_args() anyways. Either way, your documentation should properly reflect your code, otherwise it is useless. Besides the parameter issue, there is something fundamentally wrong with your query() method. It is entirely too busy. Two very common principles that are always associated with OOP are the "Single Responsibility" (SR) Principle and the "Don't Repeat Yourself" (DRY) Principle, the first isn't normally shortened to that acronym, but I will do so in this answer to avoid repeating myself. These two principles go hand in hand, and you have potentially violated both of them.SR tells me that this method should only be focused with querying the database and returning the results, after all, that's what you called it
query(). However, you are doing so much more. Such as binding the parameters and executing the statement. I wont explain each deviation to you, because you need to be able to separate your code properly, but those two should help get you started. I'm also not entirely certain, but is it necessary for that array to be assigned values by reference? I didn't feel like following it all the way through to make sure.You have potentially violated the DRY principle because each of the tasks I listed above, and each of the others I didn't, could potentially be reused in this class. Instead they are hardcoded into the one method and therefore their potential is being wasted.
Here's another DRY violator. If you have two methods performing the same task, either A) get rid of one of them, or B) have one of them point to the other. This way if you ever decided to change which array you were fetching from, you would only have to do so once. This is one of the key benefits of DRY.
function result($field) { return $this->arr[$field]; }
function __get($field) { return $this->result($field); }Ternary functions, unless nested (which they should never be), don't require parenthesis. So, this could be a stylistic point, but figured I'd let you know.
function count() { return $this->stmt ? $this->stmt->num_rows : 0; }Code Snippets
private
$conn,
$stmt,
$arr,
$eof=true,
$error
;if(mysqli_connect_errno()) { $this->error = mysqli_connect_error(); }/* returns false on error, true on successful select, number of affected rows otherwise
at minimum, query is the sql statement to execute
if there are query parameters, the second argument is is the types list, followed by the parameters
e.g. query('SELECT * FROM table WHERE id = ?', 'i', 5)*/
//OR PHPDoc Comments
/** Queries database
*
* The second argument is is the types list, followed by the parameters
*
* e.g. query('SELECT * FROM table WHERE id = ?', 'i', 5)
*
* @param string $query At minimum, query is the sql statement to execute if there are query parameters
*
* @return mixed false on error, true on successful select, number of affected rows otherwise
*/function result($field) { return $this->arr[$field]; }
function __get($field) { return $this->result($field); }function count() { return $this->stmt ? $this->stmt->num_rows : 0; }Context
StackExchange Code Review Q#14701, answer score: 2
Revisions (0)
No revisions yet.