6
\$\begingroup\$

I'm new to Slim and the three layered approach I'm using below. So far I have the API, a Presentation layer (leaving out for now), a Business Logic Layer, and a Data Access Layer. The code's working, but I know it can be improved a lot. I'd really appreciate some feedback from you all.

A few things I'm questioning already:

  1. Do I really need to create a new BLL object for each method in the API?
  2. In the DAL I'm sure I shouldn't be connecting to the database in each method. What would be a better approach?
  3. With the Slim routes, it seems like /calendars/:id is getting redundant. Instead of always passing the ID up to the BLL then to the DAL, perhaps a single session variable in the DAL would be cleaner?

Of course the code below has been trimmed to keep it short, but hopefully you get the idea.

Slim API

<?php
require $_SERVER["DOCUMENT_ROOT"] . '/api/BLL/BLL.php';
require 'Slim/Slim.php';

\Slim\Slim::registerAutoloader();

$app = new \Slim\Slim(array(
    'debug' => true
));

$app->log->setEnabled(true);

$app->group('/v1', function () use ($app) {
    $app->get('/calendars/:id/events', 'getEvents');
    $app->get('/calendars/:id/event/:eid', 'getEvent');
    $app->get('/calendars/:id/users(/:type)', 'getUsers');
    $app->get('/calendars/:id/categories', 'getCategories');
    $app->get('/calendars/:id/locations', 'getLocations');
    $app->get('/calendars/:id/holidays', 'getHolidays');
    $app->post('/calendars/:id/categories', 'addCategory');
});

$app->run();

function getHolidays ($id) {
    $bll = new BusinessLayer();
    $result = json_encode($bll->getHolidaysBLL($id));
    echo '{"holidays": ' . $result  . '}';  
}
function getUsers ($id, $type = '') {
    $bll = new BusinessLayer();
    $result = json_encode($bll->getUsersBLL($id, $type));
    echo '{"user": ' . $result  . '}';
}

function getEvents ($id) {
    $bll = new BusinessLayer();
    $result = json_encode($bll->getEventsBLL($id));
    echo $result;
}

function getEvent ($id, $eid) {
    $bll = new BusinessLayer();
    $result = json_encode($bll->getEventBLL($id, $eid));
    echo '{"event": '$result . "}";
}

function addCategory ($id) {
    $request = \Slim\Slim::getInstance()->request();
    $category = json_decode($request->getBody());       
    $bll = new BusinessLayer();
    $result = json_encode($bll->addCategoryBLL($category->cat_name, $category->cat_color, $id));
    echo $result;
}

Business Logic Layer

<?php

require_once $_SERVER["DOCUMENT_ROOT"] . "/api/DAL/DAL.php";

class BusinessLayer 
{
    var $dal;

    function __construct()  {
        $this->dal = new DataLayer();
    }

    public function getEventsBLL ($id) 
    {
        //echo "From BLL getEventsBLL: " . $id . " " . $type; exit;
        $ret = $this->dal->getEventsDAL($id);
        return $ret;
    }

    public function getEventBLL ($id, $eid) 
    {
        //echo "From BLL getEventBLL: " . $id ; exit;
        $ret = $this->dal->getEventDAL($id, $eid);
        return $ret;
    }   

    public function getHolidaysBLL ($id) {
        //echo "From getHolidaysBLL: " . $id; exit;
        $ret = $this->dal->getHolidaysDAL($id, $type);
        return $ret;        
    }

    public function getUsersBLL ($id, $type) 
    {
        //echo "From getUsersBLL: " . $id . " " . $type; exit;
        $ret = $this->dal->getUsersDAL($id, $type);
        return $ret;
    }

    public function getCategoriesBLL ($id) 
    {
        //echo "From BLL getCategoriesBLL: " . $id . " " . $type; exit;
        $ret = $this->dal->getCategoriesDAL($id);
        return $ret;
    }

Data Access Layer

<?php

class DataLayer 
{

    public function connect() 
    {
        $dbhost = "localhost";
        $dbuser = "root";
        $dbpass = "";
        $dbname = "database_here";
        $db = new PDO("mysql:host=$dbhost;dbname=$dbname", $dbuser, $dbpass);  
        $db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

        return $db;
    }

