I'm looking for tips and suggestions on cleaning up some code.

Ok so what the following code does is create 2 tables and a bar chart based on the year and client name I select from a dropdown box. It might be hard to follow and I’m willing to explain any part of the code in detail. The code works for what I’m doing right now. I know a lot of you have different, more efficient, or cleaner ways of doing a lot of what I’ve done, and that’s what I’m looking for help with. I’m not opposed to reading, so if you simply toss up a link you think might help me out, I’ll read through it. Essentially, I’m looking for constructive criticism on the following. If this is the wrong section just let me know and I’ll take care of it.

code notes are marked with <!-- and //

Thanks all,
I appreciate it!

[php]

<?php mysql_connect("localhost", "root", "") or die(mysql_error()); mysql_select_db("csv_db") or die(mysql_error()); ?> <?php $query = "select client from clients"; $result = mysql_query($query); if (mysql_num_rows($result)) { echo ""; while ($row = mysql_fetch_row($result)) { print("$row[0]"); } } ?>
<?php $query2 = "select year from yeardb"; $result2 = mysql_query($query2); if (mysql_num_rows($result2)) { echo ""; while ($row2 = mysql_fetch_row($result2)) { print("$row2[0]"); } } ?> <?php $selected = $_POST['client']; $year = $_POST['year']; $yearless = $year-1; $yearless2 = $year-2 ?> <?php $query=mysql_query("SELECT yearcode FROM yeardb WHERE year = '$year'"); $query_row=mysql_fetch_array($query); $years = $query_row['yearcode'] ?> <?php $query=mysql_query("SELECT yearcode FROM yeardb WHERE year = '$yearless'"); $query_row=mysql_fetch_array($query); $yearsless = $query_row['yearcode'] ?> <?php mysql_connect("localhost", "root", "") or die(mysql_error()); mysql_select_db("csv_db") or die(mysql_error()); $query=mysql_query("SELECT yearcode FROM yeardb WHERE year = '$yearless2'"); $query_row=mysql_fetch_array($query); $yearsless2 = $query_row['yearcode'] ?> <?php $result = mysql_query("SELECT * FROM $yearsless WHERE Client='$selected'") or die(mysql_error()); echo ""; echo $selected; echo "
"; echo "$yearless YTD Import Statistics"; echo "

"; echo ""; echo " table.sample { border-width: 1px; border-spacing: 0px; border-style: solid; border-color: gray; border-collapse: collapse; background-color: rgb(255, 250, 250); } table.sample th { border-width: 1px; padding: 3px; border-style: solid; border-color: gray; background-color: rgb(255, 250, 250); -moz-border-radius: ; } table.sample td { border-width: 1px; padding: 3px; border-style: solid; border-color: gray; background-color: rgb(255, 250, 250); -moz-border-radius: ; } "; // keeps getting the next row until there are no more to get while($row = mysql_fetch_array( $result )) { // Math and shit $Totaltax = $row['TotalGST']+$row['TotalHSTPST']; $Avduty = $row['TotalDuty']/$row['OrdersTotal']; $Avtax = $Totaltax/$row['OrdersTotal']; ?> <?php // Print out the contents of each row into a table echo ""; } echo "
Client Name Month No. of Packages No. of Orders $yearless No. of Orders $yearless2 No. of Line Items Canadian Value for Duty U.S. Value for Duty TotalDuty Total Taxes GST/HST Average Duty/Order Average Tax Per Order Average U.S. Value/Order
"; echo $row['Client']; echo " "; echo $row['Month']; echo " "; echo $row['PackagesShipped']; echo " "; echo $row['OrdersTotal']; echo " "; echo "***"; echo " "; echo $row['LinesTotal']; echo " "; echo $row['TotalVFD']; echo " "; echo "***"; echo " "; echo $row['TotalDuty']; echo " "; echo $Totaltax; echo " "; echo round($Avduty,2); echo " "; echo round($Avtax,2); echo " "; echo "***"; echo "
"; ?> <?php $result = mysql_query("SELECT * FROM $years WHERE Client='$selected'") or die(mysql_error()); echo ""; echo $selected; echo "
"; echo "$year YTD Import Statistics"; echo "

