Menu ▾ ▴

#569 Remove IBond.Order.ordinal usage

Accepted
closed
nobody
None
cdk-1.4.x
1
2012-11-15
2012-11-04
John May
No

Using the 'ordinal()' method of enum is very bad. The documentation recommends only using it for "sophisticated enum-based data structures, such as EnumSet and EnumMap.".

There are several places in the CDK where ordinal is used to represent the number of connections for that Order. e.g. SINGLE.ordinal() + 1 = 1.

This patch adds a method to the enum that provides the number of connections. This allows use to removal all instances of "order.ordinal() + 1" and also if/else statements to count/sum bond orders can be minimised. Unfortunately the if/else couldn't be completely removed as we may have a null bond order.

One explicit point of review should be on SMSD where the ordinal was being added to the bond energy Line 139. The change does not affect unit tests so this needs a look to see what the ordinal was being used for.

patch is on branch: johnmay/patch/1.4.x.removing-order-ordinal-usage

Discussion

  • John May

    John May - 2012-11-04

    Needs more work - got an infinite loop somewhere. Will find the problem then open again.

     

  • John May

    John May - 2012-11-04
    • status: open --> pending
     

  • John May

    John May - 2012-11-04
    • status: pending --> open
     

  • John May

    John May - 2012-11-04

    fixed - accidentally removed a required conditional

     

  • John May

    John May - 2012-11-04
    • Description has changed:

    Diff:

    --- old
+++ new
@@ -4,8 +4,8 @@

 This patch adds a method to the enum that provides the number of connections. This allows use to removal all instances of "order.ordinal() + 1" and also if/else statements to count/sum bond orders can be minimised. Unfortunately the if/else couldn't be completely removed as we may have a null bond order.

