0
\$\begingroup\$

I have an import books function to insert more than 5000 rows at once, I did some research and did this one but some other places I saw a different kind of methods to insert without looping and execute this. is my code got any trouble with the performance of app

public function importBooks($data, $no_of_rows)
{
    try {
        // Start transaction
        $this->db->beginTransaction();

        // Init query
        $this->db->query('INSERT INTO books_pre (title, author, publication, container, tmp_no, created_by, created_at) VALUES (:title, :author, :publication, :container, :tmp_no, :created_by, now())');

        for ($i=2; $i<=$no_of_rows; $i++) {
            // Bind values
            $this->db->bind(':title', $data[$i]['B']);
            $this->db->bind(':author', $data[$i]['C']);
            $this->db->bind(':publication', $data[$i]['D']);
            $this->db->bind(':container', $data[$i]['E']);
            $this->db->bind(':tmp_no', $data[$i]['A']);
            $this->db->bind(':created_by', $_SESSION['user_id']);

            // Execute query
            if (!$this->db->execute()) {
                return false;
            }
        }
        // Commit data
        $this->db->commitTransaction();
    } catch (Exception $e) {
        $this->db->cancelTransaction();
        echo "Failed: " . $e->getMessage();
    }
}
\$\endgroup\$
  • \$\begingroup\$ why do you want to skip the first two rows? \$\endgroup\$ – Your Common Sense Jul 10 '18 at 8:09
  • \$\begingroup\$ i don't want to skip any of those, but I have doubted this way has any performance issues \$\endgroup\$ – Looper Jul 10 '18 at 8:12
  • \$\begingroup\$ why your $data indexing starts from 2 then? \$\endgroup\$ – Your Common Sense Jul 10 '18 at 8:14
  • \$\begingroup\$ thats one i'm inserting my data from excel sheet, so i dont want to insert the heading names on database \$\endgroup\$ – Looper Jul 10 '18 at 8:15
  • \$\begingroup\$ so you are skipping the first two rows? \$\endgroup\$ – Your Common Sense Jul 10 '18 at 8:17
1
\$\begingroup\$

Your code is all right from the performance point of view.

But again, you are doing the double work with your homegrown wrapper. For the life of me I won't understand a wrapper that makes your life harder.

So again, a code with vanilla PDO instance in $this->db

$stmt = $this->db->prepare('INSERT INTO books_pre 
    (title, author, publication, container, tmp_no, created_by, created_at) 
    VALUES (:B, :C, :D, :E, :A, :user_id, now())');

try {
    $this->db->beginTransaction();
    foreach(data as $i => $row) {
        // skip the first two lines
        if ($i == 0 or $i == 1) {
            continue;
        }
        $row['user_id'] = $_SESSION['user_id'];
        $stmt->execute($row);
    }
    $this->db->commit();
} catch (Exception $e) {
    $this->db->rollback();
    throw new $e;
}

Given you already have an associative array we can use its keys as placeholder names.

PDO's execute() method can accept an associative array with data, for example

$stmt = $pdo->prepare("SELECT * FROM t WHERE id=:id");
$stmt->execute(['id'] => $id);

As you can see, we are sending an array directly into execute(), which keys that must match placeholder names. Given your array already have keys, I just used them as placeholder names in the query.

However, it'a a matter of taste and you can keep use your bind() calls instead.

An important note, there should be no such thing like echo "Failed: " in your code. Always just throw an error and let it be echoed by PHP.

On a side note, a comment like this

    // Start transaction
    $this->db->beginTransaction();

makes very little sense. Do not write comments that just duplicate a code below. Use comments to explain something not that obvious.

\$\endgroup\$
  • \$\begingroup\$ can u bit it explain me in words that, whats happening on foreach and inside of it \$\endgroup\$ – Looper Jul 10 '18 at 8:44
  • \$\begingroup\$ what happens on foreach is explained on the respective manual page, I believe, as of the inside I added the explanation. \$\endgroup\$ – Your Common Sense Jul 10 '18 at 8:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.