Uploaded image for project: 'Derby'
  1. Derby
  2. DERBY-5746

Minor refactoring of DataDictionaryImpl.getSetAutoincrementValue

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: 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
        kristwaa 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
        kristwaa 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
        knutanders 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
        knutanders 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
        kristwaa 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
        kristwaa 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
        kristwaa 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
        kristwaa 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
        dagw Dag H. Wanvik added a comment -

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

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

        Thanks, Dag.

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

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

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development