Derby
  1. Derby
  2. DERBY-5224

[patch] reduce cohesion by removing overzealous casting

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 10.9.1.0
    • Component/s: SQL
    • Labels:
      None
    • Urgency:
      Low

      Description

      various spots casts assignments to a type more restrictive than needed, increasing cohesion. Patch reduces the casting to the required types.

        Activity

        Dave Brosius created issue -
        Dave Brosius made changes -
        Field Original Value New Value
        Attachment reduce_cohesion.diff [ 12478711 ]
        Hide
        Knut Anders Hatlen added a comment -

        The patch looks fine to me. (It includes the DERBY-5200 changes too, but that should be easy enough to revert when applying the patch.) Dave, did you run any tests on the patch?

        Show
        Knut Anders Hatlen added a comment - The patch looks fine to me. (It includes the DERBY-5200 changes too, but that should be easy enough to revert when applying the patch.) Dave, did you run any tests on the patch?
        Hide
        Dave Brosius added a comment -

        ran

        java -classpath tools/java/junit.jar:tools/java/jakarta-oro-2.0.8.jar:jars/sane/derbyTesting.jar:jars/sane/derby.jar:jars/sane/derbyTools.jar:jars/sane/derbyTools.jar:jars/sane/derbyrun.jar:jars/sane/derbynet.jar:jars/sane/derbyclient.jar -Xmx800m -XX:MaxPermSize=512m junit.textui.TestRunner org.apache.derbyTesting.functionTests.suites.All

        with no errors

        Show
        Dave Brosius added a comment - ran java -classpath tools/java/junit.jar:tools/java/jakarta-oro-2.0.8.jar:jars/sane/derbyTesting.jar:jars/sane/derby.jar:jars/sane/derbyTools.jar:jars/sane/derbyTools.jar:jars/sane/derbyrun.jar:jars/sane/derbynet.jar:jars/sane/derbyclient.jar -Xmx800m -XX:MaxPermSize=512m junit.textui.TestRunner org.apache.derbyTesting.functionTests.suites.All with no errors
        Hide
        Dag H. Wanvik added a comment -

        I am not sure this change is always "in tune" with the current idioms we employ. For example, when allocating compiler nodes using the factory method, e.g.

        prnRSN = (RowCountNode)getNodeFactory().getNode(
        C_NodeTypes.ROW_COUNT_NODE,
        prnRSN,
        topList,
        offset,
        fetchFirst,
        getContextManager());

        the pattern has always been to cast to the class corresponding to the nodetype, i.e. ROW_COUNT_NODE, although we don't not always need the specialized node version at the point where it is being created. Perhaps it would be beneficial to stick to this established pattern to avoid introducing errors? I don't see the need to change this a priori?

        Show
        Dag H. Wanvik added a comment - I am not sure this change is always "in tune" with the current idioms we employ. For example, when allocating compiler nodes using the factory method, e.g. prnRSN = (RowCountNode)getNodeFactory().getNode( C_NodeTypes.ROW_COUNT_NODE, prnRSN, topList, offset, fetchFirst, getContextManager()); the pattern has always been to cast to the class corresponding to the nodetype, i.e. ROW_COUNT_NODE, although we don't not always need the specialized node version at the point where it is being created. Perhaps it would be beneficial to stick to this established pattern to avoid introducing errors? I don't see the need to change this a priori?
        Hide
        Dave Brosius added a comment -

        Do what you like of course, but if that is the case, it would seem to me that prnRSN in your example should be defined as a RowCountNode then.

        Show
        Dave Brosius added a comment - Do what you like of course, but if that is the case, it would seem to me that prnRSN in your example should be defined as a RowCountNode then.
        Hide
        Dag H. Wanvik added a comment -

        I checked again, and I see that the pattern I referred to isn't always used at all: For example in SelectNode, ca 30% of node creations do not use the pattern. So, I'm ok with the change.

        Show
        Dag H. Wanvik added a comment - I checked again, and I see that the pattern I referred to isn't always used at all: For example in SelectNode, ca 30% of node creations do not use the pattern. So, I'm ok with the change.
        Hide
        Lily Wei added a comment -

        +1 The change looks good to me. I think more cohesion is better by casting. I also run derbyall and all tests pass with the patch.

        Show
        Lily Wei added a comment - +1 The change looks good to me. I think more cohesion is better by casting. I also run derbyall and all tests pass with the patch.
        Hide
        Knut Anders Hatlen added a comment -

        Thanks, Dave, for the patch, and Dag and Lily for reviewing and testing the patch. Committed revision 1102679.

        Show
        Knut Anders Hatlen added a comment - Thanks, Dave, for the patch, and Dag and Lily for reviewing and testing the patch. Committed revision 1102679.
        Knut Anders Hatlen made changes -
        Assignee Dave Brosius [ dbrosius@apache.org ]
        Fix Version/s 10.9.0.0 [ 12316344 ]
        Issue & fix info [Patch Available]
        Component/s SQL [ 11408 ]
        Knut Anders Hatlen made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Knut Anders Hatlen added a comment -

        [bulk update] Close all resolved issues that haven't been updated for more than one year.

        Show
        Knut Anders Hatlen added a comment - [bulk update] Close all resolved issues that haven't been updated for more than one year.
        Knut Anders Hatlen made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Gavin made changes -
        Workflow jira [ 12613025 ] Default workflow, editable Closed status [ 12802714 ]
        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Resolved Resolved
        2d 18h 25m 1 Knut Anders Hatlen 13/May/11 12:22
        Resolved Resolved Closed Closed
        765d 21h 56m 1 Knut Anders Hatlen 17/Jun/13 10:19

          People

          • Assignee:
            Dave Brosius
            Reporter:
            Dave Brosius
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development