From: M.Le_maudit <mle...@us...> - 2004-01-07 13:00:48
|
Hello. Maybe these are stupid questions, but... I'll ask them anyway. ;-) (Remember I'm just a beginner in Java development.) I'm talking about the "CDKConstants" class that is part of the "org.openscience.cdk" package. 1- If I remember well, the "final" access specifier avoids modifying the attribute's value at runtime. Isn't this supposed to be necessary for a constant ? Some attributes of this class are declared without the "final" access specifier, but are commented as if they were constants. (See NMR constants and Flag constants.) Are they "real" constants or not ? - If they're not... Well, Is the class name is an appropriate one ? - If they are "real" constants, then why the "final" specifier isn't used for them ? May this cause a bug ? 2- As this class only contains constant definitions (referring to its name), why is it declared as a "class" and not as an "interface" ? There must be a good reason for that, but I don't see which one. Thanks for any enlightenment about this. ;-) Yours. M. |
From: <eg...@sc...> - 2004-01-07 13:13:53
|
Quoting "M.Le_maudit" <mle...@us...>: > > Maybe these are stupid questions, but... I'll ask them anyway. ;-) > (Remember I'm just a beginner in Java development.) > > I'm talking about the "CDKConstants" class that is part of the > "org.openscience.cdk" package. > > 1- If I remember well, the "final" access specifier avoids modifying the > attribute's value at > runtime. > Isn't this supposed to be necessary for a constant ? > > Some attributes of this class are declared without the "final" access > specifier, but are commented > as if they were constants. (See NMR constants and Flag constants.) > Are they "real" constants or not ? > - If they're not... Well, Is the class name is an appropriate one ? > - If they are "real" constants, then why the "final" specifier isn't used for > them ? May this cause > a bug ? I guess they all should be 'final'... > 2- As this class only contains constant definitions (referring to its name), > why is it declared as a > "class" and not as an "interface" ? There must be a good reason for that, but > I don't see which one. Why should it be an interface? Personally, I prefer to have code like CDKConstants.X instead of 'implements CDKConstants' in the class definition... Egon |
From: Anatoli K. <ana...@in...> - 2004-01-07 13:24:28
|
Hello, All the variables defined in any interface are public, static and final by definition and marked as such in bytecode. Stating it explicitly in source code is a matter of taste, but makes absolutely no difference at runtime. > Personally, I prefer to have code like CDKConstants.X instead of > 'implements CDKConstants' in the class definition... Egon is qute right. This is standard recommended pattern for dealing with constants in Java. Cheers, Toly -----Original Message----- From: cdk...@li... [mailto:cdk...@li...]On Behalf Of eg...@sc... Sent: 07 January 2004 13:14 To: M.Le_maudit Cc: cdk...@li... Subject: Re: [Cdk-devel] Questions about "CDKConstants" class. Quoting "M.Le_maudit" <mle...@us...>: > > Maybe these are stupid questions, but... I'll ask them anyway. ;-) > (Remember I'm just a beginner in Java development.) > > I'm talking about the "CDKConstants" class that is part of the > "org.openscience.cdk" package. > > 1- If I remember well, the "final" access specifier avoids modifying the > attribute's value at > runtime. > Isn't this supposed to be necessary for a constant ? > > Some attributes of this class are declared without the "final" access > specifier, but are commented > as if they were constants. (See NMR constants and Flag constants.) > Are they "real" constants or not ? > - If they're not... Well, Is the class name is an appropriate one ? > - If they are "real" constants, then why the "final" specifier isn't used for > them ? May this cause > a bug ? I guess they all should be 'final'... > 2- As this class only contains constant definitions (referring to its name), > why is it declared as a > "class" and not as an "interface" ? There must be a good reason for that, but > I don't see which one. Why should it be an interface? Personally, I prefer to have code like CDKConstants.X instead of 'implements CDKConstants' in the class definition... Egon ------------------------------------------------------- This SF.net email is sponsored by: IBM Linux Tutorials. Become an expert in LINUX or just sharpen your skills. Sign up for IBM's Free Linux Tutorials. Learn everything from the bash shell to sys admin. Click now! http://ads.osdn.com/?ad_id=1278&alloc_id=3371&op=click _______________________________________________ Cdk-devel mailing list Cdk...@li... https://lists.sourceforge.net/lists/listinfo/cdk-devel |
From: Miguel H. <mt...@mt...> - 2004-01-07 14:21:44
|
> All the variables defined in any interface are public, static and > final by > definition and marked as such in bytecode. Very interesting point ... I did not know that. Miguel |
From: Anatoli K. <ana...@in...> - 2004-01-07 14:48:35
|
Hello Miguel, Here is a simple proof, :-). Write a simple java interface class as shown below. Note that I do not bother with any specifiers at all for my variable. public interface a { String variable = "XYZ"; } Compile the interface and then take a look at the generated byte code. You will see something like that: public interface a { public static final String variable = "XYZ"; } Now, in our company we DO place specifiers into interface as a matter of policy, so I do not want to encourage bad practices. Nevertheless, it is not a big deal. On a separate issue, I really hate something like this public class MyClass implements XYZConstants Referring to interface constants explicitly is much better for clarity, i.e. public class MyClass { .......... XYZConstants.NAME; Cheers, Toly -----Original Message----- From: cdk...@li... [mailto:cdk...@li...]On Behalf Of Miguel Howard Sent: 07 January 2004 14:22 To: ana...@in... Cc: cdk...@li... Subject: RE: [Cdk-devel] Questions about 'CDKConstants' class. > All the variables defined in any interface are public, static and > final by > definition and marked as such in bytecode. Very interesting point ... I did not know that. Miguel ------------------------------------------------------- This SF.net email is sponsored by: IBM Linux Tutorials. Become an expert in LINUX or just sharpen your skills. Sign up for IBM's Free Linux Tutorials. Learn everything from the bash shell to sys admin. Click now! http://ads.osdn.com/?ad_id=1278&alloc_id=3371&op=click _______________________________________________ Cdk-devel mailing list Cdk...@li... https://lists.sourceforge.net/lists/listinfo/cdk-devel |
From: Anatoli K. <ana...@in...> - 2004-01-26 20:15:48
|
Hello, I believe the general issue of infinite loops should be given a very = high priority. Presently, it is relatively easy to cause CDK to enter = infinite loop while doing relatively simple I/O and layout operations. This is an extremely serious, almost fatal issue from the point of view = of using CDK as a library. The severity of infinite loop is an order of magnitude higher then any other bug or issue, even outright VM crash. VM crash at its worst could take down a server - infinite loop in a wrong server process could take down the whole local network. Unfortunately, presently it means that CDK in the form it is distributed cannot be used in serious server environment without custom = modifications to the codebase to insert custom safety checks. I have attempted to identify and possibly fix the bugs - unfortunately unsuccessfully. I know roughly the blocks of code responsible, but = cannot clearly identify the precise problems causing it. And being fair, they = are one of the most complicated pieces of functionality in CDK. Proposals: 1) Add safety checks on all pieces of code which are known to cause = infinite loops. 2) Write a test harness, which will run through the whole NCI dataset, parsing every single SMILES string and executing layout on it. 3) Identify and cap all the loops with open-ended finish conditions - a standard cause of infinite loops. Rewrite the implementation to avoid open-ended finish conditions or put safety-valves in. For (1) one of the principal culpits is the layout loop inside StructureDiagramGenerator.generateCoordinates() do=20 { ................... } while(!atomPlacer.allPlaced(molecule)); should be modified to something like int safetyCounter =3D <N atoms in molecule>; // if we cannot place at = least one atom in every layout loop, then something is very wrong. do=20 { safetyCounter --; ................... } while((!atomPlacer.allPlaced(molecule)) && (safetyCounter >0)); Cheers, Toly |
From: Christoph S. <c.s...@un...> - 2004-01-27 10:33:43
|
That's perfectly correct. For now, I have just prevented people from using SDG with disconnected=20 molecules. But your point regarding infinite loops takeing seriously and=20 will be looked at as soon as I have time again. Cheers, Chris --=20 Dr. rer. nat. habil. Christoph Steinbeck (c.s...@un...) Groupleader Junior Research Group for Applied Bioinformatics Cologne University BioInformatics Center (http://www.cubic.uni-koeln.de) Z=FClpicher Str. 47, 50674 Cologne Tel: +49(0)221-470-7426 Fax: +49 (0) 221-470-7786 What is man but that lofty spirit - that sense of enterprise. ... Kirk, "I, Mudd," stardate 4513.3.. Anatoli Krassavine wrote: > Hello, >=20 > I believe the general issue of infinite loops should be given a very hi= gh > priority. Presently, it is relatively easy to cause CDK to enter infini= te > loop while doing relatively simple I/O and layout operations. >=20 > This is an extremely serious, almost fatal issue from the point of view= of > using CDK as a library. The severity of infinite loop is an order of > magnitude higher then any other bug or issue, even outright VM crash. V= M > crash at its worst could take down a server - infinite loop in a wrong > server process could take down the whole local network. >=20 > Unfortunately, presently it means that CDK in the form it is distribute= d > cannot be used in serious server environment without custom modificatio= ns to > the codebase to insert custom safety checks. >=20 > I have attempted to identify and possibly fix the bugs - unfortunately > unsuccessfully. I know roughly the blocks of code responsible, but cann= ot > clearly identify the precise problems causing it. And being fair, they = are > one of the most complicated pieces of functionality in CDK. >=20 > Proposals: > 1) Add safety checks on all pieces of code which are known to cause inf= inite > loops. > 2) Write a test harness, which will run through the whole NCI dataset, > parsing every single SMILES string and executing layout on it. > 3) Identify and cap all the loops with open-ended finish conditions - a > standard cause of infinite loops. Rewrite the implementation to avoid > open-ended finish conditions or put safety-valves in. >=20 > For (1) one of the principal culpits is the layout loop inside > StructureDiagramGenerator.generateCoordinates() >=20 > do=20 > { > ................... > } while(!atomPlacer.allPlaced(molecule)); >=20 > should be modified to something like >=20 > int safetyCounter =3D <N atoms in molecule>; // if we cannot place at l= east > one atom in every layout loop, then something is very wrong. >=20 > do=20 > { > safetyCounter --; > ................... > } while((!atomPlacer.allPlaced(molecule)) && (safetyCounter >0)); >=20 > Cheers, > Toly |
From: Egon W. <eg...@sc...> - 2004-01-26 20:51:50
|
On Monday 26 January 2004 21:15, Anatoli Krassavine wrote: > I believe the general issue of infinite loops should be given a very high > priority. Presently, it is relatively easy to cause CDK to enter infinite > loop while doing relatively simple I/O and layout operations. > > This is an extremely serious, almost fatal issue from the point of view of > using CDK as a library. The severity of infinite loop is an order of > magnitude higher then any other bug or issue, even outright VM crash. VM > crash at its worst could take down a server - infinite loop in a wrong > server process could take down the whole local network. Good point. > Unfortunately, presently it means that CDK in the form it is distributed > cannot be used in serious server environment without custom modifications > to the codebase to insert custom safety checks. You might be quite right there... note that CDK *is* used on at least the NMRShiftDB server... but I tend to agree... Another problem with these infinate loop bugs, is that it is impossible to make a JUnit test for it... because it will hang the whole testing... unless one would use a thread that would stop it after some time... > I have attempted to identify and possibly fix the bugs - unfortunately > unsuccessfully. I know roughly the blocks of code responsible, but cannot > clearly identify the precise problems causing it. Please send the block you did identify as the cause... I will look at it asap... > And being fair, they are > one of the most complicated pieces of functionality in CDK. Indeed, and therefore very hard to maintain and extend... > Proposals: > 1) Add safety checks on all pieces of code which are known to cause > infinite loops. > 2) Write a test harness, which will run through the whole NCI dataset, > parsing every single SMILES string and executing layout on it. Yes, that's something I wanted to do for some time now... > 3) Identify and cap all the loops with open-ended finish conditions - a > standard cause of infinite loops. Rewrite the implementation to avoid > open-ended finish conditions or put safety-valves in. Toly, thanx very much for writing this down. I'll see if I can find some time to track down (and fix) the problem... to have it at least fail, instead of loop endlessly... Egon |
From: Anatoli K. <ana...@in...> - 2004-01-27 12:00:10
|
Hello Egon, Thank you for correctly understanding my concern: CDK is becoming a = standard small chemistry toolkit and as such robustness and reliability are = becoming as important as features, :-). Below are my observations and more concrete proposals. --------------------------------------- The principal source for layout infinite loops appear to be inside StructureDiagramGenerator.generateCoordinates() do=20 { ................... } while(!atomPlacer.allPlaced(molecule)); The open-ended condition at the bottom means that if the layout engine = fails to place even a single atom, then layout engine enters infinite loop. I = do not believe it is possible to guarantee the any given structure could be successfully layouted. Therefore, there should be a deterministic = "safety valve" condition. For my purposes, I usually limit the number of iterations to number of atoms. I am not 100% sure this is entirely correct in all case - but it appears to work.=20 int safetyCounter =3D molecule.getAtomCount();=20 do=20 { safetyCounter --; ................... } while((!atomPlacer.allPlaced(molecule)) && (safetyCounter >0)); if (safetyCounter <=3D 0) { throw new RuntimeException("Layout failed"); } The rationale is simple - if we cannot place at least one atom in every iteration loop, then something is going very-very wrong. This is NOT a fix, nevertheless it is a sensible precaution and I would prefer to have it (or equivalent) kept in CDK codebase. ---------------------------------- Now my observations, which might or might not be relevant to the issue. = The observations below do not represent the only way to cause infinite loop, = but this is the most straightforward to reproduce. 1) CDK layout engine seems to have problems with disconnected structures 2) if the structure is disconnected AND it contains only aliphatics, = then layout fails with NullPointerException (see the relevant bug I reported) 3) if the structure is disconnected AND it contains fused rings in both fragments, then we go into infinite loop (see the relevant bug I = reported) 4) therefore I believe that rings layout algorithm incorrectly goes = through the disconnected graph - probably it starts in a single fragments and = never manages to "escape" it once layout of complete and handle the next = fragment. 5) this is consistent with infinite loop manifestation - it never lays = out the rings in the second fragment so atomPlacer.allPlaced() is always = false 6) this is all the more frustrating since the layout correctly lays out = the individual fragments ---------------------------------- In terms of testing I had experience solving similar problems - = confirming that a layout algorithm never enters infinite loop. Assuming that there going to be relatively few "problem" structures, the best course of = action might be: iterate through a big heterogeneous dataset (NCI is very good) for each entry starts a test in new thread synchronously - wait until completed 1) write a log message into log file and FLUSH logging output = immediately 2) parse SMILES 3) layout structure 4) run few checks 5) [optional] convert back and forth between different formats several = times and check that the data do not get corrupted 6) write a log message into log file and FLUSH logging output = immediately Have a Thread Killer running in parallel which will kill any testing = thread which has run for longer then it should - write appropriate message into = log if this happens and proceed to the next record. Even if we enter infinite loop - we have a record about it. This is also a very useful test to boost your confidence about CDK robustness and identify memory leaks. If CDK survives = parsing/layout/format conversion on 250,000 different structures - it is a very good record of reliability. Cheers, Toly -----Original Message----- From: Egon Willighagen [mailto:eg...@sc...]=20 Sent: 26 January 2004 22:49 To: Anatoli Krassavine; cdk...@li... Subject: Re: [Cdk-devel] Infinite loops On Monday 26 January 2004 21:15, Anatoli Krassavine wrote: > I believe the general issue of infinite loops should be given a very = high > priority. Presently, it is relatively easy to cause CDK to enter = infinite > loop while doing relatively simple I/O and layout operations. > > This is an extremely serious, almost fatal issue from the point of = view of > using CDK as a library. The severity of infinite loop is an order of > magnitude higher then any other bug or issue, even outright VM crash. = VM > crash at its worst could take down a server - infinite loop in a wrong > server process could take down the whole local network. Good point. > Unfortunately, presently it means that CDK in the form it is = distributed > cannot be used in serious server environment without custom = modifications > to the codebase to insert custom safety checks. You might be quite right there... note that CDK *is* used on at least = the=20 NMRShiftDB server... but I tend to agree... Another problem with these infinate loop bugs, is that it is impossible = to=20 make a JUnit test for it... because it will hang the whole testing... = unless one would use a thread that would stop it after some time... > I have attempted to identify and possibly fix the bugs - unfortunately > unsuccessfully. I know roughly the blocks of code responsible, but = cannot > clearly identify the precise problems causing it. Please send the block you did identify as the cause... I will look at it = asap... > And being fair, they are > one of the most complicated pieces of functionality in CDK. Indeed, and therefore very hard to maintain and extend... > Proposals: > 1) Add safety checks on all pieces of code which are known to cause > infinite loops. > 2) Write a test harness, which will run through the whole NCI dataset, > parsing every single SMILES string and executing layout on it. Yes, that's something I wanted to do for some time now... > 3) Identify and cap all the loops with open-ended finish conditions - = a > standard cause of infinite loops. Rewrite the implementation to avoid > open-ended finish conditions or put safety-valves in. Toly, thanx very much for writing this down. I'll see if I can find some time = to=20 track down (and fix) the problem... to have it at least fail, instead of loop=20 endlessly... Egon |
From: Dr. C. S. <ste...@ic...> - 2004-01-27 20:43:14
|
Toly, thanks a lot for your thoughtful comments. I will try to solve this problem soon, since the SDG is my special baby. Unfortunately, on my side the code now even fails for our *connected* aliphatic test case for which it used to work. This is quite strange indeed. First, I guess, we should incorporate your count-down patch, because it really seems quite reasonable. :-) Cheers, Chris -- Dr. rer. nat. habil. Christoph Steinbeck (c.s...@un...) Groupleader Junior Research Group for Applied Bioinformatics Cologne University BioInformatics Center (http://www.cubic.uni-koeln.de) Zülpicher Str. 47, 50674 Cologne Tel: +49(0)221-470-7426 Fax: +49 (0) 221-470-7786 What is man but that lofty spirit - that sense of enterprise. ... Kirk, "I, Mudd," stardate 4513.3.. > Below are my observations and more concrete proposals. > > --------------------------------------- > > The principal source for layout infinite loops appear to be inside > > StructureDiagramGenerator.generateCoordinates() > do > { > ................... > } while(!atomPlacer.allPlaced(molecule)); > > The open-ended condition at the bottom means that if the layout engine fails > to place even a single atom, then layout engine enters infinite loop. I do > not believe it is possible to guarantee the any given structure could be > successfully layouted. Therefore, there should be a deterministic "safety > valve" condition. > > For my purposes, I usually limit the number of iterations to number of > atoms. I am not 100% sure this is entirely correct in all case - but it > appears to work. > > int safetyCounter = molecule.getAtomCount(); > > do > { > safetyCounter --; > ................... > } while((!atomPlacer.allPlaced(molecule)) && (safetyCounter >0)); > > if (safetyCounter <= 0) > { > throw new RuntimeException("Layout failed"); > } > > The rationale is simple - if we cannot place at least one atom in every > iteration loop, then something is going very-very wrong. > > This is NOT a fix, nevertheless it is a sensible precaution and I would > prefer to have it (or equivalent) kept in CDK codebase. > > ---------------------------------- > > Now my observations, which might or might not be relevant to the issue. The > observations below do not represent the only way to cause infinite loop, but > this is the most straightforward to reproduce. > > 1) CDK layout engine seems to have problems with disconnected structures > 2) if the structure is disconnected AND it contains only aliphatics, then > layout fails with NullPointerException (see the relevant bug I reported) > 3) if the structure is disconnected AND it contains fused rings in both > fragments, then we go into infinite loop (see the relevant bug I reported) > 4) therefore I believe that rings layout algorithm incorrectly goes through > the disconnected graph - probably it starts in a single fragments and never > manages to "escape" it once layout of complete and handle the next fragment. > 5) this is consistent with infinite loop manifestation - it never lays out > the rings in the second fragment so atomPlacer.allPlaced() is always false > 6) this is all the more frustrating since the layout correctly lays out the > individual fragments > > ---------------------------------- > > In terms of testing I had experience solving similar problems - confirming > that a layout algorithm never enters infinite loop. Assuming that there > going to be relatively few "problem" structures, the best course of action > might be: > > iterate through a big heterogeneous dataset (NCI is very good) > for each entry starts a test in new thread synchronously - wait until > completed > 1) write a log message into log file and FLUSH logging output immediately > 2) parse SMILES > 3) layout structure > 4) run few checks > 5) [optional] convert back and forth between different formats several times > and check that the data do not get corrupted > 6) write a log message into log file and FLUSH logging output immediately > > Have a Thread Killer running in parallel which will kill any testing thread > which has run for longer then it should - write appropriate message into log > if this happens and proceed to the next record. > > Even if we enter infinite loop - we have a record about it. > > This is also a very useful test to boost your confidence about CDK > robustness and identify memory leaks. If CDK survives parsing/layout/format > conversion on 250,000 different structures - it is a very good record of > reliability. > > Cheers, > Toly > > -----Original Message----- > From: Egon Willighagen [mailto:eg...@sc...] > Sent: 26 January 2004 22:49 > To: Anatoli Krassavine; cdk...@li... > Subject: Re: [Cdk-devel] Infinite loops > > On Monday 26 January 2004 21:15, Anatoli Krassavine wrote: > >>I believe the general issue of infinite loops should be given a very high >>priority. Presently, it is relatively easy to cause CDK to enter infinite >>loop while doing relatively simple I/O and layout operations. >> >>This is an extremely serious, almost fatal issue from the point of view of >>using CDK as a library. The severity of infinite loop is an order of >>magnitude higher then any other bug or issue, even outright VM crash. VM >>crash at its worst could take down a server - infinite loop in a wrong >>server process could take down the whole local network. > > > Good point. > > >>Unfortunately, presently it means that CDK in the form it is distributed >>cannot be used in serious server environment without custom modifications >>to the codebase to insert custom safety checks. > > > You might be quite right there... note that CDK *is* used on at least the > NMRShiftDB server... but I tend to agree... > > Another problem with these infinate loop bugs, is that it is impossible to > make a JUnit test for it... because it will hang the whole testing... unless > > one would use a thread that would stop it after some time... > > >>I have attempted to identify and possibly fix the bugs - unfortunately >>unsuccessfully. I know roughly the blocks of code responsible, but cannot >>clearly identify the precise problems causing it. > > > Please send the block you did identify as the cause... I will look at it > asap... > > >>And being fair, they are >>one of the most complicated pieces of functionality in CDK. > > > Indeed, and therefore very hard to maintain and extend... > > >>Proposals: >>1) Add safety checks on all pieces of code which are known to cause >>infinite loops. >>2) Write a test harness, which will run through the whole NCI dataset, >>parsing every single SMILES string and executing layout on it. > > > Yes, that's something I wanted to do for some time now... > > >>3) Identify and cap all the loops with open-ended finish conditions - a >>standard cause of infinite loops. Rewrite the implementation to avoid >>open-ended finish conditions or put safety-valves in. > > > Toly, > > thanx very much for writing this down. I'll see if I can find some time to > track down (and fix) the problem... to have it at least fail, instead of > loop > endlessly... > > Egon > > > > > > ------------------------------------------------------- > The SF.Net email is sponsored by EclipseCon 2004 > Premiere Conference on Open Tools Development and Integration > See the breadth of Eclipse activity. February 3-5 in Anaheim, CA. > http://www.eclipsecon.org/osdn > _______________________________________________ > Cdk-devel mailing list > Cdk...@li... > https://lists.sourceforge.net/lists/listinfo/cdk-devel > > |
From: Christoph S. <c.s...@un...> - 2004-01-07 13:36:37
|
>>Some attributes of this class are declared without the "final" access >>specifier, but are commented >>as if they were constants. (See NMR constants and Flag constants.) >>Are they "real" constants or not ? >>- If they're not... Well, Is the class name is an appropriate one ? >>- If they are "real" constants, then why the "final" specifier isn't us= ed for >>them ? May this cause >>a bug ? >=20 >=20 > I guess they all should be 'final'... Yes, they should. And with respect to M.'s question, not having them=20 final will not cause any bug. It will just allow modification, which one=20 might or might not want to prevent. >>2- As this class only contains constant definitions (referring to its n= ame), >>why is it declared as a >>"class" and not as an "interface" ? There must be a good reason for tha= t, but >>I don't see which one. Some things in CDK are done without a good reason :-) However, CDKConstants was an interface for a long time (I created it)=20 and then there was a riot against this design, because the whole CDK=20 code was messed up with "implements CDKConstants". I didn't really care=20 how it was done so we changed it. Cheers, Chris --=20 Dr. Christoph Steinbeck (e-mail: c.s...@un...) Groupleader Junior Research Group for Applied Bioinformatics Cologne University BioInformatics Center (http://www.cubic.uni-koeln.de) Z=FClpicher Str. 47, 50674 Cologne Tel: +49(0)221-470-7426 Fax: +49 (0) 221-470-7786 What is man but that lofty spirit - that sense of enterprise. ... Kirk, "I, Mudd," stardate 4513.3.. |
From: M.Le_maudit <mle...@us...> - 2004-01-10 21:35:01
|
Hello Christoph. > On Wednesday, January 07, 2004 2:38 PM, you wrote : > [snip] (about "final" specifier.) > > It will just allow modification, which one might or might not want to > prevent. That was what I wanted to know. (If they are not final, modification is allowed.) > [snip] And with respect to M.'s question, not having them final will not > cause any bug. [snip] If someone accidentally modifies the constant value, wouldn't this make the application behave an unexpected way ? (The constant value is not the expected one.) Isn't this a definition of a bug ? (Here, I'm talking about unexpected behaviour.) > Some things in CDK are done without a good reason :-) > > However, CDKConstants was an interface for a long time (I created it) > and then there was a riot against this design, because the whole CDK > code was messed up with "implements CDKConstants". I didn't really care > how it was done so we changed it. Oh, ... Ok. Yours. M. |
From: Miguel H. <mt...@mt...> - 2004-01-07 14:08:44
|
> I guess they all should be 'final'... I agree >> 2- As this class only contains constant definitions (referring to its >> name), why is it declared as a >> "class" and not as an "interface" ? There must be a good reason for >> that, but I don't see which one. > > Why should it be an interface? > Personally, I prefer to have code like CDKConstants.X instead of > 'implements CDKConstants' in the class definition... I think this is a question of java programming style and personal preference. (see additional notes below) But, I would recommend that the style be consistent within all of CDK. That is, either move everything over to the 'interface' style or keep it as a class. (If you keep it as a class then you should probably make the constants class 'final') Miguel For those who are not familiar with this, one of the 'idioms' of java programmming is declaring your constants in an interface. This allows all classes to 'implement' the interface. That way you don't have to prefix all the uses of constants with the class name. It makes no difference in runtime behavior or performance ... it just eliminates some typing. As in: interface MyConstants { final static int CONSTANT_A = 1234; final static int CONSTANT_B = 2468; } class Foo implements MyConstants { int bar = CONSTANT_A; ... } class Baz extends Biz implements MyConstants, OtherInterface { int blech = CONSTANT_A + CONSTANT_B; } |
From: Peter Murray-R. <pm...@ca...> - 2004-01-07 18:23:09
|
At 15:08 07/01/2004 +0100, Miguel Howard wrote: > > I guess they all should be 'final'... >I agree > > >> 2- As this class only contains constant definitions (referring to its > >> name), why is it declared as a > >> "class" and not as an "interface" ? There must be a good reason for > >> that, but I don't see which one. > > > > Why should it be an interface? > > Personally, I prefer to have code like CDKConstants.X instead of > > 'implements CDKConstants' in the class definition... > <snip/> >For those who are not familiar with this, one of the 'idioms' of java >programmming is declaring your constants in an interface. This allows all >classes to 'implement' the interface. That way you don't have to prefix >all the uses of constants with the class name. > >It makes no difference in runtime behavior or performance ... it just >eliminates some typing. > >As in: > >interface MyConstants { > final static int CONSTANT_A = 1234; > final static int CONSTANT_B = 2468; >} > >class Foo implements MyConstants { > int bar = CONSTANT_A; > >... If you have already decided to use an interface-based approach then it is natural to put the constants in the interface and not the implementation. A good example of this strategy is the XML-DOM classes (Java 1.4) where every class implements a corresponding interface (Element <... ElementImpl, etc.) Class-specific constants (e.g. symbolic strings) are then defined in the interface I have followed that pattern in developing CML DOM classes. I am not sure whether it's a good idea or not. The merits of interfaces seem to be most important when there is the possibility that more than one implementation may be developed and the user can choose between parsers, DOMs, serializers, etc without rewriting their code. (The different implementations are usually selected by classnames fed to Factory classes). I have also adopted this approach for my own molecular editing software where I define an interface which can be implemented by different libraries. At present much of this is done by CDK lib :-) P. Peter Murray-Rust Unilever Centre for Molecular Informatics Chemistry Department, Cambridge University Lensfield Road, CAMBRIDGE, CB2 1EW, UK Tel: +44-1223-763069 |
From: M.Le_maudit <mle...@us...> - 2004-01-10 21:35:16
|
Hello Egon. Hello everyone. First of all, thank you everyone for all these interesting enlightenments. > On January 07, 2004 2:13 PM Egon wrote : > [snip] > I guess they all should be 'final'... I think so. > Why should it be an interface? > Personally, I prefer to have code like CDKConstants.X instead of > 'implements CDKConstants' in the class definition... Actually my question should have been more acurate (*). The fact is that the class comment confused me : [quote (CDK Constants class comment)] An interface providing predefined values for a number of parameters used throughout the CDK. Classes using these Constants should implement this interface. [/quote] So I wanted to know which one of the class declaration and the class comment was up-to-date. Moreover, I thought it was a good opportunity to ask the Team's opinion about class/interface usage, but maybe I did this in a clumsy way (*). Anyway, all the comments were useful to me as they helped to make things less confused. Thanks, M. (*) That's why I told you I wanted to improve my english. ;-) |
From: Egon W. <eg...@sc...> - 2004-01-11 12:45:55
|
On Saturday 10 January 2004 22:33, M.Le_maudit wrote: > > Why should it be an interface? > > Personally, I prefer to have code like CDKConstants.X instead of > > 'implements CDKConstants' in the class definition... > > Actually my question should have been more acurate (*). The fact is that > the class comment confused me : > > [quote (CDK Constants class comment)] > An interface providing predefined values for a number of parameters used > throughout the CDK. Classes using these Constants should implement this > interface. > [/quote] It clearly isn't... will fix after I am done reading email... > So I wanted to know which one of the class declaration and the class > comment was up-to-date. > Moreover, I thought it was a good opportunity to ask the Team's opinion > about class/interface usage, but maybe I did this in a clumsy way (*). > (*) That's why I told you I wanted to improve my english. ;-) Well... I might now have read it well.... Anyway, I think an interface should be used when there is a common API shared by more than one Class... for example, to allow multiple implementations of the same goal, or an interface to other parts of the program, e.g. the plugin interface... Egon |
From: M.Le_maudit <mle...@us...> - 2004-01-11 19:12:13
|
> On Sunday, January 11, 2004 2:43 PM, Egon wrote : > [snip] > Anyway, I think an interface should be used when there is a common API > shared by more than one Class... for example, to allow multiple > implementations of the same goal, or an interface to other parts of the > program, e.g. the plugin interface... I understand your point of view and I'm ok with it. Thanks for this precision. Yours. M. |