Permalink
Browse files

fix bug #3305550 - moved SMARTSQuery to where used

It was initialised staticly for the outer class. However since the SMARTSQueryTool has state it can not be shared among multiple FingerPrinters. Now every CountSubstructures instance will have it's own which should make it possible to run multiple PubchemFingerprinters in parallel.

Also, made the SMARTSQueryTool throw an IllegelargumentException instead of a checked CDKException if something is wrong with the SMARTS string.

Signed-off-by: Rajarshi  Guha <rajarshi.guha@gmail.com>
  • Loading branch information...
1 parent 4c5beba commit dfb3e8624323f015628cdbe7202530510d458056 @jonalv jonalv committed with rajarshi May 24, 2011
@@ -91,16 +91,7 @@
private byte[] m_bits;
- private static SMARTSQueryTool sqt;
-
public PubchemFingerprinter() {
- try {
- sqt = new SMARTSQueryTool("C");
- } catch (CDKException e) {
- // bad practice but, the above initialization
- // will never fail so we don't bother throwing
- // the exception
- }
m_bits = new byte[(FP_SIZE + 7) >> 3];
}
@@ -308,9 +299,11 @@ public int countUnsaturatedHeteroContainingRing(int size) {
static class CountSubstructures {
private IAtomContainer mol;
+ private SMARTSQueryTool sqt;
public CountSubstructures(IAtomContainer m) {
mol = m;
+ sqt = new SMARTSQueryTool("C");
}
public int countSubstructure(String smarts) throws CDKException {
@@ -127,12 +127,17 @@ public boolean removeEldestEntry(Map.Entry eldest) {
}
};
- public SMARTSQueryTool(String smarts) throws CDKException {
+ /**
+ * @param smarts
+ *
+ * @throws IllegalArgumentException if the SMARTS string can not be handled
+ */
+ public SMARTSQueryTool(String smarts) {
this.smarts = smarts;
try {
initializeQuery();
} catch (TokenMgrError error) {
- throw new CDKException("Error parsing SMARTS", error);
+ throw new IllegalArgumentException("Error parsing SMARTS", error);
@egonw

egonw May 24, 2011

Owner

Does the TokenMgrError have a reasonable error message? If so, I rather see this changed to:

throw new IllegalArgumentException(
"Error parsing SMARTS: " + error.getMessage(),
error
);

@jonalv

jonalv May 24, 2011

Contributor

Since the error is given to the new Exceptions constructor as cause I don't think it is so important. I also question the pattern of concatenating error messages from the stack trace to generate a huge message string. I think that is what the stack trace is for... However I guess if that is the "CDK way" I guess someone could do it...

}
}
@@ -475,7 +480,7 @@ private void initializeRecursiveSmartsAtom(IAtom atom, IAtomContainer atomContai
}
}
- private void initializeQuery() throws CDKException {
+ private void initializeQuery() {
matchingAtoms = null;
query = cache.get(smarts);
if (query == null) {

0 comments on commit dfb3e86

Please sign in to comment.