PHP Application, attempting to restructure (Massive case/switch, ajax calls)

Question

background

A little over a year ago I was given the creative freedom to develop on the side of my primary responsibilities. I want to move into development, but am not currently in that role. I was given the resources to develop a PHP application that handled a lot of the reporting functions for our business.

My problem is that I have been building upon my (inexperienced) design over the last year and it is becoming cumbersome to maintain and I know I'm duplicating a lot of work (copy, pasting) and I'm looking for pointers and on where to go from here.

I expect criticism from this because even I am not happy with this design, but it has been very effective overall for my division.

The website is a HTML/JS based with PHP as the server side scripting. I use several libraries, but for the most part, I hand code a lot of the extra features. I didn't have the concept of a framework down when I first built this.

code review

Note: Please pardon the lack of documentation, I literally just created this specific example and it's the reason I'm posting on here. Because I want to fix this.

I have an index.php that is the router that dynamically pulls another php file, like the one below and displays in a div, based on the url.

index.php

<?php // require_once to have HTML headers. require_once('inc/header.php'); // require_once for wrapper, this is the menu bar located underneath the logo. require_once('inc/wrapper.php'); // start main div. echo "<div id='main'> "; // conditional to check if $_GET for file is valid. if (!empty($_GET['p'])) { $access = ms_escape_string($_GET['p']); } // prepares the query for access_level_control and stores it in an array. $sql = "SELECT access_level FROM test.dbo.users_access_control WHERE filename='$access'"; $result = sqlsrv_query($msdb, $sql); $row = sqlsrv_fetch_array($result); // uses the above array to pull a case/switch with access_level to arrange content for website, an an access level. if (!isset($_GET['p'])) { ?> <script type='text/javascript'> //<![CDATA[ document.title = 'test'; $(document).ready(function () { document.getElementById('subpagetitle').innerHTML = 'Main'; }); //]]> </script> Please choose from the menu on the left.</br> <?php } elseif ($_SESSION['accesslevel'] < $row['access_level']) { echo "You are not authorized to use this tool."; // if access level is insufficient. } else { $access = ms_escape_string($_GET['p']); $sql = "SELECT name FROM test.dbo.users_access_control WHERE filename='$access'"; $result = sqlsrv_query($msdb, $sql); $row = sqlsrv_fetch_array($result); ?> <script type='text/javascript'> //<![CDATA[ document.title = 'test - <?php echo $row['name'] ?>'; $(document).ready(function () { document.getElementById('subpagetitle').innerHTML = '<?php echo $row['name'] ?>'; }); //]]> </script> <?php require_once($access . ".php"); // opens the php file based on request. } // end main.div. echo "</div>"; // require_once footer. require_once("inc/footer.php"); ?> 

Here is an example of how just ONE of the reports functions. After calling index.php?p=datadump, the main div html is updated with the following php file.

datadump.php

<h3>Data Dump</h3> <form id='filterform'> <table class='data'> <tr> <th class='center'>Option</th> <th class='center'>Date Range</th> </tr> <tr> <td class='center'> <select id='option'> <?php $sql = "SELECT call_type_group_id id, call_type_group_name name FROM test.dbo.call_type_groups"; $result = sqlsrv_query($msdb, $sql); while ($row = sqlsrv_fetch_array($result)) { echo "<option value=" . $row['id'] . ">" . $row['name'] . "</option>"; } ?> </select> </td> <td> <input type='text' size='8' name='sdate' id='sdate' name='sdate' readonly='1' value=''/> - <input type='text' size='8' name='sdate' id='edate' name='edate' readonly='1' value=''/> </td> </tr> <tr> <th class='center' colspan='4'><input type='button' id='run' onclick="pullDataDump();" value='Run'/></th> </tr> </table> </form> </br> <div id="container"></div> <script type='text/javascript'> addTooltip(); // found in functions.js dateRange(); // found in functions.js enableEnter(); // found in functions.js </script> 

The onclick button calls a function found in ajax.js. ajax.js is essentially all very similiar looking functions that typically have one off things that make it hard to re-use the same function. So sadly, a lot of copy, pasting, tweaking happens in that file.

