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
          ->where('id = '.$pk);    
    $sid = $db->loadResult(); 

    //fetching the data of old entry from log table using sid
          ->where('sid = '.$sid)
          ->where('pas= 0')

    //updating the old entry in log table
          ->set('pas = 1')
          ->where('sid = '.$sid)
          ->where('pas= 0')->where('stops=0');

    //I'm doing this to know about is there at-least one row affected if not then we will not insert a new recorded 
    //after setting pass to 1 we will ++ class by 1 and year by 1  

    if($count == 1)
              ->columns('class, year, sid, prmoted')
              ->values($obj->class.','.$obj->year.','.$sid.','. 1);

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?


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);    
$sid = $db->loadResult(); 

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

becomes something like:

    ->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.

  • 1
    @fruppel - I would also suggest you recommend quoting column names and values within the query ;) – Lodder Apr 16 '15 at 8:11
  • 1
    @Lodder I used quoting heavily on all my SQL queries some time ago. But today I am not sure if it is really necessary. At least not for column names that are static. If you have variable column names you should use the $db->quoteName method for sure. I use $db->quote() for all my values except for integers. This is how it is done in the core too AFAIK. I added an cast to int to the where query to be safe, thanks. Do you have any other information/recommendation about that topic? – fruppel Apr 16 '15 at 8:41
  • 1
    @fruppel - Yes, you are right. It's not always absolutely required for selecting, but for updating and inserting I deem it more important. Even if it's not absolutely necessary, I personally always do it and recommend it to others as it's good practice. I've seen a lot of questions with insert queries using variables that haven't been quoted, so it's easy to forget if not done all the time – Lodder Apr 16 '15 at 9:07
  • "thanks Mr. fruppel for kind advice and so much help... actually my working style is quit different first i'm doing my all hard work regarding business logic and then in second phase i will refactor my code for batter performance then will follow some of recommendations after that will make it well documented like proper comments and meaningful variable names " – Sumer Raj Chouhan Apr 18 '15 at 10:56

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")
            "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');

    if (!$log_rows = $db->loadAssocList()) {
        "No qualifying student logs to process";
    } else {
        $insert_query = $db->getQuery(true)
            ->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)
            ->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');
        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');
        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');

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.