Avoid too many switches in controller when ajax call

PHP
1

Hi I’ve got the following url http://mywebsite/group/viewGroup/3 then with my router I can analize the url with a router function to get the controller (group), the action (viewGroup) and the parameter (group id 3)

In my controller i’ve got some switch cases where i first detect the action and then detect different ajax calls in this way:

switch ($action) {

	case 'viewGroup':

		if(isset($_POST['ajax_action']) && !empty($_POST['ajax_action'])) {

			$ajax_action = trim(filter_var($_POST['ajax_action'], FILTER_SANITIZE_STRING));

			switch($ajax_action) {

				case 'km-list-groups':

	                try{

	                    // Check if the model file exist
	                    if(file_exists(KM_MODEL_PATH.KM_DS.'km-list-groups.php')){
	             			// Load the model file
	                        require_once(KM_MODEL_PATH.KM_DS.'km-list-groups.php');
	                         exit;
	                    }else{

	                        if(IS_AJAX) {
	                            echo km_notice('Operation failed!', "Error in showing group list.", KM_ERROR_CODE, 'error');
	                           exit;
	                        }

	                    }

	                }catch (Exception $e){

	                     if(IS_AJAX) {
	                        echo km_notice('Operation failed!', $e->getMessage(), $e->getCode(), 'error');
	                        exit;
	                    }

	                }   

	            break;

			}

		}

	break;

}

Now the problem is thati’ve got several ajax call to this controller and therefore I’ve ended up with a massive number of switch.
My solution would be to create a model file for each ajax action and name this file the same the ajax action and therefore the code would be:

if(isset($_POST['ajax_action']) && !empty($_POST['ajax_action'])) {

    // Save ajax action into a variable
    $ajax_action = trim(filter_var($_POST['ajax_action'], FILTER_SANITIZE_STRING));

        try{

            // Check if the model file exist
            if (file_exists(KM_MODEL_PATH.KM_DS.$ajax_action.'.php')) {
                //Load the model to create a new ticket
                require_once(KM_MODEL_PATH.KM_DS.$ajax_action.'.php');
                exit;

            }else{

                if(IS_AJAX) {
                    echo km_notice('Operation error!', "There was an error.", KM_ERROR_CODE, 'error');
                    exit;
                }

            }

        }catch (Exception $e){

            if(IS_AJAX) {
                echo km_notice('Operation error!', $e->getMessage(), $e->getCode(), 'error');
                exit;
            }

        }   

}

I’m not using a framework or classes at the moment becuse I’m still learnng basic php. The qestion is do you think this can be a good approach? Do you have any other ideas on how to solve this problem?

Many thanks for your support

2

Seems there is room for improvement in your router. What does your router (class) look like now?

3

Hi @rpkamp thanks for your answer this is the router, I’m not using classes at the moment but only procedural PHP because I’m still learning the basics of PHP


function dispatcher($url){

    $url = trim($url, '/');
    $urlSegments = explode('/', $url);

    $scheme = ['controller', 'action', 'params'];
    $route = [];

    foreach ($urlSegments as $index => $segment){    

        if ($scheme[$index] == 'params'){

            $route['params'] = array_slice($urlSegments, $index);
            break;

        } else {

            $route[$scheme[$index]] = $segment;

        }
    }

    return $route;

}

This is my index.php file

// Get controler, action and params from url
$km_route = km_dispatcher($_SERVER['REQUEST_URI']);
// Set action variable
$km_action = (empty($km_route['action'])) ? 'default' : $km_route['action'];
// Set controller variable
$km_controller = $km_route['controller'];

if(empty($km_controller)){

	echo 'this is the home page';

}else{

	// Check if controller exist otherwise show page error 404
	if(file_exists(KM_CONTROLLER_PATH.KM_DS.$km_controller.'.php')){
		// Check if the action is alphabetic
		if(!ctype_alpha($km_action)){

	    	// Render the view for page error 500 
                echo $twig->render('/km-errors/km-error-500.twig');
	    	exit;

		}

		// Check if action is isset and is alphabetic
		if(isset($km_action)){
			// Load specific controller functions if there is a file
			if(file_exists(KM_FUNCTION_PATH.KM_DS.$km_controller.'.php')){
				require_once(KM_FUNCTION_PATH.KM_DS.$km_controller.'.php');
			}
	    	
		// Load the controller
	    	require_once(KM_CONTROLLER_PATH.KM_DS.$km_controller.'.php');
		}else{

			// Render the view for page error 500 
            echo $twig->render('/km-errors/km-error-500.twig');
	    	exit;
		}
	}else{

	    // Render the view for error 404 page not found
        echo $twig->render('/km-errors/km-error-404.twig');
	    exit;
	}
}
4

