The first thing I've noticed is that you return 2 different things:
- The data that the user wanted;
- The error information.
Now, how will you distinguish between an error-ed query and a successful one? You can't! And that makes angels angry and cry.
Return null
in case of error.
Your variable names are really....... conflicting and incomplete...
$x = 1; //Should be $i
$vec = []; //No idea what this is for
foreach($params as $p) //$p should be $param
$array = ''; // ... a variable called $array that receives a string ... WTH!?
A good variable name will give us an idea of it's content without having to read through all the code.
The name for $i
(instead of $x
) is the standard name used for an increment. Since you have $x++
below, it is a good name.
Now, you have this block:
if($stmt->execute()){
while($row = $stmt->fetch(PDO::FETCH_OBJ)){
$array = '';
foreach ($cols as $col) {
$array .= $row->$col.' ';
}
$vec[] = $array;
}
return $vec;
} else {
return $db->errorInfo();
}
Use early returns, like this:
if(!$stmt->execute()){
return null;
}
while($row = $stmt->fetch(PDO::FETCH_OBJ)){
$array = '';
foreach ($cols as $col) {
$array .= $row->$col.' ';
}
$vec[] = $array;
}
return $vec;
This helps the code to be more readable and reduces the nesting level. Too many chained if
s and loops will cause your code to be hard to read.
You have the following if
, just a little bellow:
if($stmt = $db->prepare($sql)){
$x = 1;
$vec = [];
if(count($params)){
foreach($params as $p){
$stmt->bindValue($x, $p);
$x++;
}
}
}
Applying the early return:
$stmt = $db->prepare($sql)
if(!$stmt){
return null;
}
$i = 1;
$vec = [];
foreach($params as $param){
$stmt->bindValue($i, $param);
$i++;
}
See? It is so much easier to read!
In fact, the loop can be like that: (this point is subjective!)
foreach( array_keys($params) as $i => $param ) {
$stmt->bindValue($i + 1, $params[$param]);
}
Bye bye $i++;
!
Going deeper on your code, I see that you also have some useless code.
Check this block:
if(count($params)){
foreach($params as $p){
$stmt->bindValue($x, $p);
$x++;
}
}
Why do you have that count()
there? It does nothing there. Just do this:
foreach($params as $param){
$stmt->bindValue($i, $param);
$i++;
}
The foreach
doesn't care about the length of $params
. As long as it is an array.
And now, going to this block:
while($row = $stmt->fetch(PDO::FETCH_OBJ)){
$array = '';
foreach ($cols as $col) {
$array .= $row->$col.' ';
}
$vec[] = $array;
}
return $vec;
Question time:
- What, in the name of the Lord, is going on here?
- Why are you fetching an object instead of an associative array?
- What does
$array
do?
- Why is it called
$array
?
- What is
$vec
for? What's in it?
After testing myself, I concluded this: It's bugged!!!
The expected result is different that the provided result.
I really don't get the reasoning to do $row->$col.' ';
.
Here's a test code I've build, based on yours:
//test variable, fakes the database
$items = array(
(object)array(
'b'=>5,
'c'=>'a'
),
(object)array(
'b'=>7
)
);
$cols = array('b','c');//only 1 column
//simulates $stmt->fetch(PDO::FETCH_OBJ)
while($row = array_shift($items)){
$array = '';
foreach ($cols as $col) {
$array .= $row->$col.' ';
}
$vec[] = $array;
}
var_export($vec);
Result I expected (added extra whitespace):
array (
0 =>
array (
'b' => 5,
'c' => 'a',
),
1 =>
array (
'b' => 7,
),
)
Result obtained:
<br />
<b>Notice</b>: Undefined property: stdClass::$c in <b>[...][...]</b> on line <b>7</b><br />
array (
0 => '5 a ',
1 => '7 ',
)
Check it on http://sandbox.onlinephpfunctions.com/code/1e0527eccba4ada6de7b4cdba9967ab9df4e2ce7
In conclusion: The code is broken! Fixing it is left as an exercise for you.