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

        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.
        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.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development