1
\$\begingroup\$
function loopFunction(PDO $pdo, $products) {
    $pdo->beginTransaction();
    foreach($products as $uid => $element) {
        $isPremium = $element[0] != false && in_array('isPremium', $element[0]) ? 1 : 0;
        $isExclusive = $element[0] != false && in_array('isExclusive', $element[0]) ? 1 : 0;
        $isLimited = $element[0] != false && in_array('isLimited', $element[0]) ? 1 : 0;
        $IsNew = $element[0] != false && in_array('IsNew', $element[0]) ? 1 : 0;
        $DirectCallId = isset($element[1]) ? $element[1] : false;

        insertOrUpdate($pdo, $isPremium, $isExclusive, $isLimited, $IsNew, $DirectCallId, $uid);
    }
    

    $pdo->commit();
}

function insertOrUpdate(PDO $pdo, $isPremium, $isExclusive, $isLimited, $IsNew, $DirectCallId, $uid) {

    $sql = "SELECT COUNT(*) AS numberRows FROM [PaynowCom].[DirectCall].[Report] WHERE ReportId = (:value1)";
    $stmt = $pdo->prepare($sql);
    $stmt->bindParam(':value1', $DirectCallId);
    $stmt->execute();
    $count = (int)$stmt->fetchColumn();

    if ($count) {
        $sql2 = "UPDATE TOP(1) [PaynowCom].[DirectCall].[Report]
        SET isPremium = (:value1), isExclusive= (:value2), isLimited = (:value3), IsNew = (:value4)
        WHERE reportId = (:value5)";
        $stmt2 = $pdo->prepare($sql2);
        
        $stmt2->bindParam(':value1', $isPremium);
        $stmt2->bindParam(':value2', $isExclusive);
        $stmt2->bindParam(':value3', $isLimited);
        $stmt2->bindParam(':value4', $IsNew);

        $stmt2->bindParam(':value5', $DirectCallId);
        if (!$stmt2->execute()) {
            print_r('error');
        }
    } else {
        $sql2 = "INSERT INTO [PaynowCom].[DirectCall].[Report] 
        (created, reportId, uid, isPremium, isExclusive, isLimited, IsNew)
        VALUES (getdate(), :value1, :value2, :value3, :value4, :value5, :value6)";

        $stmt2 = $pdo->prepare($sql2);
        
        $stmt2->bindParam(':value1', $DirectCallId);
        $stmt2->bindParam(':value2', $uid);
        $stmt2->bindParam(':value3', $isPremium);
        $stmt2->bindParam(':value4', $isExclusive);
        $stmt2->bindParam(':value5', $isLimited);
        $stmt2->bindParam(':value6', $IsNew);

        if (!$stmt2->execute()) {
            print_r('error');
        }
    }

I think this can be improved a lot, but I am more concerned about how the transaction would work, because I am doing a conditional insert inside the code and I am wondering if the transaction would work as expected since we have multiple MSSQL prepared statements and not just 1 for each element.

I think the insert or update could be improved and we could use something else, but I am more preoccupied with the transaction and how it works with pdo.

I heard that conditional insert can cause duplicate keys or deadlocks, but I am not too sure about that.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Why are you using a transaction here? In what way do the rows, you update or insert, depend upon each other? \$\endgroup\$ Commented Feb 5, 2022 at 10:11
  • \$\begingroup\$ I was wondering about that, because initially I wanted to do a batch insert and it made sense to do it in a transaction to reduce the number of calls, but now that I am making several calls and I do a check with if, I am not sure if it would work as intended. \$\endgroup\$
    – Sayaman
    Commented Feb 5, 2022 at 23:32

1 Answer 1

1
\$\begingroup\$

When you attempt to UPDATE the "first" row of a result set without using an ORDER BY clause in the SELECT, there is no guarantee that you are actually affecting the latest entry. Is it safe to use LIMIT without ORDER BY

I don't personally have any experience with MSSQL yet, but I think you should definitely read and absorb the insights at Solutions for INSERT OR UPDATE on SQL Server to understand the options and concerns when doing an "upsert".

Using a transaction is probably better intentioned when you implement it within your insertOrUpdate() function.

Your function naming is way too vague to be helpful/searchable within your project. Without knowing more context, I might suggest batchSaveProductPermissions(). I also find $element to be an unhelpful name for what I am interpreting as a "row" that contains "permission details". Since you are only accessing the first two elements of the row, you can even try "array destucturing" syntax inside the foreach if you like.

I see individual variable assignments in your foreach loop, but several of those variables seem to be logically related/group-able since they relate to permission values. I might package the permissions and the directCallId as an associative array before passing it into insertOrUpdate(). This will reduce some horizontal bloat and simplify maintenance when you need to extend the permission options in the future. (Generalized Demo) You can use null coalescing to replace isset() ? : syntax. I recommend not juggling different names for the same data; align $DirectCallId with your database's reportId.

function batchSaveProductPermissions(PDO $pdo, array $products): void {
    $allPermissions = ['isPremium', 'isExclusive', 'isLimited', 'IsNew'];
    $defaultPermissions = array_fill_keys($allPermissions, 0);

    foreach ($products as $uid => [$permissions, $directCallId]) {
        $payload = array_fill_keys(
            array_intersect($allPermissions, $permissions),
            1
         )
         + $defaultPermissions
         + ['reportId' => $directCallId ?? false];    
        insertOrUpdate($pdo, $uid, $payload);
    }
}

Now your receiving function can begin like this:

function insertOrUpdate(PDO $pdo, int $uid, array $payload):void {

Then within your insertOrUpdate() function, you can leverage the beauty of the associative payload to avoid writing those vague :value# placeholders -- now you can use very clear placeholder names. You can even skip those bindParam() calls by directly feeding an associative array into the execute() calls. (Some minor manipulations to the arrays will be required depending on the query.)

To avoid get_date() in your INSERT queries, apply the DEFAULT value on insertion to be the current date and let your database handle this automatically.

As a general rule, I prefer to return the affected row count from UPDATE functions and return the insertId from INSERT functions. In this case, you will expect to affect, at most, one row, so I'd probably return the id of the inserted/affected row. I realize that your code isn't currently interested in this return value, but I find that it helps with debugging if this technique is already in place.

Finally, do not print errors from a querying function. You can throw and catch exceptions if you like, but avoid expressly printing from within the function body.

\$\endgroup\$
1
  • \$\begingroup\$ I need to catch a concern in my own scripting. Might your $products array possibly have a row that doesn't have a second element ($element[1])? If this is possible, then my array destructuring technique is ill-suited. @Sayaman \$\endgroup\$ Commented Feb 7, 2022 at 3:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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