Problem
So I wrote this class to handle my ajax calls simply:
ajaxCon.php:
class ajaxCon{
private $init = false;
private $functionArray = array();
private $Message = false;
private $function = null;
private $arguments = null;
private $post = null;
public function __construct(){
if(!isset($_POST['ajaxRequest'], $_POST['ajaxFunction'])) return;
$this->init = true;
ob_start();
$this->post = $this->sanitizeArr($_POST);
$this->arguments = $this->sanitizeArr($_POST['arguments']);
$this->function = $_POST['ajaxFunction'];
}
public function addFunction($functionName, $functionCode){
if(!$this->init || !isset($functionName, $functionCode) || $functionName != $this->function) return $this;
$this->Message = call_user_func_array($functionCode, $this->arguments) ? 'success' : 'fail' ;
unset($this->init, $this->functionArray, $this->post);
$returnVars = get_object_vars($this);
ob_clean();
echo json_encode($returnVars);
ob_flush();
exit();
}
private function sanitizeArr($returnArr, $sanitizer = 'sanitize_sql_string') {
array_walk_recursive($returnArr, function (&$value) use ($sanitizer) {
$value = $sanitizer($value);
});
return $returnArr;
}
}
ajaxCon.js:
function ajax() {
this.callback = false;
this.ajaxFailure = function (returnData) {};
this.ajaxSuccess = function (returnData) {};
this.call = function(functionName, argumentArray, callback){
$.ajax( $.extend(true, {}, this.ajaxSettings, {data:{'ajaxFunction':functionName, 'arguments':argumentArray}} ) );
this.callback = callback;
};
this.ajaxReturn = function (returnData){
returnData = returnData.substring(returnData.indexOf("{") - 1);
returnData = returnData.substring(0, returnData.lastIndexOf("}") + 1);
var responseArray = $.parseJSON(returnData);
if (responseArray.Message.toLowerCase() != "success"){
this.ajaxFailure(responseArray);
return;
}
this.ajaxSuccess(responseArray);
if(this.callback != false) this.callback(responseArray);
};
window.ajaxCon = this;
this.ajaxSettings = { url: window.location.href, type: 'POST', cache: false, data: {ajaxRequest: true}, success: function (returnData) {
ajaxCon.ajaxReturn(returnData)
}};
}
and you call it like so:
<?php
$ajaxCon = new ajaxCon;
$ajaxCon->addFunction('checkEmail', function($Email){
global $ajaxCon;
if(emailIsInDB($Email)) $ajaxCon->hasAccount = true;
return true;
});
?>
<script type="text/javascript">
var ajaxCon = new ajaxCon();
function checkEmail() {
ajaxCon.call('checkEmail', {'Email':$('#Email').val()}, function(result){
if(!result.hasAccount) $('#SignUp').submit();
});
}
</script>
The drawbacks I know of are lot’s of preprocessing (The entire page has to be preprocesed) and the page then executes as normal until the php function I need is defined and called.
Any ideas on how I can optimize the code and make it run more efficiently while preserving/improving readability?
Solution
A review of the JavaScript part:
ajaxCon
is a class/constructor, so you should call itAjaxCon
, though really I would think about finding an even better name 🙂ajaxFailure
-> You have a number of functions that haveajax
in their name, since these functions are in theAjax
constructor you can drop theajax
partthis.callback = false;
-> Don’t declare callback at all and then useif(this.callback) this.callback(responseArray);
-
This could use some newlines:
$.ajax( $.extend(true, {}, this.ajaxSettings, {data:{'ajaxFunction':functionName, 'arguments':argumentArray}} ) );
and also
this.ajaxSettings = { url: window.location.href, type: 'POST', cache: false, data: {ajaxRequest: true}, success: function (returnData) { ajaxCon.ajaxReturn(returnData) }};
- Also the location of
this.ajaxSettings
is odd, this belongs on top of that function
Review for the PHP part:
- What problem does your class solve? Are you not doing too many things in one class? If your goal is to handle incoming Ajax requests, then a different object should handle the response.
- Class names should be in CamelCaps, i.e.
AjaxCon
. -
If you need something in your constructor (data from POST, etc), just ask for it in the constructor:
public function __construct(array $post) {
-
Don’t try to sanitize the entire
$_POST
array at once, it never works. What are you sanitizing it for? Data that goes into the database should go through prepared statements, data that gets outputted back to the page as HTML should be run throughhtmlspecialchars()
before they are outputted to the page. Don’t sanitize data until the moment you need it, otherwise you risk double sanitation problems (Tom & Jerry
becomesTom & Jerry
which becomesTom &amp; Jerry
, oops). - Don’t output in your function. Always
return
. Output should be on a single place only, and that’s at the very end of the run, where you need to return the response after all processing is done. - Don’t use
get_object_vars($this)
for the purpose you are using it. Specify exactly what variables you are returning, it’s more readable. -
Don’t use globals – If you need to pass a variable from the outside to an anonymous function, use
use
:function($email) use ($ajaxCon) {
-
DO NOT USE GLOBALS – Seriously, don’t.