From: Cyrus H. <ch...@bo...> - 2014-02-23 00:14:22
Attachments:
static-methods.patch
|
I should preface this by saying that I’m not really up to speed on current best practices in Java, but it seems to me that a bunch of the methods in AtomPlacer.java could be static methods (see attached patch). I don’t like non-static methods that don’t use the objects variables as it means needlessly instantiating the parent object just to call some methods that could otherwise be static. Of course I may be missing something obvious (or subtle) here, but the attached patch makes calling the AtomPlacer’s methods a bit easier for me. Note that this is a patch to the old (pre-maven) tree. Trivial enough to port, but I wanted to get this off of the stack before I go about trying to port the rest of my stuff over to the new (post-maven) tree structure. I’m happy to apply this to the new tree structure and file a pull request if this is something others would like to see in the tree. thanks, Cyrus |
From: John M. <joh...@gm...> - 2014-02-23 01:19:12
|
Sounds good, will apply patch tomorrow. Git tracks file moves. If you have the other changes as commits I can merge them in without the need to do any manual moving on your end. Sent from my iPhone > On 23 Feb 2014, at 00:14, Cyrus Harmon <ch...@bo...> wrote: > > > I should preface this by saying that I’m not really up to speed on current best practices in Java, but it seems to me that a bunch of the methods in AtomPlacer.java could be static methods (see attached patch). I don’t like non-static methods that don’t use the objects variables as it means needlessly instantiating the parent object just to call some methods that could otherwise be static. Of course I may be missing something obvious (or subtle) here, but the attached patch makes calling the AtomPlacer’s methods a bit easier for me. > > Note that this is a patch to the old (pre-maven) tree. Trivial enough to port, but I wanted to get this off of the stack before I go about trying to port the rest of my stuff over to the new (post-maven) tree structure. > > I’m happy to apply this to the new tree structure and file a pull request if this is something others would like to see in the tree. > > thanks, > > Cyrus > > <static-methods.patch> > ------------------------------------------------------------------------------ > Managing the Performance of Cloud-Based Applications > Take advantage of what the Cloud has to offer - Avoid Common Pitfalls. > Read the Whitepaper. > http://pubads.g.doubleclick.net/gampad/clk?id=121054471&iu=/4140/ostg.clktrk > _______________________________________________ > Cdk-devel mailing list > Cdk...@li... > https://lists.sourceforge.net/lists/listinfo/cdk-devel |
From: Cyrus H. <ch...@bo...> - 2014-02-23 01:28:41
|
Thanks! Yeah, I figured the rest of the changes would get moved over, but this one was on the top of the (my) stack. One other is: diff --git a/src/main/org/openscience/cdk/tools/manipulator/MolecularFormulaManipulator.java b/src/main/org/openscience/cdk/tools/manipulator/MolecularFormulaManipulator.java index d75f4e0..efebafc 100644 --- a/src/main/org/openscience/cdk/tools/manipulator/MolecularFormulaManipulator.java +++ b/src/main/org/openscience/cdk/tools/manipulator/MolecularFormulaManipulator.java @@ -95,6 +95,14 @@ public class MolecularFormulaManipulator { return count; } + public static int getElementCount(IMolecularFormula formula, IIsotope isotope) { + return getElementCount(formula, formula.getBuilder().newInstance(IElement.class, isotope)); + } + + public static int getElementCount(IMolecularFormula formula, String symbol) { + return getElementCount(formula, formula.getBuilder().newInstance(IIsotope.class, symbol)); + } + /** * Get a list of IIsotope from a given IElement which is contained * molecular. The search is based only on the IElement. hmm… looks like there are some tabs in there. well, you get the idea. thanks again, Cyrus On Feb 22, 2014, at 6:19 PM, John May <joh...@gm...> wrote: > Sounds good, will apply patch tomorrow. Git tracks file moves. If you have the other changes as commits I can merge them in without the need to do any manual moving on your end. > > Sent from my iPhone > >> On 23 Feb 2014, at 00:14, Cyrus Harmon <ch...@bo...> wrote: >> >> >> I should preface this by saying that I’m not really up to speed on current best practices in Java, but it seems to me that a bunch of the methods in AtomPlacer.java could be static methods (see attached patch). I don’t like non-static methods that don’t use the objects variables as it means needlessly instantiating the parent object just to call some methods that could otherwise be static. Of course I may be missing something obvious (or subtle) here, but the attached patch makes calling the AtomPlacer’s methods a bit easier for me. >> >> Note that this is a patch to the old (pre-maven) tree. Trivial enough to port, but I wanted to get this off of the stack before I go about trying to port the rest of my stuff over to the new (post-maven) tree structure. >> >> I’m happy to apply this to the new tree structure and file a pull request if this is something others would like to see in the tree. >> >> thanks, >> >> Cyrus >> >> <static-methods.patch> >> ------------------------------------------------------------------------------ >> Managing the Performance of Cloud-Based Applications >> Take advantage of what the Cloud has to offer - Avoid Common Pitfalls. >> Read the Whitepaper. >> http://pubads.g.doubleclick.net/gampad/clk?id=121054471&iu=/4140/ostg.clktrk >> _______________________________________________ >> Cdk-devel mailing list >> Cdk...@li... >> https://lists.sourceforge.net/lists/listinfo/cdk-devel > > ------------------------------------------------------------------------------ > Managing the Performance of Cloud-Based Applications > Take advantage of what the Cloud has to offer - Avoid Common Pitfalls. > Read the Whitepaper. > http://pubads.g.doubleclick.net/gampad/clk?id=121054471&iu=/4140/ostg.clktrk > _______________________________________________ > Cdk-devel mailing list > Cdk...@li... > https://lists.sourceforge.net/lists/listinfo/cdk-devel |