current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite
1

So today I jumbled together a script that would get data from a database, from different tables and such. I had wanted to just use simple ifs without the whiles, but I couldn't make it possible. The code I posted is useable but randomly times out when testing, and i'm afraid of how it would perform under stress. Is there any way to improve this code to stop the timing out?

<?php
    $getConference = sprintf("SELECT DISTINCT confName, tableName FROM conferences WHERE enabled='1' ORDER BY id DESC");
    $catConference = mysql_query($getConference);
    while($conference = mysql_fetch_array($catConference)) {
        echo "<h2 class=\"expand\">".$conference['confName']."</h2>
                <ul class=\"collapse\">";
        $getExistingEvent = sprintf("SELECT DISTINCT event FROM " . $conference['tableName']);
        $catExistingEvent = mysql_query($getExistingEvent);
        while($existingevent = mysql_fetch_array($catExistingEvent)) {
            $getCat = sprintf("SELECT * FROM event WHERE id='".$existingevent['event']."'");
            $catResult = mysql_query($getCat);
            while($row = mysql_fetch_array($catResult)) {
                $getWinner = sprintf("SELECT place, name, event FROM 2012rlc WHERE event='".$row['id']."' ORDER BY place");
                $catWinner = mysql_query($getWinner);
                echo "
                    <li>
                        <h2 class=\"expand\">".$row['eventName']."</h2>
                        <ul class=\"collapse\">
                    ";

                while($winner = mysql_fetch_array($catWinner)) {
                    echo "<li>".$winner['place']. " ".$winner['name']."</li>";
                }
                echo "</ul></li>";
            }
        }
        echo "</ul>";
    }
    mysql_free_result($catResult);
    mysql_free_result($catWinner);
    mysql_free_result($catExistingEvent);
    mysql_free_result($catConference);
?>

Some have asked for the database schema, which I have included below: 

CREATE TABLE `2012rlc` (
    `id` int(11) unsigned NOT NULL auto_increment,
    `place` varchar(4) default '',
    `name` varchar(255) default NULL,
    `event` int(5) default NULL,
    PRIMARY KEY  (`id`)
) ENGINE=MyISAM AUTO_INCREMENT=52 DEFAULT CHARSET=latin1;

CREATE TABLE `conferences` (
    `id` int(11) unsigned NOT NULL auto_increment,
    `confName` varchar(255) default NULL,
    `tableName` varchar(50) default NULL,
    `enabled` tinyint(1) default '0',
    PRIMARY KEY  (`id`)
) ENGINE=MyISAM AUTO_INCREMENT=5 DEFAULT CHARSET=latin1;


CREATE TABLE `event` (
    `id` int(11) unsigned NOT NULL auto_increment,
    `eventName` varchar(255) default NULL,
    `teamEvent` tinyint(1) default '0',
    PRIMARY KEY  (`id`)
) ENGINE=MyISAM AUTO_INCREMENT=47 DEFAULT CHARSET=latin1;

Output to EXPLAIN MySQL command:

id  select_type table   type    possible_keys   key key_len ref rows    Extra
1   SIMPLE  conferences ALL NULL    NULL    NULL    NULL    4   Using where; Using temporary; Using filesort
share|improve this question
    
What is the difference between conferences.confName and 2012rlc.name? Is the schema in 3NF? –  palacsint Dec 19 '11 at 22:21

1 Answer 1

up vote 1 down vote

First of all, does you tables have indexes? If you run the queries with EXPLAIN on MySql console (for example EXPLAIN SELECT DISTINCT confName, tableName FROM conferences WHERE enabled='1' ORDER BY id DESC), does it show that it uses indexes?

I'm not sure that it's a real performance issue or not but the current mysql_free_result calls doesn't free all resources, just the last one because the code runs lots of queries in loops. You should put these calls immediately after the last use of the resource, for example:

while (...) {
    ...
    while($winner = mysql_fetch_array($catWinner)) {
        echo "<li>".$winner['place']. " ".$winner['name']."</li>";
    }
    // free $catWinner at the end of every iteration
    mysql_free_result($catWinner);
}

I don't think that (generally) a good design to create separate tables for (probably) the same type of records. Instead of creating separate tables for every conference, consider creating only one table. It should have a conferenceId attribute as well as the conferences table should have a conferenceId too (instead of the tableName attribute). (Don't forget the foregin keys.)

If I'm right, this conferenceId could be in the event table and you don't need separate conference tables at all. (Feel free to post your database schema too. It could be worth a new question.)

Using sprintf does not make any sense here and in the similar calls:

$getExistingEvent = sprintf("SELECT DISTINCT event FROM " . $conference['tableName']);

It's the same as

$getExistingEvent = "SELECT DISTINCT event FROM " . $conference['tableName'];

Or it could be

$getExistingEvent = sprintf("SELECT DISTINCT event FROM %s", $conference['tableName']);

where sprintf does the including/formatting.

Edit:

I'd create the following indexes:

ALTER TABLE `conferences` ADD INDEX `enabled`(`enabled`);
ALTER TABLE `2012rlc` ADD INDEX `event_id`(`event`);

I hope it helps.

share|improve this answer
    
Alrighty, I posted my database schema in the original post. If I were to have all the data in one table, would organization be harder? Also, I have also put the output to the command you had me run in the original post as well. –  chrtr Dec 17 '11 at 18:05
    
I've updated the answer, check it, please. The schema usally should be in 3NF which removes lots of redundancy. It makes modifications easier to do. –  palacsint Dec 19 '11 at 22:24

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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