2

Today I have a PHP-project with very strange class structure. Like this:

db_base
  `-- db_base_ext extends db_base
        +-- module_1 extends db_base_ext
        +-- module_2 extends db_base_ext
        .   ...
        +-- module_N extends db_base_ext
        `-- db_user extends db_base_ext

class_1
  `-- submodule_1_1 extends class_1

db_base connects to db in __construct() and have some helper methods.

example of executing:

$db = new db_user();
$user = new user($db);
unset($db);

$db = new module_2();

I don't like that in this code we connects to DB twice and the whole class structure is not really good.

How can I make it better? I mean create only one DB instance and after this work with any class (corresponding to DB)?

Will singleton pattern be good for this?

hakre
  • 178,314
  • 47
  • 389
  • 754
Yekver
  • 4,347
  • 7
  • 29
  • 48
  • What makes you think it is connecting twice? – Jake N Aug 04 '13 at 10:13
  • @jakenoble first connect in first string, disconnect in 3rd string, second connect in 5th string. – Yekver Aug 04 '13 at 10:42
  • Perhaps use a library instead. http://redbeanphp.com/ ? – Jake N Aug 04 '13 at 10:49
  • possible duplicate of [How to properly set up a PDO connection](http://stackoverflow.com/questions/11369360/how-to-properly-set-up-a-pdo-connection) – tereško Aug 04 '13 at 16:18
  • possible duplicate of [How can I use "Dependency Injection" in simple php functions, and should I bother?](http://stackoverflow.com/questions/2255771/how-can-i-use-dependency-injection-in-simple-php-functions-and-should-i-bothe) – hakre Aug 11 '13 at 05:33

3 Answers3

4

Your problem here is that, your structures,

module_2 extends db_base_ext
db_user extends db_base_ext
submodule_1_1 extends class_1

they all do break both the Single Responsibility Principle and Liskov Substitution Principle.

How can I make it better. I mean create only one DB instance and after this work with any class (corresponding to DB), is possible?

Dependency injection is the way to go. You would instantiate one db instance and all your classes would share the same $db instance.

final class MySQLPDO extends PDO
{
    public function __construct(array $params)
    {
      parent::__construct(sprintf('mysql: host=%s; dbname=%s', $params['host'], $params['database']), $params['username'], $params['password']);

      $this->setAttribute(parent::MYSQL_ATTR_INIT_COMMAND, 'SET NAMES UTF8');
      $this->setAttribute(parent::ATTR_ERRMODE, parent::ERRMODE_EXCEPTION);
      $this->setAttribute(parent::ATTR_EMULATE_PREPARES, false);
      $this->setAttribute(parent::ATTR_DEFAULT_FETCH_MODE, parent::FETCH_ASSOC);

    }
}

$db = new MySQLPDO(array(
     'host'     => 'localhost',
     'database' => 'foo',
     'username' => 'root',
     'password' => '',
));
$user = new User($db);

$module = new Module1($user);
$foo = new Foo($db);

So, what would you gain here? Both reuse-ability and test-ability.


Note, you should avoid Singleton's as they introduce another form of global state, which is bad for unit-testing purposes.

Yang
  • 8,267
  • 7
  • 30
  • 53
  • Im not sure how the one instance of your proposal differs from a Singleton implementation. Could you explain the difference id be quite interested – zewa666 Aug 04 '13 at 13:00
  • I hope you mean `$module = new Module1($db);` ? – Yekver Aug 04 '13 at 15:32
  • @zewa666 The only difference is that, you would call your Singleton like `$db = DB::getInstance()`. Since you call it like `DB::getInstance()` you introduce another form of **global state**. `DI` should be used instead of `Singletons`... – Yang Aug 04 '13 at 15:50
  • @Yekver yeah could be also `Module($db)`. I just assumed that your module depend on users – Yang Aug 04 '13 at 15:51
  • Actually I doubt that static modifiers immediately enforce bad global spaces cause thats actually the only difference. Redbeanphp e.g. uses them throughout the whole framework – zewa666 Aug 04 '13 at 18:29
  • 1
    Don't name this creating class adapter. All it does is transposing the params array (magic keys, undocumented usage, bad idea) into a new PDO configuration. A single function could have done that, just providing the configuration. But I actually just wanted to comment on the name :) So Adapter as name is a smell, can you imagine a better name for that class? – hakre Aug 11 '13 at 05:32
  • @hakre well, a single closure function works as of `PHP5.3+`, while this thing would for outdated versions as well.. And yes, I appreciate your advice... maybe a better name would look like this `MySQLPDOProvider`.. – Yang Aug 11 '13 at 07:15
  • 1
    If you extend from PDO, it's not a `PDOProvider`, it's just a `PDO`, e.g. `MyPDO`. A provider would return a PDO when you call a method named `provide()` or similar. At least that's how I would understand it. - The closure was just exemplary, and well, PHP 5.3 is end of life. If you give suggestions, please keep it to the appropriate current stable PHP versions here on SO. It's not a question asked back in 2008 (not saying that closure is the solution to the problem). – hakre Aug 11 '13 at 07:37
2

Use proper class names

Firstly I noticed the classes in the project are named like functions. If the project tries to use PEAR naming conventions it should do it right.

Make modules independent from the database class

You should disconnect the modules from the db_base_ext class. If a module needs access to a database instance it can be provided using constructor arguments or setters. It should not extend the class directly.

$module = new Module($database);
Community
  • 1
  • 1
Bart
  • 16,076
  • 5
  • 54
  • 79
  • Are you sure that this is a good Idea to disconnect all modules from `db_base_ext`? Why is it necessary? In codeigniter for exaple all models extends basic class `CI_Model`. – Yekver Aug 04 '13 at 15:39
  • @Yekver I'm absolutely sure :) Just ask yourself this question: "Is a module a type of database?". A module clearly is not. So it should not try to act like a type of database or an extension of it. – Bart Aug 04 '13 at 15:56
  • 2
    @Yekver Investigate exactly what CI does and do exactly the opposite of that and you should be ok. – PeeHaa Aug 04 '13 at 15:57
-2

You should think about using a singleton pattern for your DB Class.

Take a look here for a tutorial. It uses PDO for the communication but should be also valid for mysqli or other types.

zewa666
  • 2,558
  • 15
  • 20