Code Quality Check

[dosek125]

Hello all,

I am wondering if someone could comment on the quality of my code.
I am working on a project and would like to know whether what I am writing is 'good enough'.

The following code is an extract from the app:

<?php
//ajax_handler.php

//Main configuration file which includes all config files (Defines are made here)
require_once('../config/main.php');
// Auto loader to automatically load classes and other files
require_once('../auto_loader.php');

//If debug is defined as true then display errors
if(__DEBUG__) {
    error_reporting(E_ALL);
    ini_set('display_errors', 1);
}

//Try block for easier error management
try {
    //Create a new instance of the core class to access it's methods
    $core = new Core;

    //Check if controller is sent through both get and post
    if( !isset($_GET['controller']) || !isset($_POST['controller']) || empty($_POST['controller']) ) {
        throw new Exception(Core::l_static('Invalid parameters received.'));
    }

    //Check if the post controller is the same as the get controller
    if( $_GET['controller'] != $_POST['controller'] ) {
        throw new Exception(Core::l_static('Invalid parameters received.'));
    } else {
        $controller = $_POST['controller'];
    }

    //Check if file exists, if yes then assign the file path to a variable
    if( $file_name = Core::get_file_name(__DIR__ . '/controllers/' . ucwords($controller) . '.php') !== False ) {
        //Include the required file
        require_once $file_name;

        //Format class name and create an instance
        $class_name = ucwords($controller) . 'Controller';
        $controller = new $class_name;

        //Check if the default 'execute' method exists in the class
        if( method_exists($controller, 'execute') === False ) {
            throw new Exception(Core::l_static('System error. [Execute does not exists]'));
        }

        //Assign the result of method to variable
        $result = $controller->execute($_REQUEST);

        //If the method returned false then throw an exception
        if( $result === False ) {
            throw new Exception(Core::l_static('Operation failed. [Method returned false]'));
        }

        //If posted via ajax return as JSON otherwise redirect to controller page
        if( isset($_GET['ajax']) ) {
            echo json_encode($result);
        } else {
            //Somehow AJAX did not load
            header('Location:' . get_class($controller));
        }

    } else {
        throw new Exception(Core::l_static('Invalid parameters received. [Invalid Controller]'));
    }

} catch(Exception $e) {
    //Catch exceptions and return them to the user
    if( isset($_GET['ajax']) ) {
        //Return as JSON if AJAX is enabled
        echo json_encode(['errors' => $e->getMessage()]);
    } else {
        //If AJAX is not enabled then return the error encoded using base64
        header('Location:' . __HOMEPAGE__ . '?error=' . base64_encode($e->getMessage()));
    }
}

This piece of code process AJAX (and ordinary POST requests if the JS scripts fail to load for any reason) POST requests.
I have a 'semi-MVC' structure set up (Classes, Controllers and Views).

Thanks.

<P.S. I am aware of the fact that this post might be a bit vague/useless and I am sorry for this.>

[pritaeas]

It's a small piece which looks very reasonable.

I do have one question. Why do you use method_exists? Wouldn't it be cleaner to require an interface on each controller to force the implementation of execute?

I'm assuming the method_exists is there as "strength in depth" since the user is in control of GET/POST vars and could end up creating an unintended (but existing) object (...Controller)?

I'd just question the use of $_REQUEST.
This can be messed with depending on the server settings for variables.order:

http://php.net/manual/en/ini.core.php#ini.variables-order

As a rule $_GET has precedence, then $_POST, then $_COOKIE (following the classic EGPCS order). BUT this depends on your php.ini file. Not sure if it's relevant.

[dosek125]

@pritaeas - Thanks, I completely forgot about using that. I have now added an interface to the controller.

@diafol - The reason I have used $_REQUEST is because I wanted to pass on both the $_GET and $_POST data to the method. The reason why I am using a mix is because some actions such as creating new resources will use $_POST and things such as retrieving a list of resources would be done with $_GET. Would it be better to stick to only one of those and pass it along to the method? Or maybe a better way would be to create an array inside the code above which would work in this manner:

<?php
    $data = [
        'post' => $_POST,
        'get' => $_GET
    ]
