From fc781c891a0da3e8976e94b13fd6b9e56ab56fb3 Mon Sep 17 00:00:00 2001 From: Trismegiste Date: Sun, 12 May 2013 20:09:13 +0200 Subject: [PATCH 1/6] comments to explain where the problem is --- ChainOfResponsibilities/ChainOfResponsibilities.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/ChainOfResponsibilities/ChainOfResponsibilities.php b/ChainOfResponsibilities/ChainOfResponsibilities.php index 82ba06e..5b2686d 100644 --- a/ChainOfResponsibilities/ChainOfResponsibilities.php +++ b/ChainOfResponsibilities/ChainOfResponsibilities.php @@ -10,6 +10,8 @@ namespace DesignPatterns; * in the chain and so forth * * Examples: + * - logging framework + * - spam filter * - Caching: first object is an instance of e.g. a Memcached Interface, if that "misses" it delegates the call to the * Database Interface * - Yii Framework: CFilterChain is a chain of controller action filters. the executing point is passed from one filter @@ -17,6 +19,8 @@ namespace DesignPatterns; * */ +// the idea is good but in general, the Handler component in this pattern +// is an abstract class which makes much more of the work interface KeyValueStorage { public function get($key); From 5ef810f3d0e9bcd34d5bb5e28bd0bc852da0efed Mon Sep 17 00:00:00 2001 From: Trismegiste Date: Sun, 12 May 2013 20:52:29 +0200 Subject: [PATCH 2/6] add the generic handler and the request classes --- ChainOfResponsibilities/Handler.php | 58 +++++++++++++++++++++ ChainOfResponsibilities/Request.php | 23 ++++++++ Tests/ChainOfResponsibilities/ChainTest.php | 22 ++++++++ 3 files changed, 103 insertions(+) create mode 100644 ChainOfResponsibilities/Handler.php create mode 100644 ChainOfResponsibilities/Request.php create mode 100644 Tests/ChainOfResponsibilities/ChainTest.php diff --git a/ChainOfResponsibilities/Handler.php b/ChainOfResponsibilities/Handler.php new file mode 100644 index 0000000..43d08e3 --- /dev/null +++ b/ChainOfResponsibilities/Handler.php @@ -0,0 +1,58 @@ +successor)) { + $this->successor = $handler; + } else { + $this->successor->append($handler); + } + } + + /** + * Handle the request. + * + * This approach by using a template method pattern ensures you that + * each subclass will not forget to call the successor. Beside, the returned + * boolean value indicates you if the request have been processed or not. + */ + final public function handle(Request $req) + { + $processed = $this->processing($req); + if (!$processed) { + if (!is_null($this->successor)) { + $processed = $this->successor->handle($req); + } + } + + return $processed; + } + + abstract protected function processing(Request $req); +} \ No newline at end of file diff --git a/ChainOfResponsibilities/Request.php b/ChainOfResponsibilities/Request.php new file mode 100644 index 0000000..d1cddfc --- /dev/null +++ b/ChainOfResponsibilities/Request.php @@ -0,0 +1,23 @@ + Date: Sun, 12 May 2013 22:08:01 +0200 Subject: [PATCH 3/6] handler and request --- ChainOfResponsibilities/Handler.php | 21 +++++++++++++++++---- ChainOfResponsibilities/Request.php | 2 +- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/ChainOfResponsibilities/Handler.php b/ChainOfResponsibilities/Handler.php index 43d08e3..7511221 100644 --- a/ChainOfResponsibilities/Handler.php +++ b/ChainOfResponsibilities/Handler.php @@ -8,11 +8,16 @@ namespace DesignPatterns\ChainOfResponsibilities; /** * Handler is a generic handler in the chain of responsibilities + * + * Yes you could have a lighter CoR with simpler handler but if you want your CoR to + * be extendable and decoupled, it's a better idea to do things like that in real + * situations. Usually, a CoR is meant to be changed everytime and evolves, that's + * why we slice the workflow in little bits of code. */ -abstract class Handler implements KeyValueStorage +abstract class Handler { - protected $successor = null; + private $successor = null; /** * Append a responsibility to the end of chain @@ -23,8 +28,10 @@ abstract class Handler implements KeyValueStorage * bad idea because you have to remove the type-hint of the parameter because * the last handler has a null successor. * - * And if you override the contructor, your Handler can no longer have a - * successor. One solution is to provide a NullObject (see pattern) + * And if you override the contructor, that Handler can no longer have a + * successor. One solution is to provide a NullObject (see pattern). + * It is more preferable to keep the constructor "free" to inject services + * you need with the DiC of symfony2 for example. */ final public function append(Handler $handler) { @@ -46,6 +53,7 @@ abstract class Handler implements KeyValueStorage { $processed = $this->processing($req); if (!$processed) { + // the request has not been processed by this handler => see the next if (!is_null($this->successor)) { $processed = $this->successor->handle($req); } @@ -54,5 +62,10 @@ abstract class Handler implements KeyValueStorage return $processed; } + /** + * Each concrete handler has to implement the processing of the request + * + * @return bool true if the request has been processed + */ abstract protected function processing(Request $req); } \ No newline at end of file diff --git a/ChainOfResponsibilities/Request.php b/ChainOfResponsibilities/Request.php index d1cddfc..6083c57 100644 --- a/ChainOfResponsibilities/Request.php +++ b/ChainOfResponsibilities/Request.php @@ -19,5 +19,5 @@ namespace DesignPatterns\ChainOfResponsibilities; */ class Request { - + // getter and setter but I don't want to generate to much noise in handlers } \ No newline at end of file From b581aef8f2d616b6ce6ec3cd5ef2a59e1b91340b Mon Sep 17 00:00:00 2001 From: Trismegiste Date: Sun, 12 May 2013 22:08:35 +0200 Subject: [PATCH 4/6] adding concrete handler --- .../Responsible/FastStorage.php | 37 ++++++++++++++++ .../Responsible/SlowStorage.php | 44 +++++++++++++++++++ 2 files changed, 81 insertions(+) create mode 100644 ChainOfResponsibilities/Responsible/FastStorage.php create mode 100644 ChainOfResponsibilities/Responsible/SlowStorage.php diff --git a/ChainOfResponsibilities/Responsible/FastStorage.php b/ChainOfResponsibilities/Responsible/FastStorage.php new file mode 100644 index 0000000..0011ea0 --- /dev/null +++ b/ChainOfResponsibilities/Responsible/FastStorage.php @@ -0,0 +1,37 @@ +_data = $data; + } + + protected function processing(Request $req) + { + if ('get' === $req->verb) { + if (array_key_exists($req->key, $this->_data)) { + // the handler IS responsible and then processes the request + $req->response = $this->_data[$req->key]; + // instead of returning true, I could return the value but it proves + // to be a bad idea. What if the value IS "false" ? + return true; + } + } + + return false; + } + +} diff --git a/ChainOfResponsibilities/Responsible/SlowStorage.php b/ChainOfResponsibilities/Responsible/SlowStorage.php new file mode 100644 index 0000000..79635bf --- /dev/null +++ b/ChainOfResponsibilities/Responsible/SlowStorage.php @@ -0,0 +1,44 @@ +_data = $data; + } + + protected function processing(Request $req) + { + if ('get' === $req->verb) { + if (array_key_exists($req->key, $this->_data)) { + $req->response = $this->_data[$req->key]; + return true; + } + } + + return false; + } + +} From 71a43e2e93e79096255f9ad336c39f120c194a39 Mon Sep 17 00:00:00 2001 From: Trismegiste Date: Sun, 12 May 2013 22:08:56 +0200 Subject: [PATCH 5/6] tests for CoR --- Tests/ChainOfResponsibilities/ChainTest.php | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/Tests/ChainOfResponsibilities/ChainTest.php b/Tests/ChainOfResponsibilities/ChainTest.php index cfe8c2f..6a9fa99 100644 --- a/Tests/ChainOfResponsibilities/ChainTest.php +++ b/Tests/ChainOfResponsibilities/ChainTest.php @@ -7,6 +7,7 @@ namespace DesignPatterns\Tests\ChainOfResponsibilities; use DesignPatterns\ChainOfResponsibilities\Request; +use DesignPatterns\ChainOfResponsibilities\Responsible; /** * ChainTest tests the CoR @@ -14,9 +15,22 @@ use DesignPatterns\ChainOfResponsibilities\Request; class ChainTest extends \PHPUnit_Framework_TestCase { + protected $chain; + + protected function setUp() + { + $this->chain = new Responsible\FastStorage(array('bar' => 'baz')); + $this->chain->append(new Responsible\SlowStorage(array('bar' => 'baz', 'foo' => 'bar'))); + } + public function testProcess() { - + $request = new Request(); + $request->verb = 'get'; + $request->key = 'bar'; + + $ret = $this->chain->handle($request); + print_r($request); } } \ No newline at end of file From f92be4efec45bc59a9ff9dd9ab8ea86ab07b6e76 Mon Sep 17 00:00:00 2001 From: Trismegiste Date: Sun, 12 May 2013 22:20:12 +0200 Subject: [PATCH 6/6] full tests --- ChainOfResponsibilities/Handler.php | 1 + Tests/ChainOfResponsibilities/ChainTest.php | 48 +++++++++++++++++++-- 2 files changed, 46 insertions(+), 3 deletions(-) diff --git a/ChainOfResponsibilities/Handler.php b/ChainOfResponsibilities/Handler.php index 7511221..907e6a9 100644 --- a/ChainOfResponsibilities/Handler.php +++ b/ChainOfResponsibilities/Handler.php @@ -51,6 +51,7 @@ abstract class Handler */ final public function handle(Request $req) { + $req->forDebugOnly = get_called_class(); $processed = $this->processing($req); if (!$processed) { // the request has not been processed by this handler => see the next diff --git a/Tests/ChainOfResponsibilities/ChainTest.php b/Tests/ChainOfResponsibilities/ChainTest.php index 6a9fa99..337559b 100644 --- a/Tests/ChainOfResponsibilities/ChainTest.php +++ b/Tests/ChainOfResponsibilities/ChainTest.php @@ -23,14 +23,56 @@ class ChainTest extends \PHPUnit_Framework_TestCase $this->chain->append(new Responsible\SlowStorage(array('bar' => 'baz', 'foo' => 'bar'))); } - public function testProcess() + public function makeRequest() { $request = new Request(); $request->verb = 'get'; - $request->key = 'bar'; + return array( + array($request) + ); + } + /** + * @dataProvider makeRequest + */ + public function testFastStorage($request) + { + $request->key = 'bar'; $ret = $this->chain->handle($request); - print_r($request); + + $this->assertTrue($ret); + $this->assertObjectHasAttribute('response', $request); + $this->assertEquals('baz', $request->response); + // despite both handle owns the 'bar' key, the FastStorage is responding first + $this->assertEquals('DesignPatterns\ChainOfResponsibilities\Responsible\FastStorage', $request->forDebugOnly); + } + + /** + * @dataProvider makeRequest + */ + public function testSlowStorage($request) + { + $request->key = 'foo'; + $ret = $this->chain->handle($request); + + $this->assertTrue($ret); + $this->assertObjectHasAttribute('response', $request); + $this->assertEquals('bar', $request->response); + // FastStorage has no 'foo' key, the SlowStorage is responding + $this->assertEquals('DesignPatterns\ChainOfResponsibilities\Responsible\SlowStorage', $request->forDebugOnly); + } + + /** + * @dataProvider makeRequest + */ + public function testFailure($request) + { + $request->key = 'kurukuku'; + $ret = $this->chain->handle($request); + + $this->assertFalse($ret); + // the last rsponsible : + $this->assertEquals('DesignPatterns\ChainOfResponsibilities\Responsible\SlowStorage', $request->forDebugOnly); } } \ No newline at end of file