Displaying orders and customers in a table, filtering out duplicates

Question

Can I make any further improvements to simplify / speed up this code? My reason for creating new arrays for the variables $name, $business, $email, $orderstatus is because when displaying results there will be identical order ID numbers stored in $orderid.

My script filters out the duplicates then with the remaining results matches up the keys from $orderid with the correct key in $name, $business, $email, $orderstatus

 <?php
session_start();
if (!isset($_SESSION['username'])){
header("location:login.php");
}
// DB Credentials
$dbusername = 'xxxx';
$dbpassword = 'xxxx';

// Connect to order database
$conn = new PDO('mysql:host=localhost;dbname=xxxx_orders', $dbusername, $dbpassword);
$conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

// Select Order IDs from database
try {
$stmt = $conn->prepare('SELECT  o.*, c.fname, c.lname, c.business FROM xxxx_orders.orders o, xxxx_customers.customers c WHERE c.email = o.email');                   
$stmt->execute();
}   
catch(PDOException $e) {  
    echo "I'm sorry, xxxx. I'm afraid I can't do that.";  
    file_put_contents('PDOErrors.txt', $e->getMessage(), FILE_APPEND);  
}

// For each result create or add to the arrays $orderid,$name,$business,$email,$orderstatus
foreach ($stmt as $result) {

$orderid[]=$result['orderid'];
$name[]=$result['fname'] . " " . $result['lname'];
$business[]=$result['business'];
$email[]=$result['email'];
$orderstatus[]=$result['orderstatus'];

}

// Filter out duplicate order IDs from $orderid array
$orderid = array_unique($orderid);

// Display results section

foreach ($orderid as $key => $value) {
        echo "<tr>";

        // Display First/Last name from array $name with $key being equal to the corresponding key from array $orderid.
        // The key is necessary to properly display the correct result
        echo "<td align=center>" . $name[$key] . "</td>";

        // Display business name from array $name with $key being equal to the corresponding key from array $orderid.
        // The key is necessary to properly display the correct result
        echo "<td align=center>" . $business[$key] . "</td>";

        // Display email from array $name with $key being equal to the corresponding key from array $orderid.
        // The key is necessary to properly display the correct result                        
        echo "<td align=center>" . $email[$key] . "</td>";

        // Display order status from array $name with $key being equal to the corresponding key from array $orderid.
        // The key is necessary to properly display the correct result        
        echo "<td align=center>" . $orderstatus[$key] . "</td>";

        // Display First/Last name from array $name with $key being equal to the corresponding key from array $orderid.
        // The key is necessary to properly display the correct result        
        echo "<td align=center><a href=editorder.php?q=" . $orderid[$key] . ">" . $orderid[$key] . "</a></td>";

        echo "</tr>";
}
?>  

Answer 1

There are several major problems.

• You've mixed presentation with logic.
• You have some error/exception handling but not enough.
• Where is your PDOErrors.txt being written to? It looks like it's being written to your web directory. This allows someone to visit http://yoursite.com/PDOErrors.txt and view information that may allow them to compromise your site.
• You read from variables without ensuring they've been initialized. Even though php does not require variables to be initialized, I've found that this is a great place for bugs to live.
• The way you are iterating over query results and creating arrays makes no sense. I know you've mentioned there are duplicates to remove but your solution is not the way to handle that situation. As others have mentioned, you either have problems with your schema that should be corrected, or you should use sql to return the proper result set.
• You should not display unlimited result sets. The app might seem like it works now but, when your data grows the app will eventually crash due to either time or memory exhaustion. Result sets should be paginated with sane limits.
• Your code is vulnerable to cross site scripting (or at least broken html). Whenever you do echo $foo, you should instead do echo htmlspecialchars($foo).

I know you've stated you are new to programming so I understand and sympathize with wanting to write code this way. You get quick results and from your perspective and I'm sure it seems like it works. But, as your app grows, you will find that this style is completely un-maintainable. Even you, the author will not be able to follow or understand it (not to mention the poor sap who comes after you).

Additionally, you have introduced some major security vulnerabilities without even realizing it. My advice is that you should immediately abandon this script and look to one of the many excellent frameworks for php (symfony or zend to name a few). These kinds of frameworks already contain what you need to write secure and organized applications. Although it would take time to learn how to use a framework, it is far less time than you would spend maintaining scripts written this way and dealing with the security problems.

Answer 2

First, since you're only going to print one HTML table row per order, make sure your query only returns one database row per order. That way there's no need for extra complexity and throwing away bandwidth and memory. This should be doable by adding a simple DISTINCT after the SELECT keyword.

Some other things:

• There is no way production code requires that much commenting.
• You can use a foreach (array($name, $business, $email, $orderstatus) as $array) to reduce the duplication in the loop.
• DB credentials, host and name should be in a configuration file.

Answer 3

Use explicit joins with the columns to join on in an ON clause. In this case it will make little difference but when queries are more complex it makes things easier to understand. Better to get into the habit now.

Don't use SELECT * in real code. It is a useful shortcut when testing but can cause a potential problem in future.

Best to use an integer unique key of the customer on the orders table rather than the email address. This should be faster, take less space and will not cause problems should 2 customers have the same email address, or should a customer change their email address.

Orderid looks like the primary key of the orders table and as I would suspect that an order can only apply to one customer you should not have any duplicates to worry about removing.

If Orderid is unique then you can just do all the processing of the rows in a single foreach, with no need to store copies of the data in an array.