Derby
  1. Derby
  2. DERBY-5746

Minor refactoring of DataDictionaryImpl.getSetAutoincrementValue

    Details

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

      Description

      DataDictionaryImpl.getSetAutoincrementValue contains an unused local variable, and while looking at removing it I saw that some of the statements in the method could be reordered to reduce object creation and improve readability.

      1. derby-5746-1a-minor_refactoring.diff
        2 kB
        Kristian Waagan
      2. derby-5746-2a-assert_on_fetch.diff
        2 kB
        Kristian Waagan

        Activity

        Hide
        Kristian Waagan added a comment -

        Attaching patch 1a, which moves some of the statements around, removes one unused local variable, and adds some comments.

        The regression tests passed with patch 1a.
        Patch ready for review.

        Show
        Kristian Waagan added a comment - Attaching patch 1a, which moves some of the statements around, removes one unused local variable, and adds some comments. The regression tests passed with patch 1a. Patch ready for review.
        Hide
        Knut Anders Hatlen added a comment -

        All the other callers of the various overloads of ConglomerateController.fetch() in DataDictionaryImpl (except one occurrence in updateCurrentSequenceValue()) either use the returned value or checks it in an assert. Perhaps we should do the same here for consistency?

        The other change, which narrows the scope of the bit set, looks like a good improvement.

        Show
        Knut Anders Hatlen added a comment - All the other callers of the various overloads of ConglomerateController.fetch() in DataDictionaryImpl (except one occurrence in updateCurrentSequenceValue()) either use the returned value or checks it in an assert. Perhaps we should do the same here for consistency? The other change, which narrows the scope of the bit set, looks like a good improvement.
        Hide
        Kristian Waagan added a comment -

        Thanks, Knut Anders.

        I committed the patch to trunk with revision 1338618, and will upload a new patch dealing with the two occurrences where the return value of the fetch-call is ignored.

        Show
        Kristian Waagan added a comment - Thanks, Knut Anders. I committed the patch to trunk with revision 1338618, and will upload a new patch dealing with the two occurrences where the return value of the fetch-call is ignored.
        Hide
        Kristian Waagan added a comment -

        Attaching patch 2a, which adds asserts in the two cases where the return value from ConglomerateController.fetch are ignored.

        Regression tests passed, read for review.

        Show
        Kristian Waagan added a comment - Attaching patch 2a, which adds asserts in the two cases where the return value from ConglomerateController.fetch are ignored. Regression tests passed, read for review.
        Hide
        Dag H. Wanvik added a comment -

        Seem good to have this debug-time check to me. +1

        Show
        Dag H. Wanvik added a comment - Seem good to have this debug-time check to me. +1
        Hide
        Kristian Waagan added a comment -

        Thanks, Dag.

        Committed patch 2a to trunk with revision 1339986.
        Closing issue.

        Show
        Kristian Waagan added a comment - Thanks, Dag. Committed patch 2a to trunk with revision 1339986. Closing issue.

          People

          • Assignee:
            Kristian Waagan
            Reporter:
            Kristian Waagan
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development