Page 8 of 15

Re: TDD workshop. Anybody welcome; Data storage abstraction.

Posted: Sat Apr 26, 2008 9:24 am
by matthijs
I'm busy :)
I'll give it a try first ok? I'm deep into the php manual trying to figure something out :)

Re: TDD workshop. Anybody welcome; Data storage abstraction.

Posted: Sat Apr 26, 2008 9:35 am
by Chris Corbyn
No worries, I'm half asleep myself anyway :) Time to hit the sack!

Re: TDD workshop. Anybody welcome; Data storage abstraction.

Posted: Sat Apr 26, 2008 3:55 pm
by matthijs
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 ;)

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;
    }
 
 
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.

Re: TDD workshop. Anybody welcome; Data storage abstraction.

Posted: Sat Apr 26, 2008 5:54 pm
by Chris Corbyn
matthijs wrote:Maybe we should also write some tests for these helper functions to make sure they do what they are supposed to do :)
No-no. That's an instant red card ;) Never test private methods. We only care that DataStore is doing the correct things at a public API level.

The code doesn't look bad to me :) It can be improved, but it's a really tricky algorithm. Here's what I came up with (Haven't refactored yet). Mine's ugly too.

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'));
  }
New code (looks easy to refactor since it's almost the same logic in each method):

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);
  }
 
}
What I'll suggest is that we try refactoring both and see who's feels cleaner.

Re: TDD workshop. Anybody welcome; Data storage abstraction.

Posted: Sat Apr 26, 2008 6:35 pm
by Chris Corbyn
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.

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;
  }
  
}
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?

Re: TDD workshop. Anybody welcome; Data storage abstraction.

Posted: Sun Apr 27, 2008 1:32 am
by matthijs
Good morning :)

I did not forget to post set() and remove(). I just had not done those yet.
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.
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.

I'll get my coffee and take a good look at your code. Or maybe first try to do the remove and set myself.

Re: TDD workshop. Anybody welcome; Data storage abstraction.

Posted: Sun Apr 27, 2008 2:10 am
by Chris Corbyn
matthijs 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.
Do try and finish those methods yourself :) This part should be good experience. It's a complicated situation. Don't be afraid to backtrack and undo changes in favour of taking a new approach. Don't be intimidated by a cascade of failing tests neither. Read the failures -- in particular the test method they come from -- and then fix them one at a time. You'll know when you're getting somewhere even if you still have failing tests.

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.

Posted: Sun Apr 27, 2008 2:50 am
by matthijs
Sorry, I just assumed they were the result of early optimization ;)
Early desperation you mean? :)

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.

Posted: Sun Apr 27, 2008 2:58 am
by Chris Corbyn
matthijs wrote:
Sorry, I just assumed they were the result of early optimization ;)
Early desperation you mean? :)

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?
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?

Re: TDD workshop. Anybody welcome; Data storage abstraction.

Posted: Sun Apr 27, 2008 3:24 am
by matthijs
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.

Posted: Sun Apr 27, 2008 3:58 am
by matthijs
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?

Re: TDD workshop. Anybody welcome; Data storage abstraction.

Posted: Sun Apr 27, 2008 4:15 am
by Chris Corbyn
matthijs wrote:Maybe later ok? My branch would be a dead end anyway, I'll just experiment and test a bit locally for now.
Ok. We should probably consolidate back down to a single code base again just so we're working on the same thing :) We can probably release the end product too if we wanted to.

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.

Posted: Sun Apr 27, 2008 4:33 am
by arjan.top
chris I think your code is not ok

For example if you have this array:

Code: Select all

 
array(
      'foo' => array(
        'bar' => array(
          'zip' => 'some value'
          )
        ),
        'bar2' => array(
          'zip2' => 'some other value'
        )
      )
 
... and you want foo/bar2/zip2

Re: TDD workshop. Anybody welcome; Data storage abstraction.

Posted: Sun Apr 27, 2008 4:37 am
by matthijs
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.

Re: TDD workshop. Anybody welcome; Data storage abstraction.

Posted: Sun Apr 27, 2008 4:44 am
by Chris Corbyn
matthijs wrote:@arjan: we should write a test for that I think? And then make it pass.
Right :)

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'));
  }
Didn't change my code and it passes:

Code: Select all

AllTests.php
OK
Test cases run: 1/1, Passes: 17, Failures: 0, Exceptions: 0
If I've missed the point you were making please provide me with a test :)