MySQL update with PHP; MySQLi prepared statement in loop

I'm going to develop an Android application that performs simple MySQL operations by invoking server-sided PHP scripts. PHP-MySQL communication is done by MySQLi extension. Each operation that I'd like to perform is in a separate PHP file, and in a separate class. There's an example of update.php:

 'SIBISIBI', 'model_pojazdu' => 'Vectra', 'marka' => 'Opel');
    $_POST['UPDATE_HEADERS'] = array('table' => 'polisy_oc', 'id' => 'numer_polisy_oc', 'id_value' => '72451');
}

class UpdateRecord
{
    protected $db_connection;

    public function __construct($db_connection) {
        $this->db_connection = $db_connection->connection;

        $this->update($_POST['UPDATE_HEADERS'], $_POST['UPDATE_DATA']);
    }

    private function update($update_headers, $update_data) { //TODO:single query
        if (in_array($update_headers['table'], $_SESSION['tables_white_list']) && in_array($update_headers['id'], $_SESSION['columns_white_list'])) {
            foreach ($update_data as $key => $value) {
                if (checkColumn($key)) {
                    $update = $this->db_connection->prepare("UPDATE {$update_headers['table']} SET $key = ? WHERE {$update_headers['id']} = ?");
                    $update->bind_param('ss', $value, $update_headers['id_value']);
                    $update->execute();
                    $update->close();
                }
            }
        }
    }
}
$db = new Database;
$update = new UpdateRecord($db);
?>

My main concern is whether Depency Injection is properly used regarding database connection (all my other scripts do the same thing).
Here's how Database class is built:

```
class Database
{
public $connection;

function __construct()
{
if(!isset($_SESSION)){
session_start();
}
$this->connection = new mysqli('localhost', $_SESSION['login'], $_SESSION['password'], 'Ubezpieczenia_OC');

if($this->connection->connect_error) {
doDatabaseConnectionError();

I'll stick to reviewing your PHP code, as it doesn't look like it's recieved the attention it deserves!

I'll do what I can to find faults, but I won't be testing anything. Alright, we ready?

• Nice! First line and we've found something! What kind file is functions.php!? Filenames should be one of the clearest names in your project. If you can't come up with a good filename, then that's a sign you need to break apart the contents within.

• I'm assuming the initial conditional empty() is for testing only? I hope so, because automatically updating data on page load is a disaster waiting to happen. Send a couple hundreds requests to a page that's doing resource heavy database access and you'll happily be sitting in a DoS attack.

-
It's a little hard to tell, but it looks like those variable sets in that conditional are longer than any desired line length. It makes it much harder for the reader to effienctly read your code if they have to do a lot of horizontal scrolling.

You can reduce this a couple ways. In this case, break down your array over multiple lines:

$_POST['UPDATE_HEADERS'] = array(
        'table' => 'polisy_oc', 
        'id' => 'numer_polisy_oc', 
        'id_value' => '72451'
    );

This way is substantially easier to read in my opinion.

For other really long lines, you may be able to put data in variables, and then reference those variables.

• @200_success had a good point: UpdateRecord isn't a very good name. Although it's hard to rename this class because it really doesn't have a single responsibility. That in itself contradicts the principles of SOLID. You might look towards making a Record class.

• __construct($db_connection) should really be __construct(Database $db_connection) to better implement dependency injection practices.

• I'd avoid automatically calling update() from the constructor. Make update() public and call it where the object is initialized.

-
If update() were in a Record class, that'd be OK. But here, I don't think the name is right. For all I know, this could really be updating the record updater!

If you don't change the class name, I suggest this be a method called updateRecord().

• Where do we get $_SESSION from? The user. And what's our golden rule? Never trust the user. I think it's a bad idea to check the POST data against session data. Make a concrete white-list somewhere. Either that, or eliminate the need to dynamically reference a table. I'm sure with enough whiteboard space you'll be able to figure it out!

• Avoid using the default $key and $value. Give some more descriptive variable names.

• What is checkColumn()?

Alright! One file down, now the next?

• Database is a very vague name. You got the noun part down, but seeing something like this would kill me!

• Something like $connection doesn't need to be public. I'd make that private/protected and encapsulate it.

• Add a visibility to your constructor.

• Your constructor has too much business logic in it! It should be for constructing the object, nothing more. Break the logic into other methods.

• Oh goodness! I do not like how you're setting up the database via the session! That is asking for a lot of trouble! Why can't the credentials be hardcoded? Or maybe put in a config file and access that way? The user has your database credentials!

• Your DBAL shouldn't have any type of business logic in it what-so-ever. It defeats the point of having it in a separate class.

And lastly, you mention your code bottlenecking, but you're not sure why. Profile your code, get some concrete numbers, and then work from there. Is it that foreach that's causing the lag, or it say the query? You'll never know unless you test it!

In the comments a couple questions were asked. I'll do my best to answer them:


functions.php is a file which contains database class and functions that are used by more than one class; shouldn't I use such resolution?

It's vague. What kind of functions are in this file? What kind of functions should go in this file? When is it appropriate to have a new class, versus just add functions into here. It's a problem of extending functional programming along side OO programming.


Making a Record class would mean that it would have methods like updateClient(), insertClient() etc. inside, which I thought would make connecting app to database harder - wouldn't it be simpler for an app just to invoke desired class which is in separate .php file?

Those methods you list might actually be a better fit in a Client class. But having CRUD operations in a single class is not a bad thing. It keeps all "record" things together, and all "client" things together.


Why adding class name in __construct parameters implements better dependency injection?

It ensures that the type hinted class is being introduced into the object. If I say __construct(Database $db), I know that a database will be passed. It also helps inheritance efforts. I coul

StackExchange Code Review Q#58553, answer score: 4