6
\$\begingroup\$

I have to create some APIs for a online game and I decided to learn to use namespaces correctly.

I used the factory pattern, then the loadModule method checks if in the Modules folder there is a class named $module and then it return the object. The folders are synchronized with the namespaces.

The main class Metin2.class.php:

<?php

namespace Metin2;
use mysqli;
use \Metin2\Utilities as Utilities;
require_once("config.php");
require_once(UTILITIES_PATH . "Autoloader.class.php");

class Metin2
{
    public $db;

    public function __construct(mysqli $database)
    {
        spl_autoload_register("Metin2\\Utilities\\Autoloader::loadModel");
        try {
            if(!$database) throw new Exceptions\Metin2Exception("The database instance     is not a valid instance");
            $this->db = new Utilities\Database($database);

        } catch (Exceptions\Metin2Exception $e) {
            echo $e->errorMessage();
        }

    }

    public function loadModule($module)
    {
        $m = "\\Metin2\\Modules\\" . $module;
        return new $m;
    }
}

Autoloader class:

<?php
namespace Metin2\Utilities;


class Autoloader {
    public static function loadModel($class){
        $class = str_replace("\\","/",$class);
        $class = explode("/",$class);
        if($class[0] == "Metin2")unset($class[0]);
        $class = implode("/",$class);
        $path = ROOT_PATH.$class.".class.php";
        if(file_exists($path)){
            require_once($path);
        }
    }
}

An example of a module:

<?php
namespace Metin2\Modules;

class Testdb {

    public $db;
    public function __construct(){
        $this->db = \Metin2\Utilities\Database::getInstance();
        print_r($this->db->getOne("account"));
    }
}

Everything works well, but I want to know if there are any things I did wrong or if there are things I can do better.

\$\endgroup\$
5
\$\begingroup\$

My thoughts:

  1. Imho Database should not belong to Utilities namespace. Working with database is likely a core part of your application, "Utilities" suggest auxiliary tools to make your life easier. But maybe the class does something different than I expect

  2. Is there a reason to keep all your application in Metin2 namespace but have separate Exceptions namespace with Metin2Exception? If it's for code completition, then a good IDE should only suggest classes inheriting from Exception in these contexts.

  3. Metin2::__constructor(): can Utilities\Database throw Metin2Exception? If not, then it's not really useful to throw exception only to catch it right away and echo

  4. It's worth considering using specialized Exception classes and keep Exception messages for details. That is because you can't catch Exception based on its message, but only class. Here you could have ConnectionFailedException extends Metin2Exception.

  5. Instead of echoing Exception message I would recommend making a function/class, that would enable you to redirect error messages to file, as it's not recommended to be printing out application messages for users to see. This way you'll also be able to log them.

  6. On a similar note you may take a look at set_exception_handler, that lets you assign a function/method to handle uncaught exceptions. And from that you can try either making your own or using a 3rd party exception handlers, which combined with set_error_handler that can be used for conversion of errors to exceptions can really improve your programming. Examples: https://github.com/nette/tracy or ... maybe http://raveren.github.io/kint/ (I'm sure there's another as nice as Tracy, but I can't remember its name).

  7. You can remove dependency of Autoloader on a global constant by adding a static property and using a setter to set its value

  8. When Autoloader finds out the target filepath does not exist, it does not react in any way ... maybe at least log it somehow, if not throw exception?

  9. What's the purpose of Testdb? I would discourage from using a service location especially when it comes to testing ... why not pass the $db as an argument?

\$\endgroup\$
  • 1
    \$\begingroup\$ 1. I put Database class in the Utilities namespaces because the class make easier my life with its methods. 2. No, it isn't. I corrected this before 3. Yes, it does 4. Yes, you are right, thanks 5. In future i'm going to do as you wrote, i have done so because of testing .6 Wow very useful, i didn't know that. Thanks 7. I don't understand about what dependency are you talking abount 8. Yes, i corrected this stupid thing five minutes ago. 9. It was made only for check if the database and the autoloader works. \$\endgroup\$ – P.Danielski Mar 18 '14 at 14: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.