Page 1 of 1

Please assist in streamlining this PHP

Posted: Wed Nov 18, 2009 2:20 pm
by j1048
Hello,

My PHP is of a standard now that needs to be lifted to the next level - I can make things work, but I know there's a better way to do it - however I'm not sure where to start.

My problem is this, which is some PHP code to read data from an online poll and display a bar chart. It makes an SQL call, calculates the percentage and length of each bar and passes that to the CSS to draw the bars. It works great - but it's not pretty, and I know there's a way to make just one SQL call to build an array of some kind, then loop through the array performing the calculations and presenting the HTML. At the moment there's 4 identical functions being performed at each stage - there must be a way to build a function or a class to handle this... but my OOP is not great!

If someone could point me in the right direction, either with some revised code or somewhere I can read up on this kind of OOP or array looping I'd be grateful!

Thanks

Code: Select all

 
<?php           
            $sql = mysql_query("SELECT COUNT(*) FROM question1 WHERE answer='1'");
            $count1 = mysql_fetch_row($sql);
            $count1 = $count1[0];
            
            $sql = mysql_query("SELECT COUNT(*) FROM question1 WHERE answer='2'");
            $count2 = mysql_fetch_row($sql);
            $count2 = $count2[0];
            
            $sql = mysql_query("SELECT COUNT(*) FROM question1 WHERE answer='3'");
            $count3 = mysql_fetch_row($sql);
            $count3 = $count3[0];
            
            $sql = mysql_query("SELECT COUNT(*) FROM question1 WHERE answer='4'");
            $count4 = mysql_fetch_row($sql);
            $count4 = $count4[0];
                
            $total = $count1 + $count2 + $count3 + $count4;
            
            $count1 = ($count1 / $total);
            $count1pc = round($count1 * 100);
            $bar1 = round($count1 * 200);
            
            $count2 = ($count2 / $total);
            $count2pc = round($count2 * 100);
            $bar2 = round($count2 * 200);
            
            $count3 = ($count3 / $total);
            $count3pc = round($count3 * 100);
            $bar3 = round($count3 * 200);
            
            $count4 = ($count4 / $total);
            $count4pc = round($count4 * 100);
            $bar4 = round($count4 * 200);
            
            print '<div class="answer_item"><div class="answer_title">Answer 1: </div><div class="bar" style="background-color: #D84848; width: ' . $bar1 . 'px;"></div><div class="pc">'.$count1pc.'%</div></div>';
            print '<div class="answer_item"><div class="answer_title">Answer 2: </div><div class="bar" style="background-color: #CDC42B; width: ' . $bar2 . 'px";></div><div class="pc">'.$count2pc.'%</div></div>';
            print '<div class="answer_item"><div class="answer_title">Answer 3: </div><div class="bar" style="background-color: #0F7794; width: ' . $bar3 . 'px;"></div><div class="pc">'.$count3pc.'%</div></div>';
            print '<div class="answer_item"><div class="answer_title">Answer 4: </div><div class="bar" style="background-color: #E4802B; width: ' . $bar4 . 'px;"></div><div class="pc">'.$count4pc.'%</div></div>';
            
            ?>

Re: Please assist in streamlining this PHP

Posted: Wed Nov 18, 2009 3:26 pm
by Weiry
Try this out,
the code itself is fairly straight forward with comments included.
Basically each stage of the code runs $numQueries amount of times.

Code: Select all

 
<?php
$numQueries = 4; // define the number of queries you want to run
 
// Set up initial variables
$total = 0;
 
// Store the queries
for($i = 1; $i <= $numQueries; $i++){
    $sqlArr[$i]="SELECT count(*) FROM question1 WHERE r1 = '{$i}'";
}
 
// Set up initial values
for($i = 1; $i <= $numQueries; $i++){
   $result = mysql_fetch_array(mysql_query($sqlArr[$i]));
   $resultArr[$i] = $result[0];
   $total += $result;
}
 
// Calculate the values and store them
for($i = 1; $i <= $numQueries; $i++){
    $countArr[$i] = array(
        "count{$i}" => ($resultArr[$i] / $total),
        "count{$i}pc" => round($resultArr[$i] * 100),
        "bar{$i}" => round($resultArr[$i] * 200)
    );
}
// Go and print each result
for($i = 1; $i <= $numQueries; $i++){
    $bar = "bar{$i}";
    $count = "count{$i}";
    $countpc = "count{$i}pc";
    print "<div class='answer_item'>
                <div class='answer_title'>
                    Answer {$i}: 
                </div>
                <div class='bar' style='background-color: #D84848; width:{$countArr[$i][$bar]}px;'>
                </div>
                <div class='pc'>
                    {$countArr[$i][$countpc]}%
                </div>
            </div>";
}
?>
 
