#580 DynamicFactory

Accepted
closed
nobody
master
1
2012-12-17
2012-11-15
John May
No

Patch adds a DynamicFactory to master and implements SilentChemObjectBuilder in terms
of the new factory. The factory is in cdk-core and is testing using a new devellib - mockito.

https://github.com/johnmay/cdk/tree/master+

More in depth description - on mailing lists: http://sourceforge.net/mailarchive/forum.php?thread_name=39DAACCF-99FC-414A-9C5D-5D994B3F269E%40ebi.ac.uk&forum_name=cdk-devel

Discussion

  • John May
    John May
    2012-11-17

    I've include and tested the changes on Debug/Default builders now also

     
  • Okay, I'm starting to look at this. Some initial thoughts:

    1) The "implmentorsOf" method seems to be missing an 'e' :)
    2) Although DynamicFactory simplifies IChemObjectFactories greatly, it is hugely complex itself. That's not a bad thing, but debugging code like this might be a nightmare.
    3) There are an awful lot of inner-classes and interfaces in the DynamicFactory. I'm fine with this in principle (as I assume that they are closely tied to the enclosing class) but again it makes it all quite confusing.

    Next, I'll try using it on an implementation of a matix-backed IAtomContainer.

     
  • John May
    John May
    2012-11-23

    1) Yep will change that.

    2) Hehe, maybe because I wrote it but I think it's much similar :D. It's basically just two maps - and it uses the constructor parameters as keys. There is actually < 300 lines of code. If there was a bug previously it could be in 1 of 3 builders which would have to be checked and debugged separately and then copy pasted across :/.

    3) Yeah fair point on that, the trouble is they are one off classes which only the factory should worry about. The rest of the API shouldn't even know they exists as it doesn't need them - well I guess maybe the factories need to know. It's difficult. I was thinking I might actually just remove the public constructor key and keep that encapsulated.

    Next, I'll try using it on an implementation of a matix-backed IAtomContainer.

    Sound cool, I was looking at the 'org.openscience.cdk.graph' the other day and thinking it would nice to have these as objects opposed to just 2D arrays. I actually use an adjacency list for the hashing now (2nd prototype now - hopefully be able to integrate it soon) which is ridiculously quick for what I needed. Some benchmarks I did on ChEBI looking at spheres of '8' (i.e. like the fingerprint does) the times to iterate over 22000 structures were about ~60 ms (AdjacencyList), ~300 ms (AdjacencyMatrix) and ~3600 ms for the normal AtomContainer. Of course the implementation changes on what the algorithm needs to do.

     
  • John May
    John May
    2012-11-23

    Just looked back at it and I could also remove some interfaces, the trouble is you can't mock 'Class<?>' hence the reason for the InterfaceProvider etc.. Actually a lot of the stuff got added to make the unit testing easier.

     
  • Yeah, an interface called 'InterfaceProvider' was a bit of a headache.

    Well, don't remove anything too hastily. Just if it makes sense. I know that testing (unfortunately) can drive some design decisions so maybe we can live with a few extra inner-interfaces.

     
  • Yeah, I've seen that fail, and already got that patch :)

    Yeah, please do sign of my patch as a formal statement in git history you were happy with it :)

     
  • John May
    John May
    2012-12-13

    Sorry for the delay, was Christmas Lunch day.

    Here is the signed patch.
    J

     
    • Where?

       
  • Applied and pushed.

     
    • status: open --> accepted
    • milestone: Needs_Review --> Accepted
     
    • status: accepted --> closed