?>

And then pass it on to the execute() method.

Thanks for the helpful replies.

I'm trying to build a web application which is going to be easily maintainable and that would be well designed/structured so this feedback will help me very much.

Yes, GET for retrieving, POST for creating. OK. But they are superglobal anyway, so should be available from everywhere. Not sure what's happpening with the execute method. Does it need superglobals to be passed to it?

[dosek125]

Hi. Thanks for the help. I will update the code.

[jkon]

Actually using superglobals always might not be a good idea. Lets suppose that we have a class that does something with data that normally came from $_POST , there is no reason why not supplying those data (and instead use the $_POST superglobal) , maybe in near future we want to use it in the exact same way but with data provided by another class , so it is best NOT to use the $_POST superglobal directly (or by static properties reference) . The same apply to Controllers , but this is a big discussion.

Some thought on your code . PHP is a multi - paradigm programming language but if I got it right you are trying to follow the OOP way. So would make more sense if it is a new instance to have () after it e.g. new Core() .

I think that defined variables creates a mess so for example DEBUG could be a public static property of a class or even better part of the configuration (I noticed that it is a .php file , if it suit you no problem but think the option to be a .json file for easy maintenance).

As I understood your code is some kind of a router, so it could be a class itself, with small methods – functions to perform the different things that you do. You are commenting but commenting those methods would be a lot easier for someone to read. I couldn't not noticing that you use a lot $_GET variables , even to decide witch Controller to load. It is method that I have never used and I am happy to see that now days even fewer people are using it. Instead you could map the redirect url to a Controller path. There are many ways to do it (if you want we can discus it further).

Exceptions . I can't really understand why you use Core::l_static() method . Exception messages could be as well in an Exceptions_Messages class as static properties with their names defining the kind of exception , also returning a code might be a good idea (the same Exception might happen in more than two places).

I can't see an exception handler , or even an error handler , maybe you have it somewhere else in your code. I wrote too much , concluding I think you are in the right road

Good point on the use of superglobals jkon (+1 for that). Most frameworks will create a static ref to these, e.g. Laravel uses Input::get('name') for get and post regardless or Input::all(). These entities are usually then passed to local variables (or class level properties) once validated, and it is these that are passed (or ref'd) in methods.

With regard to routing, I agree that mapping is the sanest method and could be done via 'routes' file containing a list of urls mapped to 'controller@method's. This may also allow the safe passing of url querstring values as method parameters.

e.g. Laravel once again:

Route::get('/users', 'UserController@index');
Route::post('/users/store', 'UserController@store');
Route::get('/users/{id}/edit', 'UserController@edit');

The UserController edit method in this case: public function edit($id){...}

I'm not suggesting developing anything like a full framework as you want a lightweight solution, but routing can be an absolute nightmare to maintain unless you have a reliable, robust and "transparent" way of dealing with it.

[jkon]

Diafol just think .. could the framework you mentioned pass any Coding Standards ? Most PHP frameworks are not frameworks ... they are just “code generator tools” or PHP libraries .. lying in a static world. The “framework” you mentioned is both ...

Most of PHP frameworks that aren't really OOP but are discussed as such (as the one we heard of) use static properties of a class for everything ... this isn't OOP ... this is just to make fun and pretend ... the fun thing about users of such frameworks is that they love static variables from a class that it is not their instance. So it is not OOP ... just functional programming

Static have their own place ... e,.g. to denote an error message ... or a utility class such as String methods .. but if all the framework lies in static methods it is not a framework at all ... it is just a library. In PHP a Library can discussed if you pay enough , but what is the meaning of all that. A library is still a library, it is not a framework.

I heard again the weird way of mapping url to Controllers. Why the weird way have become so popular I can't understand. The straight way is to map the Controller to the URL E.g. I have a site example.com ... so example.com/Hello maps to the Controllers Controller_Hello , I don't need to map anything my framework does that for me.

Little preview from _Underscore . So somebodey has example.com/Hello/asd I am in the Hello Controller with a _Get.Url object having asd as first variable

The thing is with url mappings that most of those “frameworks” as the one mentioned are just libraries and have no understanding how things in real world is done ... UrlMaps is a great extension but as such,,,, If I had to search the conf to find out witch Controller matched and how that would be an awful programming paradigm.

Most PHP frameworks as the one noted are just libraries, with many problems they implement OOP like functions but they are doing it the functional way . So any non OOP frameworks like the one mentioned should be avoided

[jkon]

I just show that “framework” that the “cool” hehehheheheheh (that is what ignorance nerds) kids do ... no OOP all functional ... all static variables .... they need a camp to learn OOP

Note 'could' not 'should' :)

