building db query with a for loop

Question

I've made a function to query the database. This function takes an array, the id of the user I want to update and a query operation.

if the query operation is UPDATE

if you look at the code below, would this be a good coding practice or is this bad code?

public function query($column, $search_value, $query_operation = "SELECT"){

if(strtoupper($query_operation == "UPDATE")){
                    $query = "UPDATE users SET ";

                    if(is_array($column)){
                        $counter = 1;
                        foreach($column as $key => $value){

                            if($counter < count($column)){
                                $query .= $key . ' = ?, ';
                            }else{
                                $query .= $key . ' = ? ';
                            }

                            $counter++;
                        }

                        $query .= "WHERE id = ?";

                        $stmt = $this->database->prepare($query);
                        $counter = 1;
                        foreach($column as $key => &$value){
                             $stmt->bindParam($counter, $value);
                            $counter++;
                        }

                        $stmt->bindParam($counter, $search_value);

                        if($stmt->execute()){
                            $stmt = $this->database->prepare("SELECT* FROM         
                                                            users WHERE id = ?");
                            $stmt->bindParam(1, $search_value, PDO::PARAM_INT);
                            $stmt->execute();
                            return $this->build_array($stmt);
                        }

                    }
                }
}

would love to hear some feedback.

For starters, bind actual parameters instead of using ?, I'm not really seeing the point of a counter inside of a foreach loop (as it will already cycle for each item in the $column array), and what is this &$value ampersand supposed to do? — Casey Dwayne
– Casey Dwayne, Commented Jan 26, 2014 at 20:20
the scope of $value is only inside of the for loop, take the ampersand away and the bindParam won't have a value to bind ;) thanks for the tip. I'll change that! — Melvin Tehubijuluw
– Melvin Tehubijuluw, Commented Jan 26, 2014 at 20:22

david strachan · Accepted Answer · 2014-01-26 23:28:03Z

I would NOT mix SELECT and UPDATE in the same function.

The following update function uses arrays for column names and values $columnNames & $values using unnamed parameters.

function update($tableName,$columnNames,$values,$fieldName,$fieldValue){
    $sql = "UPDATE `$tableName` SET ";
    foreach($columnNames as $field){
        $sql .= $field ." = ?,";
    }
    $sql = substr($sql, 0, -1);//remove trailing ,
    $sql .= " WHERE `$fieldName` = ?";
    return $sql;
  }

As table and column names cannot be passed as parameters in PDO I have demonstrated whitelistng of table names.

$tables = array("client", "Table1", "Table2");// Array of allowed table names.

Also array_push()to add value for last parameter (WHERE) into $values array

Use

if (in_array($tableName, $tables))   {
    $sql = update($tableName,$columnNames,$values,$fieldName,$fieldValue);
    array_push($values,$fieldValue);
    $STH = $DBH->prepare($sql);
    $STH->execute($values);
    }

You can use similar technique for SELECT

Collectives™ on Stack Overflow

building db query with a for loop

1 Answer 1

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related