How to refactor my iterated sequence of queries?

Question

I'm building a component which is about to be finalized, but there is a bit of code that I want to refactor. I think this code can be completed in one statement and I used more than 3 statements.

$db = JFactory::getDBO();    
$query  = $db->getQuery(true);
foreach ($pks as $i => $pk)
{
    //getting the sid from student table
    $query->clear();
    $query->select('sid')
          ->from('#__student')
          ->where('id = '.$pk);    
    $db->setQuery($query); 
    $sid = $db->loadResult(); 
    $query->clear();


    //fetching the data of old entry from log table using sid
    $query->select('*')
          ->from('#__log')
          ->where('sid = '.$sid)
          ->where('pas= 0')
          ->where('stops=0');
    $db->setQuery($query); 
    $obj=$db->loadObject('stdClass');

    //updating the old entry in log table
    $query->clear();
    $query->update('#__log')
          ->set('pas = 1')
          ->where('sid = '.$sid)
          ->where('pas= 0')->where('stops=0');
    $db->setQuery($query); 
    $db->execute();

    //I'm doing this to know about is there at-least one row affected if not then we will not insert a new recorded 
    $count=$db->getAffectedRows();
    //after setting pass to 1 we will ++ class by 1 and year by 1  
    $obj->class=$obj->class+1;
    $obj->year=$obj->year+1;

    if($count == 1)
    {
        $query->clear();
        $query->insert('#__log')
              ->columns('class, year, sid, prmoted')
              ->values($obj->class.','.$obj->year.','.$sid.','. 1);
        $db->setQuery($query);
        $db->execute();
    }       

In this code, first I select the current student record from the #__log table for next use. Then I update the same record and set pass to 1. Then using previously selected data, I insert a new record in same log table with class+1 and year+1 values.

I think this code can be highly refactored can any body help me?

Answer 1

At least I see a way how to combine your first 2 select queries into one with the help of SQL JOIN:

 $query->select('sid')->from('#__student')->where('id = '.$pk);    
$db->setQuery($query); 
$sid = $db->loadResult(); 
$query->clear();


//fetching the data of old entry from log table using sid
$query->select('*')->from('#__log')->where('sid = '.$sid)->where('pas= 0')->where('stops=0');
$db->setQuery($query); 
$obj=$db->loadObject('stdClass');

becomes something like:

$query
    ->select('l.*')
    ->from('#__log l')
    ->leftJoin('#__student s ON s.sid = l.sid')
    ->where('s.id = ' . (int) $pk)
    ->where('l.pas = 0')
    ->where('l.stops = 0')
$obj = $db->setQuery($query)->loadObject();

(not tested)

I don't see a way to make your SELECT, UPDATE and INSERT in one step.

Answer 2

To test my suggested code-refactor, I'd need access to your table structures and a fair chunk of sample rows. I'll take a stab at it anyway...

Essentially, you should perform 1 select query to isolate all rows to be processed. By including the class and year values in your qualifying result set, you will be able to construct a single batch of updates and a single batch of inserts.

Keeping total trips to the database minimal is certainly one instance of best practices. My snippet will make 1 trip if there are no qualifying rows, and 3 trips if there are qualifying rows. Your originally code was looking to make N*3 trips to the database. My refactor is a MAJOR performance improvement.

This code is not tested (...if anyone spots a mistake please comment):

try {
    // generate result set of qualifying rows which need to be updated and inserted
    $db = JFactory::getDBO();
    $select_query = $db->getQuery(true)
        ->select($db->qn(["a.sid", "a.class", "a.year"]))
        ->from("#__log a")
        ->innerJoin("#__student b ON a.sid = b.sid")
        ->where([
            "a.pas = 0",
            "a.stops = 0",
            "b.id IN (" . implode(',', array_map(function($n){return (int)$n;}, $pks)) . ")"  // just in case $pks is not secure
        ]);
    // don't show rendered queries to the public
    JFactory::getApplication()->enqueueMessage("<div>Rendered Select Query<br><b>" . $select_query->dump() . "</b></div>", 'notice');
    $db->setQuery($select_query);

    if (!$log_rows = $db->loadAssocList()) {
        "No qualifying student logs to process";
    } else {
        $insert_query = $db->getQuery(true)
            ->insert('#__log')
            ->columns($db->qn(["class", "year", "sid", "prmoted"]));

        foreach ($log_rows as $row) {  // generate batched data for just two total queries
            $update_ids[] = (int)$row['sid'];
            $insert_query->values((1 + $row['class']) . ", " . (1 + $row['year']) . ", " . (int)$row['sid'] . ", 1");
        }

        $update_query = $db->getQuery(true)
            ->update("#__log")
            ->set("pas = 1")
            ->where("sid IN (" . implode(",", $update_ids) . ")");

        // don't show rendered queries to the public
        JFactory::getApplication()->enqueueMessage("<div>Rendered Update Query<br><b>" . $update_query->dump() . "</b></div>", 'notice');
        $db->setQuery($update_query);
        $db->execute();
        JFactory::getApplication()->enqueueMessage("<div>" . $db->getAffectedRows() . " Rows Of Data Updated", 'success');

        // don't show rendered queries to the public
        JFactory::getApplication()->enqueueMessage("<div>Rendered Insert Query<br><b>" . $insert_query->dump() . "</b></div>", 'notice');
        $db->setQuery($insert_query);
        $db->execute();
        JFactory::getApplication()->enqueueMessage("<div>" . $db->getAffectedRows() . " Rows Of Data Inserted", 'success');
    }
} catch (Exception $e) {  // this will halt the execution within the try block as soon as something errs
    // don't show actual error message to public
    JFactory::getApplication()->enqueueMessage("Query Syntax Error: " . $e->getMessage(), 'error');
}