I'll give it a try first ok? I'm deep into the php manual trying to figure something out
TDD workshop. Anybody welcome; Data storage abstraction.
Moderator: General Moderators
Re: TDD workshop. Anybody welcome; Data storage abstraction.
I'm busy
I'll give it a try first ok? I'm deep into the php manual trying to figure something out
I'll give it a try first ok? I'm deep into the php manual trying to figure something out
- Chris Corbyn
- Breakbeat Nuttzer
- Posts: 13098
- Joined: Wed Mar 24, 2004 7:57 am
- Location: Melbourne, Australia
Re: TDD workshop. Anybody welcome; Data storage abstraction.
No worries, I'm half asleep myself anyway
Time to hit the sack!
Re: TDD workshop. Anybody welcome; Data storage abstraction.
Ok, I made some tests and got them to pass. But it feels very messy. I think I (mis)used almost every array function there is but at the same time didn't find the single one that would have saved me 76 lines of code
Maybe we should also write some tests for these helper functions to make sure they do what they are supposed to do 
Ok, please some feedback. In the mean time I'll continue with the remove tests and trying to clean up the mess.
Code: Select all
// DataStoreTest
public function testGetDeeperNestedKeyReturnsRightValue(){
$storageModel = $this->_createStorageModel();
$storageModel->setReturnValue('load', array('foo' => array('bar'=>array('zip'=>'some value'))));
$dataStore = $this->_createDataStore($storageModel);
$this->assertEqual('some value',$dataStore->get('foo/bar/zip'));
}
public function testHasDeeperNestedValue(){
$this->_dataStore->set('foo/bar/zip', 'some value');
$this->assertTrue($this->_dataStore->has('foo/bar/zip'));
}
Code: Select all
// DataStore
public function get($key) {
$array = split("/", $key);
$key = end($array);
if($this->has($key)){
return $this->array_search_recursive($key, $this->_store);
} else {
return null;
}
}
public function has($key){
return $this->array_key_exists_r($key, $this->_store);
}
private function array_key_exists_r($needle, $haystack) {
$result = array_key_exists($needle, $haystack);
if ($result) return $result;
foreach ($haystack as $k => $v) {
if (is_array($v)) {
$result = $this->array_key_exists_r($needle, $v);
}
if ($result) return $result;
}
return false;
}
private function array_search_recursive($needle, $haystack) {
foreach ($haystack as $key => $value)
{
if (is_array($value)) {
$node = $this->array_search_recursive($needle, $value);
}
elseif ( $key === $needle ) {
$node = $value;
}
}
return $node;
}
Ok, please some feedback. In the mean time I'll continue with the remove tests and trying to clean up the mess.
- Chris Corbyn
- Breakbeat Nuttzer
- Posts: 13098
- Joined: Wed Mar 24, 2004 7:57 am
- Location: Melbourne, Australia
Re: TDD workshop. Anybody welcome; Data storage abstraction.
No-no. That's an instant red cardmatthijs wrote:Maybe we should also write some tests for these helper functions to make sure they do what they are supposed to do
The code doesn't look bad to me
Tests:
Code: Select all
public function testSetDeeperNestedValue() {
$this->_dataStore->set('foo/bar/zip', 'some value');
$this->assertEqual('some value', $this->_dataStore->get('foo/bar/zip'));
}
public function testHasDeeperNestedValue() {
$this->_dataStore->set('foo/bar/zip', 'some value');
$this->assertTrue($this->_dataStore->has('foo/bar/zip'));
}
public function testRemoveDeeperNestedValue() {
$this->_dataStore->set('foo/bar/zip', 'some value');
$this->_dataStore->remove('foo/bar/zip');
$this->assertFalse($this->_dataStore->has('foo/bar/zip'));
}
public function testNestedValuesCanBeQueriedFromStorageModel() {
$storageModel = $this->_createStorageModel();
$storageModel->setReturnValue('load', array(
'foo' => array(
'bar' => array(
'zip' => 'some value'
)
)
));
$dataStore = $this->_createDataStore($storageModel);
$this->assertEqual('some value', $dataStore->get('foo/bar/zip'));
}Code: Select all
<?php
require_once dirname(__FILE__) . '/StorageModel.php';
class DataStore {
private $_storageModel;
private $_store = array();
public function __construct(StorageModel $storageModel) {
$this->_storageModel = $storageModel;
$this->_store = (array) $this->_storageModel->load();
}
public function set($key, $value) {
$keys = explode('/', $key);
$endKey = array_pop($keys);
$container =& $this->_store;
foreach ($keys as $nextKey) {
if (!array_key_exists($nextKey, $container)) {
$container[$nextKey] = array();
}
$container =& $container[$nextKey];
}
$container[$endKey] = $value;
}
public function get($key) {
if ($this->has($key)) {
$keys = explode('/', $key);
$endKey = array_pop($keys);
$container =& $this->_store;
foreach ($keys as $nextKey) {
$container =& $container[$nextKey];
}
return $container[$endKey];
}
}
public function has($key) {
$keys = explode('/', $key);
$endKey = array_pop($keys);
$container =& $this->_store;
foreach ($keys as $nextKey) {
if (array_key_exists($nextKey, $container)
&& is_array($container[$nextKey])) {
$container =& $container[$nextKey];
} else {
return false;
}
}
return array_key_exists($endKey, $container);
}
public function remove($key) {
if ($this->has($key)) {
$keys = explode('/', $key);
$endKey = array_pop($keys);
$container =& $this->_store;
foreach ($keys as $nextKey) {
$container =& $container[$nextKey];
}
unset($container[$endKey]);
}
}
public function save() {
return $this->_storageModel->save($this->_store);
}
}- Chris Corbyn
- Breakbeat Nuttzer
- Posts: 13098
- Joined: Wed Mar 24, 2004 7:57 am
- Location: Melbourne, Australia
Re: TDD workshop. Anybody welcome; Data storage abstraction.
Here's my previous version after the first round of refactoring. What I did was look for any temporary variables I could replace with a query. This turned out to be just the _getEndKey() method and the introduction of the array_slice() for the loop.
I then looked for common logic in each method and extracted a new method for that logic ( &_getContainer()). I did have to add an optional parameter to make it work, but it works.
What we should do now is go through your version and try to employ similar techniques to refactor it. I fear you may have refactored a little too early though since your new methods are already present. This is a fairly normal practise; but often you miss things which can be simplified if you just duplicate code first.
EDIT | Looking at yours again; did you miss the implementation of set() as well as remove() or did you just forget to post it?
I then looked for common logic in each method and extracted a new method for that logic ( &_getContainer()). I did have to add an optional parameter to make it work, but it works.
Code: Select all
<?php
require_once dirname(__FILE__) . '/StorageModel.php';
class DataStore {
private $_storageModel;
private $_store = array();
public function __construct(StorageModel $storageModel) {
$this->_storageModel = $storageModel;
$this->_store = (array) $this->_storageModel->load();
}
public function set($key, $value) {
$container =& $this->_getContainer($key, true);
$container[$this->_getEndKey($key)] = $value;
}
public function get($key) {
if ($this->has($key)) {
$container =& $this->_getContainer($key);
return $container[$this->_getEndKey($key)];
}
}
public function has($key) {
$container =& $this->_getContainer($key);
return (is_array($container)
&& array_key_exists($this->_getEndKey($key), $container));
}
public function remove($key) {
if ($this->has($key)) {
$container =& $this->_getContainer($key);
unset($container[$this->_getEndKey($key)]);
}
}
public function save() {
return $this->_storageModel->save($this->_store);
}
// -- Private methods
private function _getEndKey($key) {
$keys = explode('/', $key);
return array_pop($keys);
}
private function &_getContainer($key, $createIfNotFound = false) {
$container =& $this->_store;
foreach (array_slice(explode('/', $key), 0, -1) as $nextKey) {
if (!array_key_exists($nextKey, $container)) {
if (!$createIfNotFound) {
return false;
}
$container[$nextKey] = array();
}
$container =& $container[$nextKey];
}
return $container;
}
}EDIT | Looking at yours again; did you miss the implementation of set() as well as remove() or did you just forget to post it?
Re: TDD workshop. Anybody welcome; Data storage abstraction.
Good morning 
I did not forget to post set() and remove(). I just had not done those yet.
I'll get my coffee and take a good look at your code. Or maybe first try to do the remove and set myself.
I did not forget to post set() and remove(). I just had not done those yet.
The thing is, I didn't refactor anything. The only way I was able to get the functions to work was writing these private helper functions. I'm just beginning to learn this.What we should do now is go through your version and try to employ similar techniques to refactor it. I fear you may have refactored a little too early though since your new methods are already present. This is a fairly normal practise; but often you miss things which can be simplified if you just duplicate code first.
I'll get my coffee and take a good look at your code. Or maybe first try to do the remove and set myself.
- Chris Corbyn
- Breakbeat Nuttzer
- Posts: 13098
- Joined: Wed Mar 24, 2004 7:57 am
- Location: Melbourne, Australia
Re: TDD workshop. Anybody welcome; Data storage abstraction.
Do try and finish those methods yourselfmatthijs wrote:I'll get my coffee and take a good look at your code. Or maybe first try to do the remove and set myself.
Another thing. Having to write 70 lines of code to solve a problem doesn't mean it's not correct. It may be the only way through a problem and you can always try to figure out if you can refactor later.
If you needed to write those private methods to get the test to pass then that's perfectly valid. Sorry, I just assumed they were the result of early optimization
Re: TDD workshop. Anybody welcome; Data storage abstraction.
Early desperation you mean?Sorry, I just assumed they were the result of early optimization
A more general question though: do you have everything in subversion? I mean, even in a "simple" class like this, as soon as I get a cascade of fails, start to rewrite a few functions, etc, it's quickly almost impossible to go back to working code. Or do you have other tricks for that?
- Chris Corbyn
- Breakbeat Nuttzer
- Posts: 13098
- Joined: Wed Mar 24, 2004 7:57 am
- Location: Melbourne, Australia
Re: TDD workshop. Anybody welcome; Data storage abstraction.
I use subversion, though I haven't for this. I can set up a repository if you like and we can all have a branch each, merging agreed changes into the trunk?matthijs wrote:Early desperation you mean?Sorry, I just assumed they were the result of early optimization
A more general question though: do you have everything in subversion? I mean, even in a "simple" class like this, as soon as I get a cascade of fails, start to rewrite a few functions, etc, it's quickly almost impossible to go back to working code. Or do you have other tricks for that?
Re: TDD workshop. Anybody welcome; Data storage abstraction.
Maybe later ok? My branch would be a dead end anyway, I'll just experiment and test a bit locally for now.
Re: TDD workshop. Anybody welcome; Data storage abstraction.
Ok, maybe skip that. The point is to learn more about TDD. I'll figure out the code later.
I have 15 passing tests now. What should be our next step?
What about testing what happens when the input is not a simple clean string? Or testing if we can search for a key or value in the store?
I have 15 passing tests now. What should be our next step?
What about testing what happens when the input is not a simple clean string? Or testing if we can search for a key or value in the store?
- Chris Corbyn
- Breakbeat Nuttzer
- Posts: 13098
- Joined: Wed Mar 24, 2004 7:57 am
- Location: Melbourne, Australia
Re: TDD workshop. Anybody welcome; Data storage abstraction.
Ok. We should probably consolidate back down to a single code base again just so we're working on the same thingmatthijs wrote:Maybe later ok? My branch would be a dead end anyway, I'll just experiment and test a bit locally for now.
I don't know if you wanted to post your code to have a quick review or if you'd rather just move on with my code? I can see that this was a complex part of the system to tackle... more complex than any of the other things we'll write I'd say
Once we're back to a single code base we'll test adding other value types if you like. Realistically it's only going to be scalar values and arrays which can be persisted.
Re: TDD workshop. Anybody welcome; Data storage abstraction.
chris I think your code is not ok
For example if you have this array:
... and you want foo/bar2/zip2
For example if you have this array:
Code: Select all
array(
'foo' => array(
'bar' => array(
'zip' => 'some value'
)
),
'bar2' => array(
'zip2' => 'some other value'
)
)
Re: TDD workshop. Anybody welcome; Data storage abstraction.
Let's go on with your code Chris. My code is just the pieces I posted earlier.
@arjan: we should write a test for that I think? And then make it pass.
@arjan: we should write a test for that I think? And then make it pass.
- Chris Corbyn
- Breakbeat Nuttzer
- Posts: 13098
- Joined: Wed Mar 24, 2004 7:57 am
- Location: Melbourne, Australia
Re: TDD workshop. Anybody welcome; Data storage abstraction.
Rightmatthijs wrote:@arjan: we should write a test for that I think? And then make it pass.
Code: Select all
public function testGettingValueOnSecondPrimaryBranch() {
$storageModel = $this->_createStorageModel();
$storageModel->setReturnValue('load', array(
'foo' => array(
'bar' => array(
'zip' => 'some value'
)
),
'bar2' => array(
'zip2' => 'some other value'
)
));
$dataStore = $this->_createDataStore($storageModel);
$this->assertEqual('some other value', $dataStore->get('bar2/zip2'));
}Code: Select all
AllTests.php
OK
Test cases run: 1/1, Passes: 17, Failures: 0, Exceptions: 0