    public function getUsersDAL ($id, $type)
    {
        $sql = "SELECT * FROM users 
                WHERE calendar_id = :id";

                if ($type == "admins") {
                    $sql .= " AND isadmin = :isadmin";
                }
                if ($type == "managers") {
                    $sql .= " AND allowcats = :allowcats";
                }

                $sql .= " ORDER BY id";

        try {

            if (!empty($type)) {
                $value = 1;
            }

            $db = $this->connect();
            $stmt = $db->prepare($sql);  
            $stmt->bindParam("id", $id);

            // need to loop through these instead of hardcoding
            if ($type == "admins") {
                $stmt->bindParam("isadmin", $value);   
            }
            if ($type == "managers") {
                $stmt->bindParam("allowcats", $value);   
            }

            $stmt->execute();
            $ret = $stmt->fetchAll(PDO::FETCH_ASSOC);
            $db = null;

            return $ret;

        } catch(PDOException $e) {
            echo '{"error":{"text":'. $e->getMessage() .'}}'; 
        }
    }

    public function getCategoriesDAL ($id) 
    {

        $sql = "SELECT * FROM categories 
                WHERE calendar_id = :id 
                ORDER BY cat_name ASC";

        $db   = $this->connect();
        $stmt = $db->prepare($sql);
        $stmt->bindParam("id", $id);
        $stmt->execute();
        $ret  = $stmt->fetchAll(PDO::FETCH_ASSOC);
        $db   = null;

        return $ret;        
    }

public function addCategoryDAL ($name, $color, $id) 
{

    $sql = "INSERT INTO categories (
                cat_name, 
                catcolor,
                calendar_id
            ) 
            VALUES (:cat_name, :catcolor, :calendar_id)";

            //return $sql;
    try {
         $db = $this->connect();
         $stmt = $db->prepare($sql);
         $stmt->bindParam("cat_name", $name);
         $stmt->bindParam("catcolor", $color);
         $stmt->bindParam("calendar_id", $id);
         $stmt->execute();
         //$user->id = $db->lastInsertId();
         $db = null;
         //echo json_encode($user); 


    } catch (PDOException $e) {
     error_log($e->getMessage(), 3, 'C:\windows\temp\php.log');
     echo '{"error":{"text":'. $e->getMessage() .'}}'; 
    }                
}
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! I hope you get some good answers from our PHP pros! \$\endgroup\$ – Phrancis Jun 17 '15 at 20:37
6
\$\begingroup\$

I've seen this layering before, in fact I'm working on a .NET project with this exact layering (DataAccessLayer and BusinessLayer). The BusinessLayer object does absolutely nothing. I've seen this pattern in use many times and it introduces an additional class that adds no value. I would just use the DataAccessLayer class.

Besides, your BusinessLayer class doesn't actually contain any business logic.

The DataAccessLayer class looks mostly fine, except I would drop the "DAL" suffix on all the method names. The class name takes care of telling you its the "Data Access Layer".

The exception handling needs some reworking. In fact, if a PDOException gets thrown, I would just let it go. Catch it in your Slim controller methods and echo the error JSON. Don't do that in your DAL. And please please please please please don't swallow errors. Log them some place --- but not in the Data Access Layer.

function addCategory ($id) {
    $logger = new Logger(); // <- You will have to implement this

    try {
        $request = \Slim\Slim::getInstance()->request();
        $category = json_decode($request->getBody());       
        $bll = new BusinessLayer();
        $result = json_encode($bll->addCategoryBLL($category->cat_name, $category->cat_color, $id));
        echo $result;
    }
    catch (PDOException $ex) {
        $logger->error($ex);
        echo '{"error": "' . $ex->getMessage() . '"}';
    }
}

class Logger {
    public function error(Exception $ex) {
        // log error message and stack trace
    }
}

Separation of Concerns

@mikehomme commented:

(The business layer will) handle a lot of things such as validation, authentication, usage limits, etc.

Just those three things should be three additional layers in your application. In fact, authentication and rate limiting (usage limits) are great candidates for Slim PHP middleware. Your controller functions shouldn't even get invoked in the first place if the user is not authenticated, plus you would be repeating all of the authentication logic in each controller function. Don't Repeat Yourself (DRY) can be adhered to if you insert authentication and rate limiting in the middleware tier of your application.

Now your routes become:

$app->post('/calendars/:id/categories', 'authenticateRequest', 'enforceUsageLimit', 'addCategory');

The middleware functions:

function authenticateUser() {
    // Get auth token from query string
    // Query DB for user

    if (!$valid) {
        $app->redirect(...);
    }
}

function enforceUsageLimit() {
    // Find user in DB
    // Increment request count

    if ($numberOfRequests > $limit) {
        $app->redirect(...);
    }
}

Validation

Validation deserves a little more explanation. First, I would recommend against using plain old Arrays to hold you data. There are good O/RM's out there for PHP so you can deal with concrete classes. Your Data Access Layer should be returning Domain Models. A Domain Model is a class that represents a row in one table of the database. It gives you the ability to bundle data with behavior in a modular fashion. The validator class should handle validating the whole domain model object and keeping track of the error messages.

Putting your validation rules in it's own class or classes allows you to use the validation rules in multiple contexts (or multiple controller functions). It becomes an extra if statement, but is necessary in every MVC style framework I've used (Ruby on Rails, Codeigniter, ASP.NET MVC).

First, the domain model: Category

Category Domain Model

class Category {
    private $name;
    private $color;
    private $id;

