Derby
  1. Derby
  2. DERBY-5803

Make error handling in xaHelper more explicit

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: 10.10.1.1
    • Fix Version/s: 10.10.1.1
    • Component/s: Tools
    • Labels:
      None

      Description

      In xaHelper the method handleException returns nothing (void), but it always throws an exception. Typically used inside a catch-block, this pattern confuses (static) code analyzers and reduces code readability.

      In this case it is possible to make the method return SQLException and write "throw handleException(t)", which makes it clear that the flow will stop if it reaches the catch block. Note that the method may still throw a RuntimeException, which I will document in the method Javadoc.

      The latter pattern is other places in the Derby code base, but I haven't checked for consistency. One specific case worth mentioning is the one in DRDAConnThread. This one is different in that it only takes correct actions, but doesn't actually throw an exception since we wan't the server thread to keep processing/executing after having cleaned up and informed the client.

        Activity

        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Resolved Resolved
        3d 41m 1 Kristian Waagan 11/Jun/12 12:35
        Resolved Resolved Closed Closed
        69d 20h 44m 1 Kristian Waagan 20/Aug/12 09:19
        Gavin made changes -
        Workflow jira [ 12672560 ] Default workflow, editable Closed status [ 12801808 ]
        Kristian Waagan made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Kristian Waagan made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Assignee Kristian Waagan [ kristwaa ]
        Fix Version/s 10.10.0.0 [ 12321550 ]
        Resolution Fixed [ 1 ]
        Hide
        Kristian Waagan added a comment -

        Thanks, Knut and Dag.
        I agree none of the approaches are perfect. The forgotten throw is something the other approach suffers from too, but there it's normally a more subtle bug (i.e. a bug in a switch statement or broken logic in if-statements) where an exception is thrown in most cases but not all.

        Committed patch 1a to trunk with revision 1348818.

        Show
        Kristian Waagan added a comment - Thanks, Knut and Dag. I agree none of the approaches are perfect. The forgotten throw is something the other approach suffers from too, but there it's normally a more subtle bug (i.e. a bug in a switch statement or broken logic in if-statements) where an exception is thrown in most cases but not all. Committed patch 1a to trunk with revision 1348818.
        Hide
        Dag H. Wanvik added a comment -

        Agreed, +1

        Show
        Dag H. Wanvik added a comment - Agreed, +1
        Hide
        Knut Anders Hatlen added a comment -

        +1

        I like the pattern where we explicitly throw the exception better, as then we don't have to trick the compiler by inserting unreachable return statements. The only downside I can see, is that bugs can be introduced if someone forgets the "throw" keyword, and this won't be detected by the compiler. However, I think that disadvantage is outweighed by the benefit that the control flow is easier to understand.

        Show
        Knut Anders Hatlen added a comment - +1 I like the pattern where we explicitly throw the exception better, as then we don't have to trick the compiler by inserting unreachable return statements. The only downside I can see, is that bugs can be introduced if someone forgets the "throw" keyword, and this won't be detected by the compiler. However, I think that disadvantage is outweighed by the benefit that the control flow is easier to understand.
        Kristian Waagan made changes -
        Field Original Value New Value
        Attachment derby-5803-1a-explict_throw.diff [ 12531392 ]
        Hide
        Kristian Waagan added a comment -

        Attaching patch 1a, implementing the change described above.

        Show
        Kristian Waagan added a comment - Attaching patch 1a, implementing the change described above.
        Kristian Waagan created issue -

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development