How could I improve this code

[Andy11548]

Is there a way to improve this code, if so, could you please help me, I like to learn the best ways of doing codes and my code just looks too much, but it works 100% how I want it to.

 

<?php
include './includes/header.php';
include './includes/scripts/shoutbox.php';

$catsQ = mysql_query("SELECT * FROM `forum_cats` ORDER BY `id` ASC");
$catsF = mysql_fetch_assoc($catsQ);
?>

<div id="forumContainer">
   <div id="forumHolder">
      <?php
         $catsQ = mysql_query("SELECT * FROM `forum_cats`");
         $catsF = mysql_fetch_assoc($catsQ);
         
         do
         {
            echo '<div class="forumCats">'.$catsF['name'].'</div>';
            
            $subsQ = mysql_query("SELECT * FROM `forum_subs` WHERE `cat`='".$catsF['name']."'");
            $subsF = mysql_fetch_assoc($subsQ);
            
            do
            {
               $topicQ = mysql_query("SELECT * FROM `forum_topics` WHERE `sub_cat_id`='".$subsF['id']."'");
               $topicR = mysql_num_rows($topicQ);
               
               $replyQ = mysql_query("SELECT * FROM `forum_replys` WHERE `sub_cat_id`='".$subsF['id']."' ORDER BY `id` DESC");
               $replyF = mysql_fetch_assoc($replyQ);
               $replyR = mysql_num_rows($replyQ);
               
               $uidQ = mysql_query("SELECT * FROM `users` WHERE `username`='".$replyF['username']."'");
               $uidF = mysql_fetch_assoc($uidQ);

               echo '<div class="forumSubs">
                        <div class="subsLeft"><a href="view.php?id='.$subsF['id'].';currentpage=1">'.$subsF['name'].'</a><br /><span>'.$subsF['description'].'</span></div>
                        <div class="subsRightH">
                           <div class="subsRLeft">Posts: '.$replyR.'<br />Topics: '.$topicR.'</div>
                           <div class="subsRRight">';
                           
                           if($replyR > 0)
                           {
                              echo 'Last post by: <a href="profile.php?id='.$uidF['id'].'">'.$replyF['username'].'</a><br />in: <a href="topic.php?id='.$replyF['topic_id'].';currentpage=1">'.$replyF['topic_name'].'</a><br />';
                              if($replyF['date'] == date('d/m/Y'))
                              {
                                 echo '<strong>Today</strong> at '.$replyF['time'];
                              } else {
                                 echo 'on '.$replyF['date'].' at '.$replyF['time'];
                              }
                           } else {
                              echo 'No posts';
                           }
               echo        '</div>
                        </div>
                        <div class="clearfloat"></div>
                     </div>
                     ';
            } while($subsF = mysql_fetch_assoc($subsQ));

         } while($catsF = mysql_fetch_assoc($catsQ));
      ?>
   </div>
</div>
<?php
include './includes/footer.php';
?>

 

Thanks,

Andy.

[trq]

I would start by looking at some tutorials on sql's JOIN syntax. You have far to many queries.

[Andy11548]

Yeah, I think thats the only problem I have to be honest, is the fact I have like 6 different querys.

 

Does that mean it would take long to load if it was on a web hosting? Or would it load just the same as on localhost?

[teynon]

This is a bit rough, but you could play around with this query in PHPMyAdmin to try and build a query that selects everything in one patch.

 

SELECT * FROM `forum_cats` as t1, `forum_subs` as t2, `forum_topics` as t3, `forum_replies` as t4, `users` as t5 WHERE t3.sub_cat_id = t2.id AND t4.sub_cat_id = t2.id AND t5.username = t4.username

 

You could also add a stripper pole. Stripper poles always improve things.

[gristoi]

It is a matter of maths. If you had, for example, 6 queries that perform a set of tasks compared against 1 query that returns the same results as the 6, then you had 100 hits per second on your site executing the queries on that page then you would end up with 600 calls to the database compared to 100. So scale that up and imagine how much impact that would have on your server. Take thorpes good advice and look up on joins

[teynon]

As an answer to your question, you have a loop inside a loop.

 