    public function Category($data = array()) {
        foreach ($data as $key => $value) {
            $this->$key = $value;
        }
    }

    public function getName() {
        return $this->name;
    }

    public function setName($value) {
        $this->name = $value;
    }

    public function getColor() {
        return $this->color;
    }

    public function setColor($value) {
        $this->color = $value;
    }

    public function getId() {
        return $this->id;
    }

    public function setId($value) {
        $this->id = $value;
    }
}

This class represents a row in the "categories" table in the database. It takes a hash of key value pairs to populate its properties in the constructor. It has little more than getters and setters right now, but if you need to add functionality that concerns categories, this is the class to add it (related reading: Domain Driven Design)

Now the validation classes:

class BaseValidator {
    private $errors;

    public function __construct() {
        $this->errors = array();
    }

    public function addError($key, $message) {
        if (empty($this->errors[$key])) {
            $this->errors[$key] = array();
        }

        $this->errors[$key][] = ucfirst($key) . ' ' . $message;
    }

    public function getErrors() {
        return $this->errors;
    }

    public function hasErrors() {
        return !empty($this->errors);
    }
}

This provides the foundation for the other validator classes.

class CategoryValidator extends BaseValidator {
    public function __construct() {
        parent::__construct();
    }

    public function isValid(Category $model) {
        if (empty($model->getName())) {
            $this->addError('name', 'is required')
        }

        if (empty($model->getColor())) {
            $this->addError('color', 'is required');
        }

        if (empty($model->getId())) {
            $this->addError('Id', 'is required');
        }

        return !$this->hasErrors();
    }
}

This validates Category objects.

Now your controller function will need some changes and cleaning up:

function addCategory($id) {
    $validator = new CategoryValidator();
    $data = json_decode(\Slim\Slim::getInstance()->request()->getBody());
    $category = new Category($data);
    $category->setId($id);
    $repository = new DataLayer();

    if ($validator->isValid($category)) {
        $repository->createCategory($category);
        echo '{"category": ' . json_encode($data) . '}';
    } else {
        echo '{"errors": ' . json_encode($validator->getErrors()) . '}';
    }
}

The controller function becomes pretty dumb. The data access layer is pretty dumb. This is the position you want to be in. The controller just does stuff. Why stuff happens is delegated to the validator and domain layers of your application. What to do when stuff happens is the responsibility of the controller function. The data access layer should assume that all data is valid before persisting it to the database, and it should fail noisily and immediately if anything goes wrong (i.e. let it throw exceptions). Also notice that we pass a Category object into the data access layer instead of an array.

People rail against strongly typed code, but there are advantages, and this is one of them. You get a clear idea of what data you are dealing with, so, changes to the data access layer:

class DataLayer {

    public function addCategory (Category $category) {
        $sql = "INSERT INTO categories (
            cat_name, 
            catcolor,
            calendar_id
            ) 
            VALUES (:cat_name, :catcolor, :calendar_id)";

     $db = $this->connect();
     $stmt = $db->prepare($sql);
     $stmt->bindParam("cat_name", $category->getName());
     $stmt->bindParam("catcolor", $category->getColor());
     $stmt->bindParam("calendar_id", $category->getId());
     $stmt->execute();
    }

}

Error Handling

Slim PHP has its own error handling. Read up on it and learn it. Your controller functions don't need to trap exceptions and log them. Just let them bubble up the call stack --- unless you can gracefully handle the exception.

Additional Layers Promotes Unit Testing

Having these layers:

  1. Controller functions
  2. Middleware for authentication and rate limiting
  3. Validators
  4. Domain Models
  5. Data Access

Exposes more of your application to unit tests so you can test your application without a full blown technology stack powering it. These unit tests can cover the functionality in your validators and domain models without invoking a web server or database. Now you can throw a mob of tests at your business logic in the domain model and validator layers, and these tests execute in seconds. Compare that with how long manual testing takes, and you are setting yourself up to iterate on this application very quickly. The test coverage also means that changing frameworks or upgrading dependencies becomes easier. Upgrade the framework and depedencies, run unit tests. When the tests pass, you've got about 60 to 80% of the work done.

As a side story, I worked on a large Rails project a while back. It had close to 40 models, and integrated with over a dozen web services. We upgraded our Ruby on Rails framework, as well as upgraded the base Ruby language itself. We had about 1,800 unit tests, and around 800 automated functional tests that drove the web browser. We completed the non compatible framework, dependency AND language upgrade in one month, start to finish --- and we delivered about 5 new features at the same time. One developer was responsible for the upgrade while the others built the new features. Once the 2,400 unit and functional tests were passing, it was easy peasey, rice and cheesey to upgrade the new features as well.

