1
\$\begingroup\$

Here is my simple ticketing system:

 <?php
        session_start();
        session_id();
        ob_start();
        require("../configuration/config.php");
        $GetTickets = $con->query("SELECT * FROM tickets WHERE open='true'");
        if(!$_SESSION['Admin']) {
        header('Location: login.php'); exit();
        }
        ?>
        <!DOCTYPE HTML>
        <html>
        <head>
            <title> ticketExpress | Admin </title>
            <link rel='stylesheet' href='../assets/css/style.css'> 
        </head>
        <body>
        <div id='containerAdmin'>
        <h1> <img class='logo' src='../assets/images/logo.png' width='200' height='43'> </h1> <a href='?logout' class='logout'> Logout </a>
        <h3> Open Tickets </h3>
        <hr />
        <?php
        while($TicketInfo = $GetTickets->fetch_object()) {
        $Subject = $TicketInfo->Subject;
        if(strlen($Subject)>50) {
            $Subject = substr($Subject,0,50)."...";
        } else {
        $Subject = $TicketInfo->Subject;
        }
        echo "<div id='ticket'>".$Subject ."<a href='?delete=$TicketInfo->ID'><img style='float:right'src='../assets/images/delete.png' width='15px' height='15px'></a><a style='float:right; color:red; text-decoration:none; margin-right:10px;' href='?close=$TicketInfo->ID'> Close </a><span style='float:right; margin-right:10px;' id='responseMsg'> </span></div>";
        }
        if(isset($_GET['delete'])) {
        $ID = $_GET['delete'];
        echo "
        <script type='text/javascript'>
            var ajax = new XMLHttpRequest();
            ajax.open('POST','delete.php', true);
            ajax.setRequestHeader('Content-type','application/x-www-form-urlencoded');
            ajax.onreadystatechange = function () {
                if(ajax.readyState == 4 && ajax.status == 200) {
                    document.getElementById('responseMsg').innerHTML = ajax.responseText;
                }
                }
                ajax.send('delete=$ID');
            </script>
            ";
        }
        if(isset($_GET['logout'])) {
        session_destroy();
        header('Location: login.php');
        }
        if(isset($_GET['close'])) {
        $ID = $_GET['close'];
        echo "
        <script type='text/javascript'>
            var ajax = new XMLHttpRequest();
            ajax.open('POST','close.php', true);
            ajax.setRequestHeader('Content-type','application/x-www-form-urlencoded');
            ajax.onreadystatechange = function () {
                if(ajax.readyState == 4 && ajax.status == 200) {
                    document.getElementById('responseMsg').innerHTML = ajax.responseText;
                }
                }
                ajax.send('close=$ID');
            </script>
            ";
        }
        ?>
        <br />
        </div>
        </body>
        </html>

Could you give me tips on improving the performance of my script? I've noticed that it takes some time to receive a response, so performance can definitely be further improved. I've been doing PHP for the last 8 months or so and am still a newbie.

\$\endgroup\$
2
\$\begingroup\$

A few quick tips:

  • premature optimization is the root of all evil
  • use a profiling tool
  • premature optimization is the root of all evil
  • Don't echo markup, close php, write markup, open php tag again
  • premature optimization is the root of all evil
  • Dont query the DB, if you might have to change header('Location:...') a few lines further down. If you're going to have to redirect, do so ASAP.
  • premature optimization is the root of all evil
  • Avoid globals, they're error prone, hellish to debug and generally evil, but not as evil as premature optimization
  • premature optimization is the root of all evil

All in all, there's not enough code yet to squeeze any meaningful performance gain out of it. Not by altering the code alone, anyway.
If you do decide to use a profiling tool, also look into caching (google APC), and try your hand at bytecode caching. See how many ms you can shave off that way, but always remind yourself of one thing:

  • premature optimization is the root of all evil
\$\endgroup\$
1
\$\begingroup\$

Profiling is not an art, it's a science.

You need tools to profile your application, and know what eats up performance. It's only this way you'll be able to improve the performance.

For PHP, what's usually used is the xdebug profiler.

w.r.t your code, it's quite small. The only thing that can be slow is the SQL request.

