Listing WordPress posts with thumbnail images

Question

I am using the following code poor form. I am interested in how I can optimise this and use the correct methods. I understand the DRY concept and will look to employ that throughout my code. However, this works and its pretty quick too, even with large result sets. I would like to learn and improve this code.

   <?php

global $current_user;
get_currentuserinfo();

$user_id = $current_user->ID;
$blogUrl = get_site_url();
global $wpdb;
$DBquery = "
SELECT * 
FROM saved_collection
WHERE userid = '".$user_id."'
";

$result = $wpdb->get_results($DBquery); 

$ids = array();
foreach($result as $row)
{

$collectionID = $row->id;
$collection_name = $row->collection_name;
$collectionURL = '<a href ="' . $blogUrl . '/client/?gid=' .$user_id. '&c=' .$collectionID. '"target="_blank">Programme Link</a>';

echo '<div class="well">';
echo '<h1>' . $collection_name . '</h1>';
echo  $collectionURL;
echo '<div class="clearfix"></div>';
$cleandata = unserialize($row->collection_data);

  foreach($cleandata as $data)
  {
          $ids[] = $data['ExerciseID'];
        // START OF QUERY TO GET THE FIRST IMAGE FROM EACH EXERCISE
          global $wpdb;
            $query = "
            SELECT * 
            FROM imagemap
            WHERE exercise = '".$data['ExerciseID']."'
            LIMIT 1
            ";

            $result = $wpdb->get_results($query); 

            foreach($result as $row)

                 {
                  echo '<div class="imageGridImages imageGridImagesLarge">';
                // START OF QUERY TO GET NAME OF EACH IMAGE
                    global $wpdb;
                    $exerciseTitleQuery = "
                    SELECT post_title 
                    FROM wp_posts
                    WHERE id = '".$data['ExerciseID']."'
                    LIMIT 1
                    ";
                    $exerciseTitleQueryResult = $wpdb->get_results($exerciseTitleQuery);
                    foreach ($exerciseTitleQueryResult as $returned_post_title) 
                        {echo '<div class="thumbnail-title-block">';
                             echo '<p>' .$returned_post_title->post_title. '</p>';
                             echo '</div>'; 
                        }
                // END OF QUERY TO GET NAME OF EACH IMAGE

                echo "<img src=".$blogUrl. "/image/medium/".$row->id. ".jpg />";
                    //echo '<div class="image display" style="background-image: url(' .$blogUrl.'/image/thumb/' .$row->id. '.jpg);"></div>';


                  echo '</div>'; 
                  // end of div imageGridImages
         }
  }

echo '<div class="clearfix"></div>';
echo '</div>';

}
?>

Answer 1

One of the nested loops can be eliminated, by joining these two queries:

$query = "
SELECT * 
FROM imagemap
WHERE exercise = '".$data['ExerciseID']."'
LIMIT 1
";

$result = $wpdb->get_results($query); 

foreach($result as $row) {
    echo '<div class="imageGridImages imageGridImagesLarge">';
    global $wpdb;
    $exerciseTitleQuery = "
    SELECT post_title 
    FROM wp_posts
    WHERE id = '".$data['ExerciseID']."'
    LIMIT 1
    ";

This code is inefficient, because for every row in the first query, you run another query. For example if there are 10 rows in the first query, you'll be running 11 queries. Queries can be expensive, it's better to minimize them as much as possible, for example by selecting multiple rows in bulk instead of doing it one by one. Since in both the outer and the inner queries the parameter is the same, $data['ExerciseID'], you should be able to use a JOIN instead in a single query.

Another problem you have in the first query is that SELECT * is not recommended. You should specify exactly the columns you need, and their order. When you use SELECT *, you might be selecting too much, downloading more data than you really need, and if the order columns change in the database or new columns are added, you could have nasty bugs.

Answer 2

This is going to be a bit of a drive-by code review due to time limitations and my limited php knowledge, but hopefully I can add some value here. Here are some high-level things I've noticed about this code:

1. PARAMETERIZE YOUR QUERIES!. This is the most common security hole found on the web, so make sure you understand the problem and how to combat it.
2. Use consistent indentation/formatting. Right now your code is hard to mentally parse since the indentation is not consistent and there are lots of random comments, line breaks, etc. Take a look for a PHP code style guide that suits you best and practice following it. Soon you'll unconsciously know where to find certain elements in the code, and quickly identify things that are out of place.
3. Follow the separation of concerns design principle. Right now you have a page/method that is responsible for too many things. If you break it into distinct "concerns", it'll be easier to modify, test, diagnose, etc. For example, you could split this into methods that: 1) Parse your page inputs into strongly typed variables. 2) Query your database. 3) Format data into a web response.
4. Use string formatting rather than massive amounts of concatenation. It'll make your result a lot easier to understand/modify.

Answer 3

Sometimes the nested loops cannot be avoided in certain areas, however the purpose is not just to have optimized code, but also more readable code.

The code needs to be separated as units or each logic in a function. So there should be one function which does only one foreach and the arguments comes from another function which calls this, should be calling this function inside a foreach.

The approach here is to make it more readable and clear what you are doing.

Additional Note:

Based on the code found.

• Don't mix HTML and DB queries.... as said by Ocelot20 use design principles.
• Separate layers with MVC, keep your presentation, logic and queries in different area.
• Preferably, use Composer and manage your dependencies. Go for frameworks like, Doctrine, Zend, Symphony, CakePHP, or something that helps you to do it better.