Two weeks of development, two weeks of QA and user acceptance testing plus software upgrades to all pre production and production servers. This was all possible because of the test coverage we had.

And the test coverage was possible because we had these same layers in our application.

\$\endgroup\$
  • \$\begingroup\$ Terrific answers, helps a lot. The Slim middleware suggestion seems like a great way to go. Out of curiosity, would you put the middleware functions in Slim's actual "middleware" directory, or keep them within the main application? And trust me, I would be I'd be one happy dude to get to the level of efficiency you've described when it comes to QA, some day. ;) \$\endgroup\$ – pmahomme Jun 18 '15 at 16:55
  • \$\begingroup\$ Since the framework has a "middleware" folder, I would put it there. And it really wasn't that QA was efficient. The developers wrote and ran the tests. I guess my point was that our test coverage is what enabled this massive non compatible upgrade while delivering new features. \$\endgroup\$ – Greg Burghardt Jun 18 '15 at 17:45
  • \$\begingroup\$ It was difficult to decide which answer to accept. I'll keep the BLL in place for now as I've discussed this with other developers and they tend to agree with the layering. But I feel that in the long-term I understand your logic and thanks very much for providing code examples, along with solid real-world scenarios. \$\endgroup\$ – pmahomme Jun 23 '15 at 0:57
  • \$\begingroup\$ No. Use the business layer even though it overlaps a lot with database layer. Keep your model separate. \$\endgroup\$ – kta Nov 29 '16 at 1:52
2
\$\begingroup\$

The code looks reasonable to me. Here are a few suggestions though:

Dependency Injection

Dependency Injection helps keep your classes loosely coupled and easily testable. The simplest way to do this would be to bring them up to the top.

As you saw in the DAL each method needed to connect to the database. The DAL depends on the database connection. This dependency can be injected into the DAL layer. This is what dependency injection means, instead of creating the object in the layer we inject it into the layer or class.

class DataLayer
{
    protected $db;

    public function __construct(PDO $db)
    {
        $this->db = $db;
    }

    // Methods follow here without the need to connect.
}

Similarly the Business Layer needs a Data Layer which should be injected.

class BusinessLayer 
{
    protected $dal;

    public function __construct(DataLayer $dal)  {
        $this->dal = $dal;
    }

    // Methods follow.
}

It does seem pointless to create a $bll in each function. So, before $app->run these objects should be created.

Attribute Accessibility

The accessibility should be specified for the attributes of the class. Instead of var $dal you should make it protected $dal. var $dal makes it publicly accessible. This is a bad thing as it can be altered from outside of the class.

Memory Management

PHP runs on each request. It does not sit around all day eating through memory. So just leave it to clean up its own memory. Remove $db = null;.

This also helps for the next section.

Variables should vary

A variable should vary, if not don't create it. It only makes things harder to read. All of the $ret variables don't need to be created, just return the values from the functions directly e.g: $stmt->fetchAll(PDO::FETCH_ASSOC);.

Exception and Error Logging

This needs some cleaning up. I have already written a long answer about Exception and Error Logging which might help you.

After reading that answer the following should make sense to you: I would let the exception bubble up to the Business Logic layer (or higher) to where the failure of the DAL to execute could be understood and handled in an appropriate way.

From that answer you should also see that I would advise against inserting lots of logging throughout your code as PHP allows you to keep it all in one easily maintainable place.

\$\endgroup\$
  • \$\begingroup\$ Thanks, great suggestions. Dependency injection is also new to me so I'm a bit confused. Where does the actual connection take place, constructor or at the Slim level? \$\endgroup\$ – pmahomme Jun 18 '15 at 16:41
  • \$\begingroup\$ Up at the Slim level so that the PDO object that you create can be injected. Basically, you would end up with $bll = new BusinessLayer(new DataLayer(new PDO(/* connection settings */)));. The advantage is that the BusinessLayer and DataLayer can be tested easily by injecting mock objects so that the BusinessLayer is not hardwired to the DataLayer etc. \$\endgroup\$ – Paul Jun 18 '15 at 23:33
  • \$\begingroup\$ Your review was great. I get what you're saying when it comes to Attribute Accessibility, Memory Management, Variables Should Vary and have introduced this in my code. However I'm still having trouble with (I haven't had a chance to study it enough) Dependency Injection but won't forget it and will surely introduce a solid level of Exception and Error Logging per your long answer, which was great! \$\endgroup\$ – pmahomme Jun 23 '15 at 1:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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