From: Egon W. <e.w...@sc...> - 2007-01-29 12:06:29
|
On Monday 29 January 2007, sh...@us... wrote: > @@ -102,6 +102,8 @@ > if (!(params[0] instanceof Integer) ){ > throw new CDKException("The parameter must be of type > Integer"); } > + if(params.length==0) > + return; > maxIterations = ((Integer) params[0]).intValue(); > } Stefan, I am not sure this is the right way to do this: fail silently when incorrect input has been given. The NPE it did before it not great either, but at least shows that something is wrong in the code of the user of this class. At least I would suggest to throw a CDKException, like: if (params.length!=1) throw new CDKException("Expected one parameter of class Integer."); Or combine them into one if-statement: if (params.length!=1 || !(params[0] instanceof Integer) ){ throw new CDKException("The parameter must be of type Integer"); } There are likely many such incomplete testing in the QSAR descriptors, as everyone copy/pasted code :) Egon -- e.w...@sc... Cologne University Bioinformatics Center (CUBIC) Blog: http://chem-bla-ics.blogspot.com/ GPG: 1024D/D6336BA6 |
From: Egon W. <e.w...@sc...> - 2007-01-29 12:24:10
|
On Monday 29 January 2007, Stefan Kuhn wrote: > The point is (for me) that the parameter is optional. At least the > GasteigerMasilliBla class has a default for it, so I think the descriptor > should use the default as well - that's what my implementation does (I > believe). Yeah, there is a default. There should be, but why are you trying to overwrite the default then by calling the method? The method is supposed to be called only to overwrite the default. Egon -- e.w...@sc... Cologne University Bioinformatics Center (CUBIC) Blog: http://chem-bla-ics.blogspot.com/ GPG: 1024D/D6336BA6 |
From: Stefan K. <ste...@un...> - 2007-01-29 12:31:40
|
Am Monday 29 January 2007 13:21 schrieb Egon Willighagen: > On Monday 29 January 2007, Stefan Kuhn wrote: > > The point is (for me) that the parameter is optional. At least the > > GasteigerMasilliBla class has a default for it, so I think the descri= ptor > > should use the default as well - that's what my implementation does (= I > > believe). > > Yeah, there is a default. There should be, but why are you trying to > overwrite the default then by calling the method? The method is suppose= d to Am I trying? I can't see where. l 140 if(maxIterations !=3D 0) peoe.setMaxGasteigerIters(maxIterations); has an if. > be called only to overwrite the default. > > Egon --=20 Stefan Kuhn B. Sc. M. A. Cologne University BioInformatics Center (http://www.cubic.uni-koeln.de) Z=FClpicher Str. 47, 50674 Cologne Tel: +49(0)221-470-7428 Fax: +49 (0) 221-470-7786 My public PGP key is available at http://pgp.mit.edu |
From: Egon W. <e.w...@sc...> - 2007-01-29 12:35:30
|
On Monday 29 January 2007, Stefan Kuhn wrote: > Am Monday 29 January 2007 13:21 schrieb Egon Willighagen: > > There should be, but why are you trying to > > overwrite the default then by calling the method? > > Am I trying? I can't see where. l 140 > if(maxIterations != 0) > peoe.setMaxGasteigerIters(maxIterations); > has an if. Well, that setParameters() is called somewhere... Otherwise the patch does not have any effect anyway... Egon -- e.w...@sc... Cologne University Bioinformatics Center (CUBIC) Blog: http://chem-bla-ics.blogspot.com/ GPG: 1024D/D6336BA6 |
From: Stefan K. <ste...@un...> - 2007-01-29 12:39:21
|
Am Monday 29 January 2007 13:33 schrieb Egon Willighagen: > On Monday 29 January 2007, Stefan Kuhn wrote: > > Am Monday 29 January 2007 13:21 schrieb Egon Willighagen: > > > There should be, but why are you trying to > > > overwrite the default then by calling the method? > > > > Am I trying? I can't see where. l 140 > > if(maxIterations !=3D 0) > > peoe.setMaxGasteigerIters(maxIterations); > > has an if. > > Well, that setParameters() is called somewhere... Otherwise the patch d= oes > not have any effect anyway... So you say the user should set paramters only if he wants to change somet= hing?=20 That's ok for me as well (please not my patch had another part [a comment= =20 patch], so keep this anyway). > > Egon --=20 Stefan Kuhn B. Sc. M. A. Cologne University BioInformatics Center (http://www.cubic.uni-koeln.de) Z=FClpicher Str. 47, 50674 Cologne Tel: +49(0)221-470-7428 Fax: +49 (0) 221-470-7786 My public PGP key is available at http://pgp.mit.edu |
From: Egon W. <e.w...@sc...> - 2007-01-29 12:43:05
|
On Monday 29 January 2007, Stefan Kuhn wrote: > Am Monday 29 January 2007 13:33 schrieb Egon Willighagen: > > On Monday 29 January 2007, Stefan Kuhn wrote: > > > Am Monday 29 January 2007 13:21 schrieb Egon Willighagen: > > > > There should be, but why are you trying to > > > > overwrite the default then by calling the method? > > > > > > Am I trying? I can't see where. l 140 > > > if(maxIterations !=3D 0) > > > peoe.setMaxGasteigerIters(maxIterations); > > > has an if. > > > > Well, that setParameters() is called somewhere... Otherwise the patch > > does not have any effect anyway... > > So you say the user should set paramters only if he wants to change > something?=20 Correct. > That's ok for me as well (please not my patch had another part=20 > [a comment patch], so keep this anyway). OK, I just rewrite it as: if (params.length!=3D1 || !(params[0] instanceof Integer) ){ =A0=A0=A0=A0=A0=A0=A0=A0throw new CDKException("The parameter must be of ty= pe Integer"); } OK? Egon =2D-=20 e.w...@sc... Cologne University Bioinformatics Center (CUBIC) Blog: http://chem-bla-ics.blogspot.com/ GPG: 1024D/D6336BA6 |
From: Rajarshi G. <rg...@in...> - 2007-01-29 14:28:29
|
On Mon, 2007-01-29 at 13:04 +0100, Egon Willighagen wrote: > On Monday 29 January 2007, sh...@us... wrote: > > @@ -102,6 +102,8 @@ > > if (!(params[0] instanceof Integer) ){ > > throw new CDKException("The parameter must be of type > > Integer"); } > > + if(params.length==0) > > + return; > > maxIterations = ((Integer) params[0]).intValue(); > > } > > Stefan, > > I am not sure this is the right way to do this: fail silently when incorrect > input has been given. I strongly agree - if there is a problem, let the caller know. This was a big issue when writing the descriptor GUI - it was running OK, but I would get invalid numbers or I would have to make extra checks to ensure that I was getting a value at all. At least I can catch an exception and handle it :) > The NPE it did before it not great either, but at least > shows that something is wrong in the code of the user of this class. Agreed. ------------------------------------------------------------------- Rajarshi Guha <rg...@in...> GPG Fingerprint: 0CCA 8EE2 2EEB 25E2 AB04 06F7 1BB9 E634 9B87 56EE ------------------------------------------------------------------- The emacs religion: to be saved, control excess... |