Discussion of testing theory and practice, including methodologies (such as TDD, BDD, DDD, Agile, XP) and software - anything to do with testing goes here. (Formerly "The Testing Side of Development")
<?php
class DNS_Db
{
/**
* Holds an instance of this class
*
* @var DNS_Db
* @access private
* @static
*/
static private $_db = null;
/**
* Grab (the only) an instance of this database object
* This is a singleton class
*
* @return DNS_Db
*/
public static function getInstance()
{
if (is_null(self::$_db))
{
self::$_db = new DNS_Db;
}
return self::$_db;
}
}
?>
I am going to continue, but please let me know if there's anything wrong. (if you were directed here from the devnetstore thread, please follow this thread as it progresses )
public function testClassExists()
{
$this->assertTrue(class_exists('DNS_Db'));
}
This is the sort of superfluous stuff I refer to leaving out of tests That's not testing your code's behaviour or interface
I tend to create a directory "units" and within that a load of files runAllTests.php, runTestOfFoo.php, runTestOfBar.php. Then I keep the actual test case classes in separate files which can be included wherever needed. I guess that's what you're doing here? Putting the test runner code in the test case itself causes complications when you need to use the test in another suite of tests.
"static private" should technically be "private static" on a side note
Apart from that rather pointless first test method the rest looks fine. Now test you do actually get a singleton
<?php
require_once '../library/DNS/Db.php';
class DNS_DB_Test extends UnitTestCase
{
public function testGetInstance()
{
$db = DNS_Db::getInstance();
$this->assertIsA($db, 'DNS_Db');
}
public function testInstanceIsASingleton()
{
$a = DNS_Db::getInstance();
$b = DNS_Db::getInstance();
$this->assertReference($a, $b);
}
}
?>
My code passed without any adjustments... see now I'm lost. This is about as far as I get and then I don't know what to do. OK, I'm thinking real hard on this one... now I want to test that getInstance will throw an exception (return null) if it is missing params (which will be $host, $user, and $pass). Does that sound like it's in the ballpark? ::fingers crossed::
I tend to test by method, so I'll have a testInstance for the classes instance() method, and a testFetchArray for the fetchArray() method, should it have one. I'm testing my classes interface, so any accessor is tested to be returning the expected values. If they don't, I just need to know which method/accessor is not behaving expectedly.
Think ahead a few steps. A database can exist without a password, so if $pass is null you could still get a valid object at the end. I also think getInstance() should not have parameters. I think you're largely reaching for a Factory Pattern. Have a factory() static method which handles all the creational logic. e.g.
class DNS_Db_Exception extends Exception
{
}
class DNS_Db
{
private static $_instance = null;
private $_connection = null;
private function __construct()
{
}
/**
* If we want to force the user through a factory,
* might make this private...
*/
public static function getInstance()
{
if(is_null(self::$_instance))
{
self::$_instance = new self();
}
return self::$_instance;
}
public static function factory($database, $user, $password = null)
{
if(!isset($database) || empty($database) || !is_string($database))
{
throw new DNS_Db_Exception('No valid database name has been supplied');
}
if(!isset($user) || empty($user) || !is_string($user))
{
throw new DNS_Db_Exception('No valid user name has been supplied');
}
$instance = self::getInstance();
/**
* Attempt connection, check for failure, throw exception on error,
* otherwise return the instance! Have setters for database details
* Need to determine the database driver later.
*/
return $instance;
}
}
I think this is why you felt something was flawed. Hopefully I added little more but the "next" step (extract creation logic from getInstance() to factory()). I also added an Exception throw (better than returning a value for any error - which gets unpredictable), and changed $_db to $_instance - more descriptive. Also you can create an object using the self keyword .
I have not added this, but a singleton usually isn't directly instantiable - you can add a private __construct() method to block that.
require_once 'DNS/Db.php';
class DNS_Db_Test extends UnitTestCase
{
/*
* Removed first test, we already know the class
* type since we use it to call static methods
*/
/*
* getInstance() is currently public
*/
public function testInstanceIsASingleton()
{
$a = DNS_Db::getInstance();
$b = DNS_Db::getInstance();
$this->assertReference($a, $b);
}
public function testFactory_DatabaseException()
{
$this->expectException(new DNS_Db_Exception('No valid database name has been supplied'));
$db = DNS_Db::factory('', 'validuserstring');
}
public function testFactory_UsernameException()
{
$this->expectException(new DNS_Db_Exception('No valid user name has been supplied'));
$db = DNS_Db::factory('validdatabasestring', '');
}
public function testFactory_MultipleCallsReturnReference()
{
$db1 = DNS_Db::factory('str1', 'str1');
$db2 = DNS_Db::factory('str2', 'str2');
$this->assertReference($db1, $db2);
}
}
Of course may need to handle where user is trying to connect to a different database or with different credentials with second/third/other calls to factory().
care to explain why your first requirement is that your DB class is a singleton?
it may be just me but i really dislike the way singletons are used in php.
they tend to give you nightmares testing the code. imagine you need more than one DB object?
Or what happens if you call
right.... you get the same instance but with unexpected credentials . the second call will return an object with the credentials from the first call (depends on your implementation). in my eyes that will make your tests dependant on each other. one workaorund is a method to clear the internal singleton reference but that is not an ideal solution.
the ideal singleton for me would be transparent to the client code. what i mean is the client should not need to know if he deals with a singleton or not. i know that this is hard to achieve in php but there are other ways getting arround it (Registry, Factory...).
With a database connection, it's set early in the application run. If handled correctly the factory() is called once, and the created object passed as a parameter to other levels of the application.
Controllers in the MVC can have a setParam() method for example. So long as there's very light coupling (classes not directly using the DNS_Db classname to call static methods everywhere for example) it should be fine.
Maugrim_The_Reaper wrote:With a database connection, it's set early in the application run. If handled correctly the factory() is called once, and the created object passed as a parameter to other levels of the application.
Controllers in the MVC can have a setParam() method for example. So long as there's very light coupling (classes not directly using the DNS_Db classname to call static methods everywhere for example) it should be fine.
yep, you are right. but that would be the point to factor the getInstance out (: if it is called only once get rid of it and use the well known instanciation with new.
Maugrim_The_Reaper wrote:With a database connection, it's set early in the application run. If handled correctly the factory() is called once, and the created object passed as a parameter to other levels of the application.
Controllers in the MVC can have a setParam() method for example. So long as there's very light coupling (classes not directly using the DNS_Db classname to call static methods everywhere for example) it should be fine.
yep, you are right. but that would be the point to factor the getInstance out (: if it is called only once get rid of it and use the well known instanciation with new.
cheers
Chris
I see no harm in having the singleton. You can always make the constructor public and allow new instances if requested, or provide a facotry getNewInstance(). With something like a database connection I see a purpose in it. Granted, singletons are used badly elsewhere.
wow, I can now see I still have not come to grasp this concept yet... very frustrating. Maybe I should start with a really simple task. Can somebody think of a really simple task.. something that would take an "easy-to-think-of" interface? I think coming up with the interface I want is what I'm having a hard time with... I guess that means it's time to hit the books again. I thought I had this stuff down.
EDIT: Ok, I'm going with a registry, since I know how I would want a registry's interface to be (more or less)
class DNS_Registry_Test extends UnitTestCase
{
public function testGetInstance()
{
$registry = DNS_Registry::getInstance();
$this->assertIsA($registry, 'DNS_Registry');
$a = DNS_Registry::getInstance();
$b = DNS_Registry::getInstance();
$this->assertReference($a, $b);
}
public function testSet()
{
$registry = DNS_Registry::getInstance();
$registry->set('name', 'value');
// How would I test that it actually set the value, since I want DNS_Registry's data to be private?
}
}
I mentioned this in your other thread, but remember that TDD is a Design process -- coding is still coding. So the question is always "What is the next design issue?" You started by saying that implementing a Singleton was the first design issue. And you got flack because current thinking is that you only create a Singleton when forced to. So this "pair" design session is working because another coder said "Oi Ninja! Remember about Singletons!" So get back to your quest of "I'm writing an active record library... again" and think your way backwards to the first thing you would need -- probably a database connection.
I agree, your first example is simply demonstrating "pair programming" - two or more people working alongside each other on the same code. If you followed your own track, and maintained tests, you would still have a reversible design decision capable of being made down the line. In TDD, you rarely (might have to mock classes that don't exist yet and whose presence you have inferred) have an interface designed before you write your first test. Using a Singleton was fine, using getInstance() to pass parameters was fine. You haven't made any *bad* decisions yet .
alright, I'm still going with the registry for now though... I just think that since I'm pretty aware of how it should interface, it will be an easier "first real tdd experience"
I'll pick the DNS_Db class when I finish that (for those of you on the Devnetstore project, this stuff is just in that folder, so that's why the DNS prefix... this is not code I'm committing or anything - fyi)
I think I figured out the answer to the question that was commented in the last code posted...
class DNS_Registry_Test extends UnitTestCase
{
public function testGetInstance()
{
$registry = DNS_Registry::getInstance();
$this->assertIsA($registry, 'DNS_Registry');
$a = DNS_Registry::getInstance();
$b = DNS_Registry::getInstance();
$this->assertReference($a, $b);
}
public function testSet()
{
$registry = DNS_Registry::getInstance();
$registry->set('name', 'value');
// How would I test that it actually set the value, since I want DNS_Registry's data to be private?
// You wouldn't, that isn't part of the interface, it's part of the implementation. Next thing to test would be that it only accepts objects.
}
}
just on a side note... does somebody want to help explain mock objects? (I've read about them on lastcraft.com, but I could use a little further explaining)