ajax.js

function pullDataDump() { $(function () { var option = $('#option').val() var sdate = $('#sdate').val() var edate = $('#edate').val() $('#container').html('<img src="inc/img/busy.gif" alt="Currently Loading" id="loading" />'); $.ajax({ url: 'inc/ajax.php?p=pullDataDump', type: 'POST', data: 'sdate=' + sdate + '&edate=' + edate + '&option=' + option, success: function (html) { $('#container').html(html); dataTable(); // found in functions.js } }); return false; }); } 

Which then calls a php file with the arguments in a POST request where the server side scripting retrieves the data. ajax.php is essentially a massive case/switch that I have been building upon for a long time.

ajax.php

case 'pullDataDump': { $sdate = $_POST['sdate']; $edate = $_POST['edate']; $option = $_POST['option']; ?> <table id="report" class="data"> <thead> <th class="center">DateTime</th> <th class="center">Calls Offered</th> <th class="center">Calls Handled</th> <th class="center">Calls Abandoned</th> <th class="center">Handle Time</th> <th class="center">Overflow Out</th> <th class="center">SL Calls Offered</th> <th class="center">SL Calls Abandoned</th> <th class="center">SL Calls Handled</th> </thead> <?php $sql = "SELECT DateTime, SUM(CallsOfferedHalf - OverflowOutHalf) CallsOffered, SUM(CallsHandledHalf) CallsHandled, SUM(TotalCallsAbandToHalf) CallsAbandonded, SUM(HandleTimeHalf) HandleTime, SUM(OverflowOutHalf) OverFlowOut, SUM(ServiceLevelCallsOfferedHalf) ServiceLevelCallsOffered, SUM(ServiceLevelAbandHalf) ServiceLevelAbandHalf, SUM(ServiceLevelCallsHalf) ServiceLevelCallsHandled FROM test.dbo.Call_Type_Half_Hour hh WHERE CallTypeID IN (SELECT call_type_id FROM ww_sl.dbo.call_type_group_members WHERE call_type_group_id = $option) AND DateTime >= '$sdate 00:00:00' AND DateTime <= '$edate 23:30:00' GROUP BY hh.DateTime ORDER BY hh.DateTime"; $result = sqlsrv_query($msdb, $sql); while ($row = sqlsrv_fetch_array($result)) { ?> <tr> <td class="center"><?php echo $row['DateTime']; ?></td> <td class="center"><?php echo $row['CallsOffered']; ?></td> <td class="center"><?php echo $row['CallsHandled']; ?></td> <td class="center"><?php echo $row['CallsAbandonded']; ?></td> <td class="center"><?php echo $row['HandleTime']; ?></td> <td class="center"><?php echo $row['OverFlowOut']; ?></td> <td class="center"><?php echo $row['ServiceLevelCallsOffered']; ?></td> <td class="center"><?php echo $row['ServiceLevelAbandHalf']; ?></td> <td class="center"><?php echo $row['ServiceLevelCallsHandled']; ?></td> </tr> <?php } echo "</table>"; } break; 

The #container div is updated with the results and I have a jquery library that allows manipulation of the displayed results.

My big concern is the increasing growing ajax.js and ajax.php, and I'm not sure where to move from here.

Answer 1

My general suggestion to you is use a CMS, like WordPress or Drupal or if you want a more low-level approach, a framework like CodeIgniter or Zend. Just so you know, a CMS is usually built on top of a framework.

One reason I suggested a framework or CMS (and not to fix your code) is because when your app grows, plain PHP just won't cut it. And reinventing the wheel isn't efficient either. As you can see, your code is a mashup of both server-side and client-side code, presentational and operational code.

Aside from good looking code, you need to consider:

• Maintainability - Is it easy to fix? Can I find the problem easily?
• Extendability - Is it easy to add more parts?
• Fault-resistant - Will it survive when a part breaks? Or does everything explode?

A framework usually splits your code into MVC. Your data (usually the SQL part), your page and your logic will be split into modules, so that if anything goes wrong, you would easily know where to find it.

So, all in all, I suggest you learn a framework or CMS and refactor your code. Learning curves differ per system, so I suggest you try one out and see if it fits your style.

Answer 2

Joseph's answer will help you in the short term, but continuing bad practices will hurt you in the long term. So, here are some things you can look at.

Your code should not have circular logic in it, so the *_once() versions of require() and include() should be unnecessary. There are occasions where it can't be avoided, but most of the time it is simple to avoid.

require 'inc/header.php';//you can use parenthesis if you want 

You really should avoid echoing HTML directly from your PHP. Your editor or IDE will have a much easier time performing proper highlighting, auto-completion, etc..., if you escape PHP, especially for simple HTML.

?> <div id='main'> <?php 

Proper HTML uses double quotes for attributes not single quotes. And neglecting them entirely, as I've seen you do later in your code, should also be avoided. HTML is very forgiving, but it will only go so far. Say for instance that one of your dynamically included attributes has a space in it and you neglect to use quotes. How is your browser supposed to know what to do with that second part of the attribute? I haven't really seen the single/double quote issue cause too much harm, but I imagine there is something out there that will throw a fit. Maybe something that requires proper syntax to iterate over the DOM. Its better to just use proper syntax and avoid the issue.

<div class="class1 class2" id="main"> 

Your variables and array pointers should be descriptive. What is "p"? I have the same issue at work with the code I'm currently working on. The previous programmer's excuse for using "arg0" was that it was purposefully ambiguous so the customer wouldn't know what the values were being used for and now neither do I because we have 15 different instances where "arg0" is being used. The problem with trying to hide information away from the client is that the information is being used in GET and is thus public. More than likely, and especially so in my case, the same information is displayed to the customer in plain text on the the screen explaining what those values were anyway. If you want to hide information from the customer, use POST or a session.

if (!empty($_GET['p'])) { 

Don't use JavaScript to perform a static change to your webpage. The document title is a field in the HTML headers that can easily be changed. You can even use PHP in it, like I see you doing later.

document.title = 'test'; //is the same as <title>test</title> 

If you have access to jQuery, you should definitely utilize it. jQuery makes using JavaScript enjoyable. jQuery is JS simplified.

$(document).ready(function () { document.getElementById('subpagetitle').innerHTML = 'Main'; }); //compared to $( document ).ready( function() { $( '#subpagetitle' ).html( 'Main' ); } ); 

Double quotes in PHP tells the PHP parser that there is something in the quoted string that needs to be parsed, thus using double quotes requires the slightest bit more processing power. That's not to say you should go through and change all of your double quotes over to single quotes, that's premature optimization. I'm just pointing this out so that I can now say that if you are using double quotes anyway, you might as well throw your PHP variables into your string to be parsed rather than escape from the string.

require_once($access . ".php"); require_once("$access.php");//the same thing echo "<option value=" . $row['id'] . ">" . $row['name'] . "</option>"; //braces allow you to use more complex variables echo "<option value=\"{$row['id']}\">{$row['name']}</option>"; //braces also let you append text to the end of a simple variable echo "{$var}iable"; 

There are a few things that might potentially be wrong with your pullDataDump() function. First, why are you calling a jQuery function inside of the JS function? This seems redundant and unnecessary. Second, your variables option, sdate, edate are missing semicolons and seem unnecessary. Using jQuery serialize() on the form will get you the same string. Finally, the url, type properties of your AJAX object can be retrieved from the form. Doing so means that should you need to change the action or method later you will only have to do so in one location.

function pullDataDump() { $('#container').html('<img src="inc/img/busy.gif" alt="Currently Loading" id="loading" />'); $.ajax({ $form = $( '#form' ); url: $form.attr( 'action' ), type: $form.attr( 'method' ), data: $form.serialize(), success: function (html) { $('#container').html(html); dataTable(); // found in functions.js } }); return false; } 

Finally, don't use return false use preventDefault().You might also consider changing the above to use jQuery's load() method.