Edit: made an error, should work now :)

Re: Please assist in streamlining this PHP

Posted: Wed Nov 18, 2009 3:32 pm
by requinix
Rather than execute a query for every type of answer,

Code: Select all

SELECT COUNT(1) FROM `question1` GROUP BY `answer`
Use that with a loop. A loop - no need for four of them.

Re: Please assist in streamlining this PHP

Posted: Thu Nov 19, 2009 5:57 am
by j1048
Thanks chaps,

Both examples work, and are not dissimilar to a solution I came up with myself:

Code: Select all

<?php 
 
$sql = mysql_query("SELECT * FROM question1");
$total = mysql_num_rows($sql);
 
$colour = array('D84848','CDC42B','0F7794','E4802B');
 
$i = 1;
foreach($colour as $value)
{
    $sql = mysql_query("SELECT COUNT(*) FROM question1 WHERE answer='$i'");
    $count = mysql_fetch_row($sql);
    $count = $count[0];
    
    $count = ($count / $total);
    $countpc = round($count * 100);
    $bar = round($count * 200);
    
    print '<div class="answer_item"><div class="answer_title">Answer '.$i.': </div><div class="bar" style="background-color: #'.$value.'; width: ' . $bar . 'px;"></div><div class="pc">'.$countpc.'%</div></div>';
    $i++;
}
?>
Which although uses less code, it still makes too many SQL calls - 5 in fact, and 1 more than my original script as I have to find the total. The colours array is needed to spit out the different bar colours into the loop.

But this version is an improvement:

Code: Select all

 
<?php
$sql = mysql_query("SELECT * FROM question1");
$total = mysql_num_rows($sql);
 
$colour = array('D84848','CDC42B','0F7794','E4802B');
 
$sql = mysql_query("SELECT COUNT(1) FROM `question1` GROUP BY `answer`");
 
$i = 1;
foreach($colour as $value)
{
    
    $count = mysql_fetch_row($sql);
    $count = $count[0];
    
    $count = ($count / $total);
    $countpc = round($count * 100);
    $bar = round($count * 200);
    
    print '<div class="answer_item"><div class="answer_title">Answer '.$i.': </div><div class="bar" style="background-color: #'.$value.'; width: ' . $bar . 'px;"></div><div class="pc">'.$countpc.'%</div></div>';
    $i++;
}
?>
That looks better - but am I right in thinking that it's only making 1 SQL call with the line

Code: Select all

$sql = mysql_query("SELECT COUNT(1) FROM `question1` GROUP BY `answer`");
or does the mysql_fetch_row in the loop

Code: Select all

foreach($colour as $value)
{
    
    $count = mysql_fetch_row($sql);
mean that it's getting hit 4 times? It's my understanding that once the SQL call is made the data is held in the $sql variable and that's what's looped through using mysql_fetch_row? If that's the case, why can't I print_r() any data from $sql, even when I've done a mysql_fetch_array() on it? For example:

Code: Select all

$sql = mysql_query("SELECT COUNT(1) FROM `question1` GROUP BY `answer`");
$answer = mysql_fetch_array($sql);
print_r($answer);
 
Gives

Code: Select all

Array ( [0] => 2 [COUNT(1)] => 2 )
That's not a useful array of data!

Is there a way to explain that so that it sticks in my brain?!

Thanks!

Re: Please assist in streamlining this PHP

Posted: Thu Nov 19, 2009 7:35 am
by iankent
First off I'd change this bit:

Code: Select all

$sql = mysql_query("SELECT * FROM question1");
$total = mysql_num_rows($sql);
to this:

Code: Select all

$sql = mysql_query("SELECT count(answer) as total FROM question1");
$row = mysql_fetch_assoc($sql);
$total = $row['total'];
mysql_free_result($sql); // you should always do this when you get a valid result!!
Reason being that your original query still asks MySQL to retrieve all data from the table which is unnecessary. The second version only counts a single column - better still if you can make that a numerical and indexed column, for example a unique ID column (just change it to count(id) as total or whatever, the rest stays the same)

you are also right in thinking that this line:

Code: Select all

$sql = mysql_query("SELECT COUNT(1) FROM `question1` GROUP BY `answer`");
only makes a single call to mysql_query. mysql_fetch_assoc/mysql_fetch_array/mysql_fetch_row don't go back to mysql to get the data, so no more requests to the server are made

re print_r - you can't print_r a result from mysql_query. mysql_query returns an object that can't be output using print_r, you'd need to fill an array with each row from the result and print_r that! when you print_r the $answer variable on the last bit, it's displaying exactly what you asked for - one row of the result from the query

Code: Select all

SELECT COUNT(1) FROM `question1` GROUP BY `answer`
When you call mysql_fetch_array it's giving you both two types of result in one - an associative array and a numeric array. This is why you're getting [0]=2 and [COUNT(1)]=2
What its actually telling you is the result of COUNT(1) for that row of the result is equal to 2 - doesn't matter whether you access it as $answer[0] or $answer['COUNT(1)'].

Does that not give the information you're looking for? If not, give us the table structure and exactly what info you want to get from it and I'll see what I can come up with :)