That means if you have a forum topic with 10 replies, you have to execute 11 queries. (A bit more in your case the way you are doing it. You might not want to do what I did above, but you can still get rid of the loop inside a loop thing by joining the select statements.

[trq]

It's not the only problem, but it is where I would suggest you start. Another, for instance is that fact that you don't bother checking your queries execute successfully or return any results before going ahead and using those results.

 

But yeah, it is of course going to take longer, use more resources and be less efficient to execute multiple queries.

[teynon]

I wouldn't use do-while for that either, but that's just my opinion. I still use the old fashioned while ($req=@mysql_fetch_array($query))

[Andy11548]

I tried removing the loop inside the loop but then it doesn't select the correct information,

 

Is this where the "joined" querys would sort that problem out for me?

------------------------------

As to the "executing", should I add a "or die" to the end of them saying "There was a problem executing the querys, please contact a Administrator" or something?

------------------------------

I knew the "do while" would pop up in here somewhere, when I used "while", it never worked the same, for some very strange reason

 

Thanks,

Andy.

[teynon]

I would do it like this:

 

$sql="SELECT * FROM TABLE";
if ($query=@mysql_query($sql)) {
   if (@mysql_num_rows($query) > 0) {
      while ($req=@mysql_fetch_array($query)) {
          // Output goes in here.
          print_r($req);
      }
   }
   else {
     echo "NO ROWS SELECTED";
   }
}
else {
   echo "ERROR MESSAGE";
}

[trq]

Just a note on teynon's code. There really is no need for all the error suppressors.

[Andy11548]

I would do it like this:

 

$sql="SELECT * FROM TABLE";
if ($query=@mysql_query($sql)) {
   if (@mysql_num_rows($query) > 0) {
      while ($req=@mysql_fetch_array($query)) {
          // Output goes in here.
          print_r($req);
      }
   }
   else {
     echo "NO ROWS SELECTED";
   }
}
else {
   echo "ERROR MESSAGE";
}

 

Thanks for this, I will look into using this as soon as I learn query combinding!

 

-------------------------

 

This is going to sound lazy, but, would any of you be willing to teach me the secret about query joining?

 

I would really appriciate it,

 

Thanks,

Andy.

[trq]

There are plenty of tutorials around. We don't really need to write another.

 

Go have a look, if you get stuck, come back and ask a more specific question.

[teynon]

@thorpe, force of habit. I don't like errors / warnings in production :/

[Andy11548]

Right, I'm guessing teynon's example previously would be query joining, as it looks something like that.

 

So, if I wanted to combine:

$topicQ = mysql_query("SELECT * FROM `forum_topics` as q1 WHERE `sub_cat_id`='".$subsF['id']."'");

AND

 

 $replyQ = mysql_query("SELECT * FROM `forum_replys` WHERE `sub_cat_id`='".$subsF['id']."' ORDER BY `id` DESC");

 

It would be something like this?

 

 $q = mysql_query("SELECT * FROM `forum_replys` as q1, `forum_topics` as q2 WHERE q1.sub_cat_id='".$subsF['id']."' AND q2.sub_cat_id='".$subsF['id']."' ORDER BY q1.id DESC");

[teynon]

The secret for me for joining sql queries is to play with it in PHPMyAdmin. Go to your database, then click on the edit SQL and try to join statements in there. It will give you an error if you don't do it right. Just keep working through the errors and issues until it selects it the way you want.

[Andy11548]

Right, I understand query joining now.

 

The next question is...

 

How do I use the 'mysql_fetch_assoc' function with it?

[teynon]

You can determine the output fields in your joined statement like this:

 

select t1.id as ID, t2.id as ID2, t1.FIELD as NEWNAME FROM blah as t1, foo as t2

 

I wouldn't use mysql_fetch_assoc I would use mysql_fetch_array like in the example I posted.

 

Google "mysql_fetch_assoc vs mysql_fetch_array" and you can see some discussions on why I prefer that.

[Andy11548]

Right, ok. I got my query

 

SELECT * FROM `forum_cats` AS cats, `forum_subs` AS subs, `forum_topics` AS topics, `forum_replys` AS replys, `users` AS users WHERE subs.cat=cats.name AND topics.sub_cat_id=subs.id AND replys.sub_cat_id=subs.id AND users.username=replys.username ORDER BY cats.id ASC, replys.id DESC

 

Now I don't have a clue how the eff to use it, haha .

[teynon]

If you run that statement in PHPMyAdmin, what does it return?

[Andy11548]

Sorry, I was at the gym, this is what it returns:

 

Showing rows 0 - 6 (7 total, Query took 0.0030 sec)

[Andy11548]

I'm not going to lie, joining all thoes querys together really did not work how it does if they're on thier own.

 

It's showed the same cat/sub cat about 50000 times.

[teynon]

Unless someone provides an example before then, I will make a working example with your exact code tonight.

[Andy11548]

I'll PM you the page without the joined querys so you can see how it is.

 

Thanks alot,

Andy .

[teynon]

Here is the query I have constructed. Took about 20 minutes to work through. I will post the PHP code shortly. Your database design is pretty flawed, by the way.

 

SELECT 
t3.name as Name,
t1.name as SubName, 
t1.description as SubDescr,
t4.topic_name as TopicName, 
t4.id as TopicId, 
t5.username as Username,
(SELECT COUNT(*) FROM forum_topics as t6 WHERE t6.sub_cat_id = t1.id) as Topics,
(SELECT COUNT(*) FROM forum_replys as t7 WHERE t7.sub_cat_id = t1.id) as Replies

FROM 
forum_subs as t1 
	LEFT JOIN forum_topics as t2 
		ON t2.sub_cat_id = t1.id 
	INNER JOIN forum_cats as t3 
		ON t3.name = t1.cat 
	LEFT JOIN forum_replys as t4 
		ON t4.sub_cat_id = t1.id 
	LEFT JOIN users as t5 
		ON t5.username = t4.username 
GROUP BY t1.name 
ORDER BY 
t3.id ASC, t1.name ASC, t4.id DESC