It’s rather confusing that you’re using all this terms that you normally find as class names but you use them for functions and file names. At least you’ve got the concepts right :slight_smile:

So, a few observations.

The dispatcher

First is that the dispatcher is quite naive as it stands and is really tied to the URL schema, forcing you to write all URLs a certain way. I’d recommend to make that more loose by introducing a default controller and a default action. The parsing would then be done as follows:

function dispatcher(string $url): array {
    $segments = explode('/', trim($url, '/'));

    $route = [];
    $route['controller'] = array_shift($segments) ?: 'default';
    $route['action'] = array_shift($segments) ?: 'index';
    $route['params'] = $segments;

    return $route;
}

(running example: https://repl.it/repls/WhichVioletCallbacks)

This has multiple advantages:

  1. The code is a lot simpler
  2. The home page is no longer a special case (it is the index action of the default controller now)
  3. Action is no longer required, it defaults to index, so you can have a URL like /about that goes to the index action of the about controller

Routing to controllers

Another thing I would recommend is making it possible to have separate files per action, instead of throwing all actions of a single “controller” into one file.

For that you would have to change your index.php file like so:

if (file_exists(KM_CONTROLLER_PATH.KM_DS.$km_controller.'.'.$km_action.'.php')) {
    require_once(KM_CONTROLLER_PATH.KM_DS.$km_controller.'.'.$km_action.'.php');
} else {
    require_once(KM_CONTROLLER_PATH.KM_DS.$km_controller.'.php');
}

Then you can also have a file like default.index.php for the index action of the default controller and you no longer have to stuff all actions into one big switch statement.

Cleaning up

There are a few more things in the index.php that seem less ideal. One thing I would recommend is using guard clauses, which makes code much easier to read. Another thing is that you’re rendering pages for certain HTTP codes (404, 500) but not actually sending the header, so all pages still return HTTP status code 200, and bots will think the page exists and works fine. Lastly, if the $km_action is not alnum the code renders a 500 page, but that’s not what’s going on there. 500 means the server made an error, but it didn’t. The client sent input we don’t want, so that’s a 400 error, not 500. You can read about HTTP status codes here: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status

So, taking all of that into account the index.php would be as follows:

// Check if a controller file exists
if (file_exists(KM_CONTROLLER_PATH.KM_DS.$km_controller.'.'.$km_action.'.php')) {
    $controllerFile = KM_CONTROLLER_PATH.KM_DS.$km_controller.'.'.$km_action.'.php');
} elseif (file_exists($controllerFile = KM_CONTROLLER_PATH.KM_DS.$km_controller.'.php')) {
    $controllerFile = KM_CONTROLLER_PATH.KM_DS.$km_controller.'.php';
} else {
    header('HTTP/1.1 404 Not Found');
    $twig->render('/km-errors/km-error-400.twig');
    exit;
}

// Check that the action consists of alphanum chars only
if (!alnum($km_action)) {
    header('HTTP/1.1 400 Bad Request');
    $twig->render('/km-errors/km-error-400.twig');
    exit;
}

// Load specific controller functions if there is a file
if (file_exists($controllerFunctionsFile = KM_FUNCTION_PATH.KM_DS.$km_controller.'.php')) {
    require_once($controllerFunctionsFile);
}

// Start output buffering so that we don't send anything to the client unless we know
// for sure it is all correct. If it some point an error occurs we can throw away the 
// output so for and render an error page instead.
ob_start();
try {
    // Run the controller
    require $controllerFile;
} catch (Throwable $t) {
    // Something went wrong while running the controller
    // Throw away any output so far
    ob_end_clean();

    // Render an error page
    header('HTTP/1.1 500 Internal Server Error');
    $twig->render('/km-errors/km-error-500.twig');

    exit;
}

// Everything went fine, send the output to the client
echo ob_get_clean();

Notice there is no more nesting of if checks here. The code just runs from top to bottom and if it any point it finds a failure conditions it stops then and there.

1 Like
5

Hi @rpkamp many thanks for your time and help

6

This topic was automatically closed 91 days after the last reply. New replies are no longer allowed.