"; echo ""; echo " table.sample { border-width: 1px; border-spacing: 0px; border-style: solid; border-color: gray; border-collapse: collapse; background-color: rgb(255, 250, 250); } table.sample th { border-width: 1px; padding: 3px; border-style: solid; border-color: gray; background-color: rgb(255, 250, 250); -moz-border-radius: ; } table.sample td { border-width: 1px; padding: 3px; border-style: solid; border-color: gray; background-color: rgb(255, 250, 250); -moz-border-radius: ; } "; // keeps getting the next row until there are no more to get while($row = mysql_fetch_array( $result )) { // Math and shit $Totaltax = $row['TotalGST']+$row['TotalHSTPST']; $Avduty = $row['TotalDuty']/$row['OrdersTotal']; $Avtax = $Totaltax/$row['OrdersTotal']; ?> <?php // Print out the contents of each row into a table echo ""; } echo "
Client Name Month No. of Packages No. of Orders $year No. of Orders $yearless No. of Line Items Canadian Value for Duty U.S. Value for Duty TotalDuty Total Taxes GST/HST Average Duty/Order Average Tax Per Order Average U.S. Value/Order
"; echo $row['Client']; echo " "; echo $row['Month']; echo " "; echo $row['PackagesShipped']; echo " "; echo $row['OrdersTotal']; echo " "; echo "***"; echo " "; echo $row['LinesTotal']; echo " "; echo $row['TotalVFD']; echo " "; echo "***"; echo " "; echo $row['TotalDuty']; echo " "; echo $Totaltax; echo " "; echo round($Avduty,2); echo " "; echo round($Avtax,2); echo " "; echo "***"; echo "
"; ?> <?php $result = mysql_query("SELECT OrdersTotal FROM $years WHERE Client='$selected' AND Month='January $year'") or die(mysql_error()); $row = mysql_fetch_array( $result ); $jan = $row['OrdersTotal']; $result = mysql_query("SELECT OrdersTotal FROM $years WHERE Client='$selected' AND Month='February $year'") or die(mysql_error()); $row = mysql_fetch_array( $result ); $feb = $row['OrdersTotal']; $result = mysql_query("SELECT OrdersTotal FROM $years WHERE Client='$selected' AND Month='March $year'") or die(mysql_error()); $row = mysql_fetch_array( $result ); $mar = $row['OrdersTotal']; $result = mysql_query("SELECT OrdersTotal FROM $years WHERE Client='$selected' AND Month='April $year'") or die(mysql_error()); $row = mysql_fetch_array( $result ); $apr = $row['OrdersTotal']; $result = mysql_query("SELECT OrdersTotal FROM $years WHERE Client='$selected' AND Month='May $year'") or die(mysql_error()); $row = mysql_fetch_array( $result ); $may = $row['OrdersTotal']; $result = mysql_query("SELECT OrdersTotal FROM $years WHERE Client='$selected' AND Month='June $year'") or die(mysql_error()); $row = mysql_fetch_array( $result ); $jun = $row['OrdersTotal']; $result = mysql_query("SELECT OrdersTotal FROM $years WHERE Client='$selected' AND Month='July $year'") or die(mysql_error()); $row = mysql_fetch_array( $result ); $jul = $row['OrdersTotal']; $result = mysql_query("SELECT OrdersTotal FROM $years WHERE Client='$selected' AND Month='August $year'") or die(mysql_error()); $row = mysql_fetch_array( $result ); $aug = $row['OrdersTotal']; $result = mysql_query("SELECT OrdersTotal FROM $years WHERE Client='$selected' AND Month='September $year'") or die(mysql_error()); $row = mysql_fetch_array( $result ); $sep = $row['OrdersTotal']; $result = mysql_query("SELECT OrdersTotal FROM $years WHERE Client='$selected' AND Month='October $year'") or die(mysql_error()); $row = mysql_fetch_array( $result ); $oct = $row['OrdersTotal']; $result = mysql_query("SELECT OrdersTotal FROM $years WHERE Client='$selected' AND Month='November $year'") or die(mysql_error()); $row = mysql_fetch_array( $result ); $nov = $row['OrdersTotal']; $result = mysql_query("SELECT OrdersTotal FROM $years WHERE Client='$selected' AND Month='December $year'") or die(mysql_error()); $row = mysql_fetch_array( $result ); $dec = $row['OrdersTotal']; ?> Highcharts Example
	<script type="text/javascript" src="http://ajax.googleapis.com/ajax/libs/jquery/1.8.2/jquery.min.js"></script>
	<script type="text/javascript">

$(function () {
var chart;
$(document).ready(function() {
chart = new Highcharts.Chart({
chart: {
renderTo: ‘container’,
type: ‘column’
},
title: {
text: ‘YTD No. of Orders’
},
subtitle: {
text: ‘’
},
xAxis: {
categories: [
‘Jan’,
‘Feb’,
‘Mar’,
‘Apr’,
‘May’,
‘Jun’,
‘Jul’,
‘Aug’,
‘Sep’,
‘Oct’,
‘Nov’,
‘Dec’
]
},
yAxis: {
min: 0,
title: {
text: ‘Rainfall (mm)’
}
},
legend: {
layout: ‘vertical’,
backgroundColor: ‘#FFFFFF’,
align: ‘left’,
verticalAlign: ‘top’,
x: 100,
y: 70,
floating: true,
shadow: true
},
tooltip: {
formatter: function() {
return ‘’+
this.x +’: ‘+ this.y +’ mm’;
}
},
plotOptions: {
column: {
pointPadding: 0.2,
borderWidth: 0
}
},
series: [{
name: ‘2011’,
data: [<?php echo $jan ?>, <?php echo $feb ?>, <?php echo $mar ?>, <?php echo $apr ?>, <?php echo $may ?>, <?php echo $jun ?>, <?php echo $jul ?>, <?php echo $aug ?>, <?php echo $sep ?>, <?php echo $oct ?>, <?php echo $nov ?>, <?php echo $dec ?>]

        }, {
            name: '2012',
            data: [42.4, 33.2, 34.5, 39.7, 52.6, 75.5, 57.4, 60.4, 47.6, 39.1, 46.8, 51.1]

        }]
    });
});

});