-One explicit point of review should be on SMSD where the ordinal was being added to the bond energy [Line 167](
-https://github.com/johnmay/cdk/commit/031316a81e9c8f56522d6555c305c6f4dd0d92f1#L14L167). The change does not affect unit tests so this needs a look to see what the ordinal was being used for.
+One explicit point of review should be on SMSD where the ordinal was being added to the bond energy [Line 139](
+https://github.com/johnmay/cdk/commit/5113d4a66662fc2c97a070dfc461f62111411d2a#L14L139). The change does not affect unit tests so this needs a look to see what the ordinal was being used for.

 patch is on branch: [johnmay/patch/1.4.x.removing-order-ordinal-usage](https://github.com/johnmay/cdk/commits/patch/cdk-1.4.x/removing-order-ordinal-usage)
     

  • Egon Willighagen

    Egon Willighagen - 2012-11-10

    I find the term 'connections' misleading... why not just "integerValue" and asInteger() ?

    I also suggest this to go into master, rather than cdk-1.4.x.

     

  • John May

    John May - 2012-11-10

    I think something concise is needed - we could actually just have two methods it we can't agree.

    Order.toInteger() and (Order.connections() or Order.electronPairs())

     

    • Egon Willighagen

      Egon Willighagen - 2012-11-10

      What do you mean with concise() here? toInteger() is shorter than connections()...

      My problem with the latter is that a bond.connections() sounds like bond.getAtoms()... The name electronPairs() comes closer, but I would go for electronPairCount() then, as we use fooCount() elsewhere too...

      Still, these things is what the Order reflects in the first place... it's just that some algorithms like to have the value as integer...

       

  • John May

    John May - 2012-11-10

    Also master is okay, but I think it's quite an important change and there may be subtle bugs because of it. It seems that SMSD uses it a fair bit and it's not clear wether it was used incorrectly or whether it really was the order() - 1.

     

    • Egon Willighagen

      Egon Willighagen - 2012-11-10

      That is for Asad to tell us...

       

  • John May

    John May - 2012-11-10

    I think .numeric() is best :-)

    My problem with the latter is that a bond.connections() sounds like bond.getAtoms()... The name electronPairs() comes closer, but I would go for electronPairCount() then, as we use fooCount() elsewhere too...

    What do you mean with concise() here? toInteger() is shorter than connections()..

    Concise is actually about the word count and what they express: Giving a lot of information clearly and in a few words; brief but comprehensive
    But, in this case as you say connections() sounds like it means accessing the connections of the atom so that isn't correct.

    Anyways, Master or 1.4? I think there may be bugs because of it, which is why I patched for 1.4.

    J

    On 10 Nov 2012, at 14:46, Egon Willighagen egonw@users.sf.net wrote:

    What do you mean with concise() here? toInteger() is shorter than connections()...

    My problem with the latter is that a bond.connections() sounds like bond.getAtoms()... The name electronPairs() comes closer, but I would go for electronPairCount() then, as we use fooCount() elsewhere too...

    Still, these things is what the Order reflects in the first place... it's just that some algorithms like to have the value as integer...

    patches:569 Remove IBond.Order.ordinal usage

    Status: open Labels: IBond.Order ordinal cdk-1.4.x Created: Sun Nov 04, 2012 02:32 PM UTC by John May Last Updated: Sat Nov 10, 2012 02:16 PM UTC Owner: nobody

    Using the 'ordinal()' method of enum is very bad. The documentation recommends only using it for "sophisticated enum-based data structures, such as EnumSet and EnumMap.".

    There are several places in the CDK where ordinal is used to represent the number of connections for that Order. e.g. SINGLE.ordinal() + 1 = 1.

    This patch adds a method to the enum that provides the number of connections. This allows use to removal all instances of "order.ordinal() + 1" and also if/else statements to count/sum bond orders can be minimised. Unfortunately the if/else couldn't be completely removed as we may have a null bond order.

    One explicit point of review should be on SMSD where the ordinal was being added to the bond energy Line 139. The change does not affect unit tests so this needs a look to see what the ordinal was being used for.

    patch is on branch: johnmay/patch/1.4.x.removing-order-ordinal-usage

    Sent from sourceforge.net because you indicated interest in https://sourceforge.net/p/cdk/patches/569/

    To unsubscribe from further messages, please visit https://sourceforge.net/auth/prefs/

    Everyone hates slow websites. So do we.
    Make your web apps faster with AppDynamics
    Download AppDynamics Lite for free today:
    http://p.sf.net/sfu/appdyn_d2d_nov_______
    Cdk-patches mailing list
    Cdk-patches@lists.sourceforge.net
    https://lists.sourceforge.net/lists/listinfo/cdk-patches

     

    • Egon Willighagen

      Egon Willighagen - 2012-11-10

      OK, let's try cdk-1.4.x... we do need unit tests exposing some problems then...

       

  • Egon Willighagen

    Egon Willighagen - 2012-11-12

    Did you upload replacement patches with .numeric() instead of .connections() yet?

     

  • John May

    John May - 2012-11-12

    No not yet, Will change it now.

    I've asked Asad about the SMSD usages.

    Could you have a look at the one use in valency-check? It was using .ordinal() (i.e. without a +1) but replacing this with the '.numeric()' doesn't cause any test failures but I'm not sure if it's correct.

    John

    On 12 Nov 2012, at 05:22, Egon Willighagen egonw@users.sf.net wrote:

    Did you upload replacement patches with .numeric() instead of .connections() yet?

    patches:569 Remove IBond.Order.ordinal usage

    Status: open Labels: IBond.Order ordinal cdk-1.4.x Created: Sun Nov 04, 2012 02:32 PM UTC by John May Last Updated: Sat Nov 10, 2012 02:16 PM UTC Owner: nobody

    Using the 'ordinal()' method of enum is very bad. The documentation recommends only using it for "sophisticated enum-based data structures, such as EnumSet and EnumMap.".

    There are several places in the CDK where ordinal is used to represent the number of connections for that Order. e.g. SINGLE.ordinal() + 1 = 1.

    This patch adds a method to the enum that provides the number of connections. This allows use to removal all instances of "order.ordinal() + 1" and also if/else statements to count/sum bond orders can be minimised. Unfortunately the if/else couldn't be completely removed as we may have a null bond order.

    One explicit point of review should be on SMSD where the ordinal was being added to the bond energy Line 139. The change does not affect unit tests so this needs a look to see what the ordinal was being used for.

    patch is on branch: johnmay/patch/1.4.x.removing-order-ordinal-usage

    Sent from sourceforge.net because you indicated interest in https://sourceforge.net/p/cdk/patches/569/

    To unsubscribe from further messages, please visit https://sourceforge.net/auth/prefs/

    Everyone hates slow websites. So do we.
    Make your web apps faster with AppDynamics
    Download AppDynamics Lite for free today:
    http://p.sf.net/sfu/appdyn_d2d_nov_______
    Cdk-patches mailing list
    Cdk-patches@lists.sourceforge.net
    https://lists.sourceforge.net/lists/listinfo/cdk-patches

     

  • John May

    John May - 2012-11-12

    Okay rebased,
    https://github.com/johnmay/cdk/commits/patch/cdk-1.4.x/removing-order-ordinal-usage

    On 12 Nov 2012, at 08:39, John May jwmay@users.sf.net wrote:

    No not yet, Will change it now.

    I've asked Asad about the SMSD usages.

    Could you have a look at the one use in valency-check? It was using .ordinal() (i.e. without a +1) but replacing this with the '.numeric()' doesn't cause any test failures but I'm not sure if it's correct.

    John

    On 12 Nov 2012, at 05:22, Egon Willighagen egonw@users.sf.net wrote:

    Did you upload replacement patches with .numeric() instead of .connections() yet?

    patches:569 Remove IBond.Order.ordinal usage

    Status: open Labels: IBond.Order ordinal cdk-1.4.x Created: Sun Nov 04, 2012 02:32 PM UTC by John May Last Updated: Sat Nov 10, 2012 02:16 PM UTC Owner: nobody

    Using the 'ordinal()' method of enum is very bad. The documentation recommends only using it for "sophisticated enum-based data structures, such as EnumSet and EnumMap.".

    There are several places in the CDK where ordinal is used to represent the number of connections for that Order. e.g. SINGLE.ordinal() + 1 = 1.

    This patch adds a method to the enum that provides the number of connections. This allows use to removal all instances of "order.ordinal() + 1" and also if/else statements to count/sum bond orders can be minimised. Unfortunately the if/else couldn't be completely removed as we may have a null bond order.

    One explicit point of review should be on SMSD where the ordinal was being added to the bond energy Line 139. The change does not affect unit tests so this needs a look to see what the ordinal was being used for.

    patch is on branch: johnmay/patch/1.4.x.removing-order-ordinal-usage

    Sent from sourceforge.net because you indicated interest in https://sourceforge.net/p/cdk/patches/569/

    To unsubscribe from further messages, please visit https://sourceforge.net/auth/prefs/

    Everyone hates slow websites. So do we.
    Make your web apps faster with AppDynamics
    Download AppDynamics Lite for free today:
    http://p.sf.net/sfu/appdyn_d2d_nov_
    Cdk-patches mailing list
    Cdk-patches@lists.sourceforge.net
    https://lists.sourceforge.net/lists/listinfo/cdk-patches

    patches:569 Remove IBond.Order.ordinal usage

    Status: open Labels: IBond.Order ordinal cdk-1.4.x Created: Sun Nov 04, 2012 02:32 PM UTC by John May Last Updated: Mon Nov 12, 2012 05:22 AM UTC Owner: nobody

    Using the 'ordinal()' method of enum is very bad. The documentation recommends only using it for "sophisticated enum-based data structures, such as EnumSet and EnumMap.".

    There are several places in the CDK where ordinal is used to represent the number of connections for that Order. e.g. SINGLE.ordinal() + 1 = 1.

    This patch adds a method to the enum that provides the number of connections. This allows use to removal all instances of "order.ordinal() + 1" and also if/else statements to count/sum bond orders can be minimised. Unfortunately the if/else couldn't be completely removed as we may have a null bond order.

    One explicit point of review should be on SMSD where the ordinal was being added to the bond energy Line 139. The change does not affect unit tests so this needs a look to see what the ordinal was being used for.

    patch is on branch: johnmay/patch/1.4.x.removing-order-ordinal-usage

    Sent from sourceforge.net because you indicated interest in https://sourceforge.net/p/cdk/patches/569/

    To unsubscribe from further messages, please visit https://sourceforge.net/auth/prefs/

     

  • Egon Willighagen

    Egon Willighagen - 2012-11-15
    • status: open --> closed
    • milestone: Needs_Review --> Accepted
     

  • Egon Willighagen

    Egon Willighagen - 2012-11-15

    Looks good now. I does not introduce regressions, and it is an API extension. Since using .ordinal() is that dangerous, I agree it should go into 1.4.x...

    Applied and pushed.

     

Log in to post a comment.