I also think the guy is on the right track. Using OOP so far.

[lorenzoDAlipio]

$_REQUEST will also process even if the method is not defined. Such as

<form method="" action="yourprocessor.php">

Malicious request can also be sent to your site, in this manner

yourprocessor.php?name=hello&last=world&code=javascript:;alert(something);

print_r($_REQUEST);

although the javascript will not cut it on today's advance browsers, the use of $_REQUEST is highly discouraged. It was added only for the purpose of pre-development and no way it was intended for lazy form processing.

If you want to improve your application, you may want to consider browsing the packages in packagist.org and learn how to use composer. Composer is a dependency management for PHP.

here is a collection of routers from packagist.org. You can browse them and select the one that you can comfortably implement to your controller, model, and view. If you don't want to install the package via composer, you can get it from github and download as a zip file.

Under any circumstances, you DO NOT do this to create an object of the controller and its method.( Assuming the part of your codes below execute or instantiating something, please don't do it).

 $result = $controller->execute($_REQUEST);

executing something that we don't really know is pretty scary.

Most routers, works like this, regardless if it is the biggest router like the symfony2 router or a simple router like mine. **WARNING! this router contains many static and yes, I am considering going back to school maybe someone can teach me how to code in OOP. **

This how we can filter the segments from the router.

Let say, the uri request is like this

http://locahost/dir1/dir2/controller/method/par_1/par_2/

and our route definition is set like this

$routes = array(
                "controller/method/$1/$2" => "ActualController/actualMethod/$1/$2"
                );

the above route definition illustrates how router class can be use to create an alias for the actual controller. So, we are hiding the actual file name and the actual class,method. While the method parameters are taken as face values.

Once the uri is parsed, the router will return this in the background

ActualController/actualMethod/par_1/par_2

Using the router output, you can create a simple Dispatcher class similar to the implementation of Front Controller Design Patterns. It does not matter which design pattern you use, the most important is to be able to make sense of the router's output..

Let us simulate a simple dispatcher

$segments = 'ActualController/actualMethod/par_1/par_2'; 
list($controller, $method, $params) = explode("/", $segments, 3);

$controller = (isset($controller)? $controller : 'error');
$method = (isset($method)? $method : 'error');

$params = explode("/",$params); // this will give us an array.

/*
* this will create controller object and call the requested method
*/

call_user_func_array(array($controller, $method), array($params[0], $params[1]));

As we can see, our simple codes above put the segments to 3 variables. Using the segments we easily created a controller object, controller method, and method's parameters.

Let us create a simple controller that will respond to our example codes above.

class ActualController
{
    public function __construct()
    {
        // add something here. like an instance of the model and the view

     }

     public function actualMethod($par1,$par2)
     {

         echo $par1 .' '. $par2 .' <br/>';

     }




}

we should be getting an output similar to this

par_1 par_2

Make sure you know how to take out your application directories. In my uri example above, we can do it like this

$dir = parse_url('http://locahost/dir1/dir2/controller/method/par_1/par_2/');

$base_app_dir = $dir['path'];

making this

        dir1/dir2/

as the base directory of your application.. this is very important in defining the location of your site's assets. e.g. css, jquery, and template files.

Good luck to you... you can read more about design patterns by reading the book written by the Gang of Four (Erich Gamma,Richard Helm,Ralph Johnson,John Vlissides) entitled Design Patterns:Elements of Reusable Object-Oriented Software

You can also check the Symfony Components for further reading. Some of the important components in Laravel are borrowed from Symfony.