</body>

[/php]

Well, there’s quite a bit here that can be improved. I guess I’ll start you off with something :slight_smile:

This is possibly the most inefficient part of the script that I can see.

[php]
$result = mysql_query(“SELECT OrdersTotal FROM $years WHERE Client=’$selected’ AND Month=‘January $year’”) or die(mysql_error());
$row = mysql_fetch_array( $result );
$jan = $row[‘OrdersTotal’];

$result = mysql_query(“SELECT OrdersTotal FROM $years WHERE Client=’$selected’ AND Month=‘February $year’”) or die(mysql_error());
$row = mysql_fetch_array( $result );
$feb = $row[‘OrdersTotal’];

$result = mysql_query(“SELECT OrdersTotal FROM $years WHERE Client=’$selected’ AND Month=‘March $year’”) or die(mysql_error());
$row = mysql_fetch_array( $result );
$mar = $row[‘OrdersTotal’];

$result = mysql_query(“SELECT OrdersTotal FROM $years WHERE Client=’$selected’ AND Month=‘April $year’”) or die(mysql_error());
$row = mysql_fetch_array( $result );
$apr = $row[‘OrdersTotal’];

$result = mysql_query(“SELECT OrdersTotal FROM $years WHERE Client=’$selected’ AND Month=‘May $year’”) or die(mysql_error());
$row = mysql_fetch_array( $result );
$may = $row[‘OrdersTotal’];

$result = mysql_query(“SELECT OrdersTotal FROM $years WHERE Client=’$selected’ AND Month=‘June $year’”) or die(mysql_error());
$row = mysql_fetch_array( $result );
$jun = $row[‘OrdersTotal’];

$result = mysql_query(“SELECT OrdersTotal FROM $years WHERE Client=’$selected’ AND Month=‘July $year’”) or die(mysql_error());
$row = mysql_fetch_array( $result );
$jul = $row[‘OrdersTotal’];

$result = mysql_query(“SELECT OrdersTotal FROM $years WHERE Client=’$selected’ AND Month=‘August $year’”) or die(mysql_error());
$row = mysql_fetch_array( $result );
$aug = $row[‘OrdersTotal’];

$result = mysql_query(“SELECT OrdersTotal FROM $years WHERE Client=’$selected’ AND Month=‘September $year’”) or die(mysql_error());
$row = mysql_fetch_array( $result );
$sep = $row[‘OrdersTotal’];

$result = mysql_query(“SELECT OrdersTotal FROM $years WHERE Client=’$selected’ AND Month=‘October $year’”) or die(mysql_error());
$row = mysql_fetch_array( $result );
$oct = $row[‘OrdersTotal’];

$result = mysql_query(“SELECT OrdersTotal FROM $years WHERE Client=’$selected’ AND Month=‘November $year’”) or die(mysql_error());
$row = mysql_fetch_array( $result );
$nov = $row[‘OrdersTotal’];

$result = mysql_query(“SELECT OrdersTotal FROM $years WHERE Client=’$selected’ AND Month=‘December $year’”) or die(mysql_error());
$row = mysql_fetch_array( $result );
$dec = $row[‘OrdersTotal’];
[/php]

There is almost always a better solution than re-querying the same table over and over. For example, we can build an array of all months.

[php]
$data = array(); // stores data for all months
$result = mysql_query(“SELECT Month, OrdersTotal FROM $years WHERE Client=’$selected’”) or die(mysql_error()); // selecting Month allows us to use it as an array key
while($row = mysql_fetch_assoc($result)) {
$month = strtotime($row[‘Month’]); // for sorting we convert your string representation of month/year to a timestamp, I recommend NOT using a string in your database
$data[$month] = $row[‘OrdersTotal’]; // build our data array
}
ksort($data, SORT_NUMERIC); // sort $data numerically by key $month
[/php]

Then, you can simply implode the sorted array for your data variable:

[php]
data: [<?php echo implode(',', $data) ?>]
[/php]

So we’ve reduced 12 queries to 1 and made the code much more efficient and readable.

Sponsor our Newsletter | Privacy Policy | Terms of Service