2
\$\begingroup\$

I have a custom framework that I am not sure if certain things I have applied are wrong and if so, just why.

I am not well versed in any framework, so I was forced to use mine.

Here is an example of a controller found in /app/controllers/UserProfile.php

<?php

    class UserProfile
    {
       public function showUserProfile($import)
       {
         // get view object
         $view =  $import["load"]::view();
         // get orm object
         $db   =  $import["load"]::orm();
         // get form builder object
         $form =  $import["load"]::formBuilder();
         //  extract username & id from current url. i.e. dev.foo.com/john/11
         list($userName, $id) = $import["args"];
         // get view object, for loading templates
         $view::header();   // ~ require  .. view/header.php
         // DATABASE QUERY.
         $users = $db->table("users");    
         // SELECT username, age, country from users where username = $username AND id = $id;
         $select = $users->read(["username, age, country", [$username, $id]]);
         $view::body(["select"=>$select]);
       }
    }
?>

And if you are wandering how the DI is being handled, here is how:

<?php 

class ServiceProvider
{

    function service($getService)
    {

        switch ($getService) {
            case 'orm':
                return new \foo\bar\orm(new \pdo('....'))
                break;

            case 'view':
                $views_dir = __DIR__.'../path/to/views/dir';
                return new \foo\bar\viewCreatror($views_dir);
                break;

            case 'formBuilder':
                return new \foo\bar\formBuilder('..');
                break;

            default:
                # code...
                break;
        }


    }
}
?>

Now, since I also need to get the URL fragments like :blog-title/:id. I allow my router to handle that like this.

<?php 
namespace foo\bar; 
class Router 
{
    function add(){}
    function get()
    {
        $service = new \foo\bar\ServiceProvider;
        $url     = $this->getExplodedUrl; 
        // IF URL matches, and controller + method exists. 
        return (new $controller)->$method(["load"=>$service, "args"=>$url]);
    }
}

The thing is, that everything works well, but to improve it, I need to know which parts need the most urgent attentions.

\$\endgroup\$
7
  • 1
    \$\begingroup\$ Try watching: youtube.com/watch?v=-FRm3VPhseI \$\endgroup\$
    – tereško
    Sep 25, 2014 at 18:56
  • 3
    \$\begingroup\$ Please consider reading about service location, and the difference between it and dependency injection. In short you've lost the whole benefit of classes having explicit dependencies. \$\endgroup\$ Sep 25, 2014 at 18:56
  • \$\begingroup\$ @tereško thanks. >>BenjaminGruenbaum: That is part of my question, It works well, it does what DI is supposed to do. I agree there is a tight coupling inside the router class, but for the most part, the service provider provides the components as & when requested. \$\endgroup\$ Sep 25, 2014 at 19:05
  • \$\begingroup\$ Hmm ... damn, I actually wanted to link you to this lecture. The problem with service locator is that it lies about the requirements of the class. If you want to reuse some module, which heavily relies on a service locator, it becomes extremely complicated, because said service locator in turn depends on two 3rds of your application code. \$\endgroup\$
    – tereško
    Sep 25, 2014 at 20:28
  • 1
    \$\begingroup\$ You state that your code does what DI is supposed to do. True. But that doesn't mean it's good code. Your ServiceProvider is all knowing. It literally knows everything. Adding classes means divinginto your ServiceProvider. And what about singletons? You create a new ORM and a new PDO everytime you ask for the orm, ... \$\endgroup\$
    – Pinoniq
    Sep 26, 2014 at 9:15

1 Answer 1

1
\$\begingroup\$

General impression

You are not actually injecting any of the dependencies into UserProfile, you are injecting a service provider that has to locate the dependencies.

It may feel like dependency injection but it really isn't.

What you are doing here is, basically, creating spaghetti code.

Suggested improvements

My question would be: What is the UserProfile responsible for?

Your UserProfile class depends on a View, ORM and FormBuilder class. It is in control of retrieving the user from the DB, outputting a header and outputting the user in the view body. The FormBuilder does not even seem to be used.

There are several ways this could be cleaned up, for now I'll use "classic" MVC.

Assuming (based on the function name) that this class is primarily a View, it should only need the user Data and your ViewCreator to function. Your router would call the Controller that would retrieve the Model and pass it on to your View.

The suggestion below completely removes the service locator from the picture. Please note that the code is written for clarity in conveying the message, it is not meant to be 100% working or correct (especially the static call syntax in the controller).

namespace \foo\bar;

class Router 
{        
    function get()
    {
        $url = $this->getExplodedUrl();

        // get view object
        $viewsDir = __DIR__ . '/../path/to/views/dir'; //@TODO: Read this from a config file
        $view =  new ViewCreator($viewsDir)

        // get orm object
        $db   =  new Orm(new \Pdo('...'))

        // get form builder object
        $formBuilder = new FormBuilder('...');

        // IF URL matches, and controller + method exists. 
        return (new $controller)->$method($url, $db, $view, $formBuilder);
    }
}

class UserController
{
    public function showUserProfile($args, $db, $view)
    {
        // extract username & id from current url. i.e. dev.foo.com/john/11
        list($userName, $id) = $args;
        $select = UserRecord::fetchUserFromDb($db, [$userName, $id]);
        UserProfile::showUserProfile($view, ["select"=>$select]);
    }
}

class UserProfile
{
    /**
     * Display a given User with a given View
     */
    public function showUserProfile($view, $viewData)
   {  
     // get view object, for loading templates
     $view::header();   // ~ require  .. view/header.php
     $view::body($viewData);
   }
}

class User
{
    /**
     * Get a given User from a given Database
     */
    public function fetchUserFromDb($db, $user)
    {
         // DATABASE QUERY.
         $users = $db->table("users");    

         // SELECT username, age, country from users where username = $username AND id = $id;
         $select = $users->read(["username, age, country", $user]);

         return $select;
    }
}

Things could be cleaned up even further by not initiating classes from the router but injecting them into the router from your delivery mechanism/root/index.php/etc.

Further Suggestions

You say you're not well versed in any framework but what you seem to be doing here is very similar to the way Silex works, which uses Pimple (which was mentioned in the comments to your question). The learning curve is quite flat so it could be worth the effort of seeing how Silex does things.

If studying an entire framework takes too much of an effort I would strongly suggest you take a look at PHP-DI to observe the way a Dependency Injector should work.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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