[Cdk-devel] [ cdk-Patches-3306775 ] fix for bug 3305550 - Pubchemfingerprinter is thread incompa

[SourceForge.net <no...@so...>]

Patches item #3306775, was opened at 2011-05-24 09:54
Message generated for change (Comment added) made by jonalv
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3306775&group_id=20024

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: master
Group: Needs Review
Status: Open
Resolution: None
Priority: 5
Private: No
Submitted By: Jonathan Alvarsson (jonalv)
Assigned to: Rajarshi Guha (rajarshi)
Summary: fix for bug 3305550 - Pubchemfingerprinter is thread incompa

Initial Comment:
See attached patch

----------------------------------------------------------------------

>Comment By: Jonathan Alvarsson (jonalv)
Date: 2011-05-24 14:03

Message:
I don't agree. The Argument of that constructor is an IAtomContainer and
there is nothing wrong with it. The exception would be because of something
being broken already when the constructor was called. Hence an
IllegalState. In this case that state would for example be that the hard
coded SMARTS String is wrong. It's not an argument to the method that
throws the Exception, it's a state which the caller can not effect. (Then
there of course is the issue with CDKException: 1: It should be removed all
together. 2: If not so, it should at least be an unchecked Exception. But
that fight is for another day I guess... :)

----------------------------------------------------------------------

Comment By: Rajarshi Guha (rajarshi)
Date: 2011-05-24 13:49

Message:
I think it's OK - however I'd prefer that IllegalArgumentException be
thrown instead, since the the reason for an exception at that point is an
invalid SMARTS.

Otherwise good to go

----------------------------------------------------------------------

Comment By: Egon Willighagen (egonw)
Date: 2011-05-24 11:15

Message:
Looks good to me, and I like the throwing of an IllegalStateException.

I am not sure about the effect of moving the instantiation into this
place. Rajarshi, please judge that bit.

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=320024&aid=3306775&group_id=20024