Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am currently developing a Content Management System, and I have just completed the Error Handling system, which handles errors according to the user's settings.

I want to know every single thing I could improve to make it more flexible, robust, light weight and fast processing. If there are better ways of doing things, I am ready to re-do stuff. I just want comments on ErrorHandling System, in this global.php you can see how I instantiate them. It would also be helpful if you can suggest renames to variables, etc to make more sense. This project is going to be up on GitHub soon. Please be as strict as possible, and point out even a slight flaw in it.

Global.php

<?php
/**
 * @author Hassan Althaf
 * @link www.HassanTech.org
 * @package IDK CMS
 */
require_once("config.php");

if($configuration['errors']['show_errors'] == 0) {
    error_reporting(0);
}

require_once("ErrorHandling/interface.ErrorHandlerMapper.php");
require_once("ErrorHandling/class.ErrorHandlerController.php");
require_once("ErrorHandling/class.ErrorHandlerLocalMapper.php");
require_once("ErrorHandling/class.ErrorHandlerDatabaseMapper.php");

$database = new mysqli(
    $configuration['database']['host'], 
    $configuration['database']['username'], 
    $configuration['database']['password'],
    $configuration['database']['database']
);

if($configuration['errors']['error_log_method'] == 'local') {
    $errorHandlerMapper = new CMS\Core\ErrorHandling\ErrorHandlerLocalMapper();
} elseif($configuration['errors']['error_log_method'] == 'database') {
    $errorHandlerMapper = new CMS\Core\ErrorHandling\ErrorHandlerDatabaseMapper($database);
}

$errorHandlerController = new CMS\Core\ErrorHandling\ErrorHandlerController($errorHandlerMapper);

if($configuration['errors']['enable_error_logs'] == 1) {
    if($database->connect_errno) {
        echo $database->connect_error;
    }
}

I have 3 classes and 1 interface.

class.ErrorHandlerController.php:

<?php
namespace CMS\Core\ErrorHandling;
class ErrorHandlerController {
    private $errorHandlerMapper;

    public function __construct(ErrorHandlerMapper $errorHandlerMapper) {
        $this->errorHandlerMapper = $errorHandlerMapper;
    }

    public function handleErrorLogging($error) {
        return $this->errorHandlerMapper->log($error);
    }

    public function handleReturningErrorList() {
        return $this->errorHandlerMapper->returnErrorList();
    }

    public function handleErrorListTruncating() {
        return $this->errorHandlerMapper->truncateErrorList();
    }
}

class.ErrorHandlerDatabaseMapper.php:

<?php
namespace CMS\Core\ErrorHandling;
use mysqli;

class ErrorHandlerDatabaseMapper implements ErrorHandlerMapper {
    private $database;

    public function __construct(mysqli $database) {
        $this->database = $database;
    }

    public function log($error) {
        $query = $this->database->prepare("INSERT INTO errors VALUES('', ?);");
        $query->bind_param('s', $error);
        $query->execute();
        $query->close();
        return true;
    }

    public function returnErrorList() {
        $query = $this->database->prepare("SELECT error FROM errors");
        $query->execute();
        $query->bind_result($error);
        while($query->fetch()) {
            $errors[] = $error;
        }
        $query->close();
        return $errors;
    }

    public function truncateErrorList() {
        $query = $this->database->prepare("TRUNCATE TABLE errors");
        $query->execute();
        $query->close();
        return true;
    }
}

class.ErrorHandlerLocalMapper.php:

<?php
namespace CMS\Core\ErrorHandling;
class ErrorHandlerLocalMapper implements ErrorHandlerMapper {
    public function log($error) {
        return file_put_contents('errorLog.txt', file_get_contents('errorLog.txt') . $error . ";");
    }

    public function returnErrorList() {
        return explode(';', file_get_contents('errorLog.txt'));
    }

    public function truncateErrorList() {
        return file_put_contents('errorLog.txt', '');
    }
}

interface.ErrorHandlerMapper.php:

<?php
namespace CMS\Core\ErrorHandling;
interface ErrorHandlerMapper {
    public function log($error);
    public function returnErrorList();
    public function truncateErrorList();
}
share|improve this question
1  
What if your database is somehow unreachable? What happens with the errors? –  Kid Diamond Aug 31 at 17:28

1 Answer 1

up vote 3 down vote accepted

Errorhandler or ErrorLogger?

As far as I can see, you wrote an ErrorLogger, not an error handler. You are not registering an error handler. The only thing these classes seem to do is Logging and giving access to the errors - with the option to truncate them.

So in fact, you created a Logger that can delete all errors (now why would you do that?), so call it a Logger.

The good peeps over at psr - standards have create a really nice interface for a Logger. Implementing that interface would be a big plus since you could easily change it with a complete different logging engine if needed.

Code "errors"

You are using namespaces, that's good. But the location of your files is just illogical. Stick to psr-4 or psr-1. Standards exist because they are proved to be good. And instead of using all those requires. Use an auto loader instead. Write a custom one or use a complete solution like Symfony loader or even composer.

In your database you are only saving the error, nothing more. Some extra information would be useful. Like when did it occur for instance.

If every method starts with the same word and if that word can also be found in the class name, you probably don't need it. Writing:

$errorHandler = new ErrorHandlerController;
$errorHandler->handleErrorLogging($error);

It feels weird. You say that it should handle logging errors. But you need an error to handle error logging. But the only thing it does is log an error. So what does the ErrorHandlerController do? It passes everything to the ErrorMapper. So why not use the error mapper?

$errorHandler = new ErrorHandlerDatabaseMapper;
$errorHandler->log($error);

Way more readable.

Then your method name: returnErrorList. Return to where? The client? What you are doing is 'getting'. So getErrors or getErrorsAsList. But what is a list? It's just an array

The same goes for truncateErrorList. What is the List keyword doing there? And why is it returning something? This means that every piece of code that truncates all the errors needs to check for success, so throw an Exception.

When using the local error mapper problems will arise if the error message contains ';'.

And as a last remark. If you are creating an ErrorHandler and after you have created your error handler you write:

if($configuration['errors']['enable_error_logs'] == 1) {
    if($database->connect_errno) {
        echo $database->connect_error;
    }
}

Your class failed. It is not handling errors.

Disclaimer: everything I wrote is here to help you; If you feel offended, I apologise. This is not what I intended.

share|improve this answer
    
I didnt update that part of the code yet. –  Hassan Althaf Sep 1 at 10:03

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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