Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use interfaces (and builders) instead of implementations #187

Closed
wants to merge 4 commits into from

Conversation

egonw
Copy link
Member

@egonw egonw commented Dec 28, 2015

No description provided.

@johnmay
Copy link
Member

johnmay commented Dec 28, 2015

MoleculeFactory was replaced with TestMoleculeFactory.... I couldn't remove MoleculeFactory so stuck it in misc.

@egonw
Copy link
Member Author

egonw commented Dec 28, 2015

[MoleculeFactory]... mmm, too bad there was not a @deprecated :/

@johnmay
Copy link
Member

johnmay commented Dec 28, 2015

misc is effectively don't use this... :p like the IUPAC parser...

@johnmay
Copy link
Member

johnmay commented Dec 28, 2015

API change on MoleculeFactory... not sure how widely used it is by outside the main library but we should have a stable API.

@egonw
Copy link
Member Author

egonw commented Dec 28, 2015

"misc is effectively don't use this" --> huh? I'd say log4j, diff at least are very much meant to be used! under the right conditions... and extra certainly has important functionality, and so does control (for Bioclipse, but also JChemPaint, though they forked that)

@egonw
Copy link
Member Author

egonw commented Dec 28, 2015

"API change on MoleculeFactory" -> to me, not depending on interfaces but on implementations is a bug.

@egonw
Copy link
Member Author

egonw commented Dec 28, 2015

BTW, while I disagree about using 'misc'... for MoleculeFactory I do agree... that should not really be used. As far as I know, it only really existed to aid the tests... it seems to me that when TestMoleculeFactory got introduced, not all tests started using it, because these patches actually update many test-* classes...

So, shall I just remove the MoleculeFactory patch then?

@johnmay
Copy link
Member

johnmay commented Dec 28, 2015

Need too look more into it, but yes essentially all test usages should be TestMoleculeFactory... I think there was some usage outside of tests though (but only in misc)?

Disagree on the interfaces comment by the way, it's not black and white. If there's only one implementation it's perfectly reasonable to use the concrete instances. With the molecules it's very confusing to new comers the whole data/silent usage as the wrong one is the default :-).

@johnmay
Copy link
Member

johnmay commented Dec 28, 2015

My mistake on misc, I thought that was the module... I meant 'extra'. It's not that it's not useful it's that it doesn't fit and hence may be out of scope.

@egonw
Copy link
Member Author

egonw commented Dec 28, 2015

["the wrong one is the default"]... well, for long JChemPaint was a key users which critically depended on this functionality... the whole idea of interfaces is to actually not being forced to use this "wrong one"! That's why I'm trying to make everything not depending on them... actually, the cdk-silent is "wrong" too... I still think the implementation can yet be a factor faster... why not make an implementation that uses a connection matrix internally, and not use List at all?

@egonw
Copy link
Member Author

egonw commented Dec 28, 2015

The history of the "extra" module is actually that *everything" is in extra... from that starting point, I started putting stuff in other modules :) There is still stuff in "extra" that should probably go somewhere else...

@johnmay
Copy link
Member

johnmay commented Dec 31, 2015

I'll make a patch that moves all usage of MoleculeFactory over to TestMoleculeFactory... otherwise API changes need addressing.

@egonw
Copy link
Member Author

egonw commented Dec 31, 2015

Sounds good.

@johnmay johnmay mentioned this pull request May 1, 2017
@egonw
Copy link
Member Author

egonw commented May 14, 2017

(I may redo some of this at some later point, but likely just from scratch...)

@egonw egonw closed this May 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants