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

if (!isset($_SESSION['username'])){
// DB Credentials
$dbusername = 'xxxx';
$dbpassword = 'xxxx';

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

// 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');                   
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) {

$name[]=$result['fname'] . " " . $result['lname'];


// 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>";
  • 1
    \$\begingroup\$ You set the location to login.php but then you don't exit. So you send the data to the unauthorized user and hope the browser redirects quick enough before he sees it? I hope I am missing something not having done php in years. \$\endgroup\$ Commented May 31, 2013 at 8:04
  • \$\begingroup\$ Rob Apodaca's answer is spot-on. See also: codereview.stackexchange.com/questions/26803/… \$\endgroup\$ Commented Jun 2, 2013 at 4:07

3 Answers 3


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.

  • \$\begingroup\$ Thanks Rob! I'm actually designing this page for my own company. Is there a way to put PDOErrors.txt below the public directory? I'm not too worried about the security of this page since I'm the only one who will view/use it's functions, but will be good practice to implement these tips. I see a lot of people talking about mixing presentation with logic... does it create a security vulnerability or is it just frowned upon because it's messy? \$\endgroup\$
    – gb2016
    Commented Jun 3, 2013 at 4:51
  • \$\begingroup\$ I'm still unsure if people understand my reasoning for iterating the results this way. Say I have order ID 12345 which contains products A, B, and C. There is a line in the database with order ID 12345, product A, a line with order ID 12345 product B, and another line with order ID 12345 and product C. There are also many other variables that may differ on each line... you could even have order ID 12345, product C, broken and order ID 12345, product C, working. I could not think of another way to display the results correctly as I only want each order ID display once (order history) \$\endgroup\$
    – gb2016
    Commented Jun 3, 2013 at 5:00
  • \$\begingroup\$ Since I am using array_unique the keys in the array change to say for example, 1,5,7,9 after filtering out duplicate order IDs and I need keys 1,5,7,9 from name,business,email,orderstatus to be displayed... otherwise I end up with keys 2,3,4 (from the first order) showing on the next 3 orders instead of keys 5, 7 and 9 which will display the correct information. That is why I did foreach $orderid as $key => $value because it lines everything up properly. Now if there is an SQL query that can handle all that for me I need to learn it... as I can see how it will shorten the code a lot \$\endgroup\$
    – gb2016
    Commented Jun 3, 2013 at 5:08
  • \$\begingroup\$ @GarrettBurke yes, you can and should put log files outside of the webroot. As I metioned, the various frameworks do this for you already. If you must, try file_put_contents(__DIR__ . '/../PDOErrors.txt', /*...*/); \$\endgroup\$ Commented Jun 6, 2013 at 20:29

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.
  • \$\begingroup\$ Thank you. The reasoning behind all the notes is for my own personal use -- as I am new to coding, it just makes it so much easier to understand the logic. It's also my own web page so nobody else will see it. Looking in to the other stuff. \$\endgroup\$
    – gb2016
    Commented May 30, 2013 at 11:54
  • \$\begingroup\$ DISTINCT gives you distinct rows, no matter what the contents of your database. Of course, if you have multiple duplicate IDs in xxxx_orders.orders your DB has bigger problems. \$\endgroup\$
    – l0b0
    Commented May 30, 2013 at 15:37
  • \$\begingroup\$ I am not sure if this will work because the same order ID is replicated many times in the orders database. When a customer places an order, each order has unique product ID, but will have the same order ID for each row. If they order 10 products, 10 identical order IDs are inserted in to the orders database but they all have a different product ID. The e-mail for the specific customer is then added to each of the 10 rows so when the customer logs in there is a reference point (their e-mail) to pull up their order history, but I only want to display each order ID 1 time to the customer \$\endgroup\$
    – gb2016
    Commented May 30, 2013 at 15:39
  • \$\begingroup\$ Also I should say the order ID is not based on the default ID automatically assigned by mySQL (id column) -- it is assigned by using sha1(uniqid(mt_rand(), true)) and there is an actual column called orderid. I used sha1(uniqid(mt_rand(), true)) not for security just wanted something that would not have much chance of duplicating an order ID across 2 unrelated orders \$\endgroup\$
    – gb2016
    Commented May 30, 2013 at 15:43

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.


Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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