p.s. I always find it easier to name my aggregate columns in SQL and always use mysql_fetch_assoc to get a neat associative array back, i.e.:

Code: Select all

$result = mysql_query('SELECT datejoined, COUNT(datejoined) AS total FROM tbl_users GROUP BY datejoined;');
while($row = mysql_fetch_assoc($result) {
    // we now have the following vars available:
    echo 'On ' . $row['datejoined'] . ' we had ' . $row['total'] . ' new users';
}

Re: Please assist in streamlining this PHP

Posted: Thu Nov 19, 2009 12:46 pm
by j1048
Thanks Ian!

Yeah you've cleared up some things for me there, thank you.

I've been playing with this code today actually, trying to streamline it myself and I've got this far: (Below) The original script was used in 4 PHP files, for 4 bar charts, with 4 bars on each from 4 database tables. So that was a total of 16 SQL calls, and loads of separate loops and duplicate code... madness! Now it's down to one file and 40 lines of code.

So here's where it's at now:

Code: Select all

 
<?php 
function get_bar_chart($charts_array, $data_array, $table_prefix) 
{
    $run = count($charts_array);
    
    for($k = 0; $k < $run; $k++)
    {
        $table = $table_prefix.($k+1);
    
        $sql = mysql_query("SELECT COUNT(id) FROM ".$table);
        $total = mysql_fetch_row($sql);
        
        $sql = mysql_query("SELECT COUNT(*) FROM ".$table." GROUP BY `answer`");
        
        print '<div class="poll_answers">
                <div class="poll_question">'.$charts_array[$table].'</div>';
        $question = 'question'.$chart;
        for($i = 0; $i < 4; $i++)
        {
            $count = mysql_fetch_row($sql);
            $count = $count[0];
            
            $count = ($count / $total[0]);
            $countpc = round($count * 100);
            $bar = round($count * 200);
            
            print '<div class="answer_item"><div class="answer_title">'.$data_array[$k][$i]['answer'].': </div><div class="bar" style="background-color: #'.$data_array[$k][$i]['colour'].'; width: ' . $bar . 'px;"></div><div class="pc">'.$countpc.'%</div></div>';
            
        
        }
        
        print '</div>';
        
    }
}
 
print get_bar_chart($charts, $data, 'question'); // Chart array, data array and mysql table prefix
?>
 
Information for the question titles and bar colours comes from a couple of arrays:

Code: Select all

 
$charts = array(question1=>'How do you get to work?', question2=>'What is your prefered drink?');
 
$data = array(
            array(
                array(colour=>'D84848', answer=>'Train'),
                array(colour=>'CDC42B', answer=>'Bus'),
                array(colour=>'635CFF', answer=>'Car'),
                array(colour=>'20C411', answer=>'Walk')
                ),
            array(
                array(colour=>'D84848', answer=>'Coffee'),
                array(colour=>'CDC42B', answer=>'Tea'),
                array(colour=>'635CFF', answer=>'Water'),
                array(colour=>'20C411', answer=>'Other')
                )
            );
 
Which could just as easily be populated and controlled from another database table.

So that function get_bar_chart() takes the arrays and loops through them at the same time as the SQL results, spitting out the HTML for as many bar charts as you give it info for.

At the moment it performs 1 SQL call for the total number, and 1 more for the GROUP BY count of each answer - per chart, for a total of 8 SQL calls, which is an improvement over the previous incarnation.

Can it go further? Can we get the total number of votes AND the GROUP BY count in 1 SQL Call?

Also can I take the function get_bar_chart() further? I'm sure there's a more efficient way to organise the array data and looping structure.....

Re: Please assist in streamlining this PHP

Posted: Thu Nov 19, 2009 1:03 pm
by iankent
Are you getting this info from separate tables? It looks that way as you're using $table = $table_prefix.$k.

If so, then you can get it done in one query but there's probably not much benefit from doing so. You'd need to create one query for each table (all returning the same columns) and UNION them together to get the final result, but from a mysql point of view its the same as running separate tables (more resource intensive if doing further order by's or limits on the UNION'd resultset)

If its coming from the same table (or, could be made to come from the same table), you could alter the query to retrieve the count(id) for all of them using a GROUP BY.

Also, you should change the COUNT(*) bit of the second query to count a single column otherwise MySQL needs to retrieve the full row rather than just a single column