\$\endgroup\$
5
  • \$\begingroup\$ Hey, thanks for the response! What should I consider to improve the sql request performance? Also, what do is your opinion on New Relic? \$\endgroup\$ – Exwolf Aug 14 '13 at 10:00
  • \$\begingroup\$ @Exwolf New Relic is another system for this, and is nice for long-running performance checking. It's also an external service, so it might not always suit your needs. When I need a one-shoot profiling session, I just go with xdebug. \$\endgroup\$ – Florian Margaine Aug 14 '13 at 10:02
  • \$\begingroup\$ @Exwolf regarding the SQL performance, first check out this is the real issue. Then, go on db.stackexchange.com if you want this kind of help :) \$\endgroup\$ – Florian Margaine Aug 14 '13 at 10:03
  • \$\begingroup\$ while(true); is pretty small too but it's working really slowly for me (smallness of code has nothing to do with perf is the point) \$\endgroup\$ – Esailija Aug 14 '13 at 12:52
  • \$\begingroup\$ @Esailija just saying that the code is pretty small to not involve a huge number of lines to bootstrap a framework or whatever. \$\endgroup\$ – Florian Margaine Aug 14 '13 at 12:57
0
\$\begingroup\$

Let me point out some issues I see in your code

  • Separate the layout from the logic and get rid of the output puffer.
  • Calling session_id(); has no effect,
  • Storing a admin flag in the session is only slightly better than adding a GET parameter admin=1. In other words this is no secure solution.
  • Florian is right, run a debugger/profiler to find your performance issues. Check microtime() as a lightweight profiler.

Update

I splitted your code into two files. And removed the ajax call, as it is useless do create client code on the server to issue a new request to do something on the server. Just do it right away (Is not the best approach in general. Google for GET after POST or how to update the page after a AJAX call without reloading the whole page.)

<?php
session_start();
if(!$_SESSION['Admin']) {
    header('Location: login.php');
    exit();
}

if(isset($_GET['logout'])) {
    session_destroy();
    header('Location: login.php');
    exit();
}

require("../configuration/config.php");
$GetTickets = $con->query("SELECT * FROM tickets WHERE open='true'");

$tickets=array();
$messages=array();
while($TicketInfo = $GetTickets->fetch_object()) {
    $Subject = $TicketInfo->Subject;
    if(strlen($Subject)>50) {
        $Subject = substr($Subject,0,50)."...";
    } else {
    $Subject = $TicketInfo->Subject;
    $tickets[$TicketInfo->Id]=$Subject
    $messages[$TicketInfo->Id]='';
}

if(isset($_GET['delete'])) {
    $ID = $_GET['delete'];
    //TODO delete
    $messages[$ID]='Deleted';
}

if(isset($_GET['close'])) {
    $ID = $_GET['close'];
    //TODO close
    $messages[$ID]='Closed';
}

include "template.php"

template.php

<!DOCTYPE HTML>
<html>
<head>
    <title> ticketExpress | Admin </title>
    <link rel='stylesheet' href='../assets/css/style.css'> 
</head>
<body>
<div id='containerAdmin'>
<h1> <img class='logo' src='../assets/images/logo.png' width='200' height='43'> </h1> <a href='?logout' class='logout'> Logout </a>
<h3> Open Tickets </h3>
<hr />
<?php foreach ($tickets as $id=>$subject):?>
    <div id='ticket'>
        <?=$subject?>
        <a href='?delete=<?=$id?>'><img style='float:right'src='../assets/images/delete.png' width='15px' height='15px'></a>
        <a style='float:right; color:red; text-decoration:none; margin-right:10px;' href='?close=<?=$id?>'> Close </a>
        <span style='float:right; margin-right:10px;' id='responseMsg'><?=$messages[$id]?></span>
    </div>
<?php endforeach?>
<br />
</div>
</body>
</html>
\$\endgroup\$
5
  • \$\begingroup\$ Hey, what would a more secure alternative to storing the admin flag be? Thanks. \$\endgroup\$ – Exwolf Aug 14 '13 at 10:05
  • \$\begingroup\$ Right now I only have a nice german tutorial on this topic, but I guess Google will help you in translating or find a similar English one. Actually the login part is not that critical right now. As a first step you should focus on the layout/logic separation. This will also help you profiling your code. \$\endgroup\$ – mheinzerling Aug 14 '13 at 10:50
  • \$\begingroup\$ Should I use a templating engine such as Smarty? Or just use str_replace and make my own simple templating engine? \$\endgroup\$ – Exwolf Aug 14 '13 at 11:04
  • 1
    \$\begingroup\$ Don't make a templating engine at all... Php is a template engine as is. Simply create all your variables and then include the correct file with html + php echo statements \$\endgroup\$ – Pinoniq Aug 14 '13 at 11:37
  • \$\begingroup\$ @Exwolf I updated my post showing how to split your code using a separate template file. \$\endgroup\$ – mheinzerling Aug 14 '13 at 12:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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