Menu
▾
▴
#580 DynamicFactory
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
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.
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.
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.
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.
Patch for 1. John, if you agree with that patch, I can push the rest to master.
Yep that's fine. Do you want me to sign it.
Also there was a failing unit test that needed correction after the intial patches.
Here is the commit on master+
https://github.com/johnmay/cdk/commit/0ebbf3a7c85339fc277d54a9cb2e7ddb6a2e9189
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 :)
Sorry for the delay, was Christmas Lunch day.
Here is the signed patch.
J
Where?
Damn it didn't attached....
Applied and pushed.