I have a database from which I'd like to return a tuple of two elements (ID and URL) with check digit to see if operation was successful.
Is correct this code to return this tuple and control?
public function getManufacturer($id) {
$stmtManufacturer = $this->dbConnection->prepare("SELECT ID, URL FROM ERP.MANUFACTURER WHERE ID = ?");
$stmtManufacturer->bind_param("i", $id);
$stmtManufacturer->execute();
$stmtManufacturer->store_result();
$stmtManufacturer->bind_result($id, $url);
$stmtManufacturer->fetch();
$stmtManufacturer->close();
return [
ACTION_RESULT => $this->processServerResponse($stmtManufacturer),
"data" =>
[
"id" => $id,
"url" => $url
]
];
}
ACTION_RESULT
is defined as a constant in the class.
I think function processServerResponse()
could be improved:
public function processServerResponse($stmt) {
if ($stmt->errno || $stmt->affected_rows === 0) {
$_actionResult = 0;
} else if ($stmt->affected_rows !== 0) {
$_actionResult = 1;
}
return $_actionResult;
}
-
\$\begingroup\$ Welcome to Code Review! I've slightly modified wording and added some more formatting to the text of the post. I hope you don't mind that. \$\endgroup\$Incomputable– Incomputable2017年03月07日 12:47:03 +00:00Commented Mar 7, 2017 at 12:47
1 Answer 1
The whole processServerResponse()
method looks rather alien to me. I never seen anything like that. A server response shouldn't be so much tightly coupled with such a specific stuff like a statement class for one of PHP extensions. A server response should be from a server, not a statement.
Besides, you are mixing two response codes here, "error occurred" and "zero results found". It should be different codes.
Usually, errors are checked by a distinct error handler, while number of results found can be checked within the same routine that returns the data.
So I would make it this way
public function getManufacturer($id) {
$stmt = $this->dbConnection->prepare("SELECT ID, URL FROM ERP.MANUFACTURER WHERE ID = ?");
$stmt->bind_param("i", $id);
$stmt->execute();
$stmt->store_result();
$stmt->bind_result($id, $url);
$found = $stmt->fetch();
return [
ACTION_RESULT => (int)$found,
"data" =>
[
"id" => $id,
"url" => $url
]
];
}
while errors should be checked with a site-wide handler like this (taken from my article on writing Db wrappers)
set_error_handler("myErrorHandler");
function myErrorHandler($errno, $errstr, $errfile, $errline)
{
error_log("[$errno] $errstr in $errfile:$errline");
header('HTTP/1.1 500 Internal Server Error', TRUE, 500);
header('Content-Type: application/json');
echo json_encode(["error" => 'Server error']);
exit;
}
So this way you will get different responses for the different scenarios.
On a side note, an obligatory suggestion: consider to use PDO instead of mysqli. This alone will do a huge refactoring for your code:
public function getManufacturer($id) {
$stmt = $this->pdo->prepare("SELECT ID id, URL url FROM ERP.MANUFACTURER WHERE ID = ?");
$stmt->execute([$id]);
$data = $stmt->fetch();
return [
ACTION_RESULT => (int)(bool)$data,
"data" => $data;
];
}
-
\$\begingroup\$ I was already thinking about refactoring the whole class to use PDO, however thank you for the suggestion. Would the error control function be implemented outside the class? In the main code? \$\endgroup\$Juan Antonio Clavero Garcia– Juan Antonio Clavero Garcia2017年03月07日 14:46:00 +00:00Commented Mar 7, 2017 at 14:46
-
2\$\begingroup\$ Yes, error control function should be implemented outside. The idea is that in case of error you should log it for the programmer and send appropriate text for the user. And then halt the execution. This scenario is common for all classes and functions and thus should be implemented separately \$\endgroup\$Your Common Sense– Your Common Sense2017年03月07日 15:04:46 +00:00Commented Mar 7, 2017 at 15:04