static exceptions

[sap]

static exceptions

[HrFlorianHoffmann]

We have discussed the very same points several times. Our discussions resulted in the conclusion that the following would be the ideal way to handle exceptions:

METHODS lower_method
  RAISING
    cx_flexible_exception.

METHOD lower_method.
  RAISE EXCEPTION NEW cx_flexible_exception( ).
ENDMETHOD:

METHOD middle_method.
  lower_method( ).
ENDMETHOD.

METHOD upper_method.
  TRY.
      middle_method( ).
    CATCH cx_flexible_exception.
      " ...
  ENDTRY.
ENDMETHOD.

This is, by the way, exactly what Robert C. Martin advertises for Java. We want to declare the exception in the lower_method that actually throws it, to not surprise callers. Then we want to let the exception bubble upwards through middle_methods without forcing them to redeclare or catch the exception. This avoids refactoring cascades if exceptions change. Finally, we want to catch the exception in some upper_method and handle it.

However, excactly this way is not possible in ABAP because none of the available three exception types supports it:

• cx_static_check forces middle_method to redeclare the cx_flexible_exception. Although this is "only a warning", it results in a Prio Very High issue that will prevent transport release in standard system setups. Even if you are willing to accept this, the code will not actually work but dump in middle_method because there is no catch block.

• cx_dynamic_check doesn't improve this. Alhough it accepts the missing redeclaration and catch block in middle_method, the code will still dump here if the exception is actually thrown.

• cx_no_check also doesn't help. Although the method bodies work, the METHODS lower_method definition now is no longer allowed to declare cx_flexible_exception and compilation fails with a syntax error.

[HrFlorianHoffmann]

Forwarded this discussion to the ABAP language teams. They will review whether this can/will be changed.

One suggestion is to allow all exception types in the RAISING clause, which would enable the CX_SY_NO_HANDLER-based pattern I lined out some posts above.

This is not a promise or commitment. This feature request may still be rejected for technical or effort reasons. It may also take a while to add it to the core.

[fabianlupa]

Adding my experience with exception classes. These are mostly in line with the styleguide.

Usually I use CX_STATIC_CHECK subclasses to indicate exceptions that must be handled by the caller, i. e. /ABC/CX_NO_AUTHORITY or /ABC/CX_NETWORK_ERROR. As the writer of the method I indicate cases that can (likely) happen and that I do not handle directly but rather pass on the responsibility to the caller.

I use CX_DYNAMIC_CHECK subclasses for exceptions that should have been preventable if the caller would have used the method correctly. This is indicated in the ABAP Doc of the method. For example there a no non-nullable object references in ABAP, so if io_thing is not bound I raise /ABC/CX_ILLEGAL_ARGUMENT with TEXTID NULLPOINTER. I do not expect the caller to handle this exception, I want it to bubble up and eventually raise a shortdump indicating a programming error because there was a programming error on the caller's side. Since it is dynamic check I declare /ABC/CX_ILLEGAL_ARGUMENT in the method signature and document in the ABAP Doc comment when it is raised.
I feel like I am allowed to raise a dynamic exception here because the caller violated the contract to my method. I however have to provide the tools so he is able to call my method correctly. Example:

CLASS lcl_stream DEFINITION.
  PUBLIC SECTION.
    METHODS:
      "! Open the connection
      "! @raising /abc/cx_illegal_state | Connection was already opened
      open RAISING /abc/cx_illegal_state,
      "! Close the connection
      "! @raising /abc/cx_illegal_state | Connection was not opened
      close RAISING /abc/cx_illegal_state,
      "! Is the connection open
      "! @parameter is_open | Connection is open
      is_open RETURNING VALUE(rv_open) TYPE abap_bool.
  PROTECTED SECTION.
  PRIVATE SECTION.
    DATA:
      mv_is_open TYPE abap_bool.
ENDCLASS.

CLASS lcl_stream IMPLEMENTATION.
  METHOD open.
    IF is_open( ) = abap_true.
      RAISE EXCEPTION TYPE /abc/cx_illegal_state.
    ENDIF.
    mv_is_open = abap_true.
  ENDMETHOD.

  METHOD close.
    IF is_open( ) = abap_false.
      RAISE EXCEPTION TYPE /abc/cx_illegal_state.
    ENDIF.
    mv_is_open = abap_false.
  ENDMETHOD.

  METHOD is_open.
    rv_open = mv_is_open.
  ENDMETHOD.
ENDCLASS.

I use subclasses of CX_NO_CHECK only for internal errors. For example if I call a function module that raises 8 different exceptions I might handle or reraise some of them and the others I do not expect to happen at all. So if any of the others happen I raise /ABC/CX_INTERNAL_ERROR expecting it to create a shortdump. (This includes the OTHERS exception as there might be new exceptions added after I implemented my call.)

Sidenote:
Unfortunately for me this is not always the case as others write amazing lines of code like this:

TRY.
    lo_nuclear_plant_manager->turn_on( ).
  CATCH cx_root ##CATCH_ALL ##NO_HANDLER.
    " Nothing to do here...
ENDTRY.

" Continue business as usual

And by others I include SAP standard code as some exits are wrapped in a similar way and in the end the user just gets a MESSAGE lx_ex TYPE 'S' DISPLAY LIKE 'E' and I see nothing while in the background a database deadlock occurred.

That's why my /ABC/CX_INTERNAL_ERROR class actually has logging logic build in to the constructor where it serializes itself, the callstack and system variables into a trace database table using a secondary database connection that gets committed immediately.

(Once I work with a release where RAISE SHORTDUMP is available I might use that instead.)

Regarding method signatures I try to always consider the context of each method so implementation details don't leak (especially in interfaces) and the caller has reasonable stuff to handle. For example if I should implement some math logic and provide several implementations, one using rv_sum = iv_a + iv_b one calling a math as a service webservice and one using quantum computing the webservice one will likely have to deal with a bunch of networking exceptions. The caller is not interested in how it is implemented so the networking exceptions should be wrapped and rethrown as a different kind of exception that is declared in the interface. Otherwise the interface would have to know all its implementations.

One thing that comes to mind is the NOT_FOUND problem.

CLASS-METHODS:
  "! Get customer by id
  "! @parameter iv_customer_id | Customer id
  "! @parameter ro_customer | Customer instance
  "! @raising /abc/cx_not_found | No customer found with this id
  get_customer_by_id IMPORTING iv_customer_id     TYPE kunnr
                     RETURNING VALUE(ro_customer) TYPE REF TO lcl_customer
                     RAISING   /abc/cx_not_found.

First decision you can take is if you want to raise an exception in the case a customer is not found or if you want to just return null. I personally like to raise the exception as handling null values is not fun and you have to duplicate the error message on the caller's side.
The next question is which type of exception this should be. According to the logic I stated above

• CX_NO_CHECK: This is not an internal error.
• CX_STATIC_CHECK: It could be a static check exception. But I expect the caller in most cases to already have a valid customer id as he likely retrieved it from somewhere else instead of making it up.
• CX_DYNAMIC_CHECK: I currently would use dynamic check to indicate that the method has not been called correctly according to the interface as the customer id was not valid. This should be fine as long as I also provide a way to validate customer ids, like does_customer_exist which just returns a boolean.

The advantage of using CX_DYNAMIC_CHECK instead of CX_STATIC_CHECK in this case is that the caller does not have write the try catch block if he knows the id is valid. In CX_STATIC_CHECK you would have to write an empty exception handler cluttering up the code just to get rid of the syntax warning (looking at you CX_SALV_NOT_FOUND...). And should you not know if the id is valid you can just handle the exception and even propagate it up the callstack keeping the information where it was raised, the error message etc.

In my opinion these are not really options:

• Unwrapping CX_SY_NO_HANDLER
• Having syntax warnings everywhere about missing CX_STATIC_CHECK handlers
• Adding CX_STATIC_CHECK to any method signature at all. It does not tell the caller what happened.
• Adding exceptions all the way up the call chain to all signatures. As I understand it the OO-exceptions are precisely for not doing that as you are able to keep the context and error message using either the PREVIOUS parameter (or using inheritance) while before with classic exceptions you just had a return code and had to reraise everything manually or lose the information why the error happened.

(Honorable mention to CX_UUID_ERROR: Do you expected me to start the UUID daemon on OS level in the catch block and try again???)

[pokrakam]

@flaiker "IS INITIAL" implied that my example retrurned a data element not an object :-)
Just kidding, yes that's a perfectly valid case, it goes back to what I said about knowing the caller's intent. If I return a chainable object, then more thought is needed. The same goes for the null vs. zero question. So I agree there are more reasons to use exceptions.

It's also a matter of coding approach. I like to start simple and refactor often; if the code starts to look like your example then it's time for a rethink.

To me an empty result is also just that bit more loosely coupled. I guess some of my preference partly stems from habit of working with BAPIs and other APIs. If it's a functional type scenario then I'll prefer the "caller must check" approach until other factors become overriding.

Another angle is caution from a consumer point of view. Can I always rely on the method I'm calling raising an exception correctly? No, been burnt several times so I am quite liberal with IS BOUND anyway, and sometimes a simple ASSERT <f> IS BOUND / IS NOT INITIAL. provides that extra safety net.

[HrFlorianHoffmann]

Reformulated the section in question to hopefully avoid similar misunderstandings in the future

Even if we were willing to accept these warnings, without redeclaring /clean/flexible_exception, middle_method would not forward it but trigger a cx_sy_no_handler exception which ultimately leads to a dump.

[marcellourbani]

Love the log on creation idea.
Makes sense if you police it properly all over the codebase or you can live with shortdumps

Have to defend the unwrapping of CX_SY_NO_HANDLER. I mostly do it for reporting errors to the user, and the message 'an exception was raised and not handled' should never reach his eyes
On a log sure, followed by the original message and perhaps a stack trace

I do agree on ignoring the warnings on principle, when I have time I catch & wrap. But I often don't
Anyway the point was that both wrapping, bubbling explicitly and ignoring suck IMHO

For CX_UUID_ERROR I guess you want to tell someone to call Basis :)

[fabianlupa]

Have to defend the unwrapping of CX_SY_NO_HANDLER. I mostly do it for reporting errors to the user, and the message 'an exception was raised and not handled' should never reach his eyes

Exactly, which is why (as I understand it) you should never catch CX_SY_NO_HANDLER. It being raised indicates a programming error (either a RAISING or a CATCH is missing somewhere) which should be reported to the developer and not to the user. And a shortdump does exactly that.

[marcellourbani]

@HrFlorianHoffmann

That's why I usually unpack CX_SY_NO_HANDLER

METHOD upper_method.
  DATA: flexible_exception TYPE REF TO cx_flexible_exception,
             nohandler              TYPE REF TO cx_sy_no_handler.
  TRY.
      middle_method( ).
    CATCH cx_flexible_exception INTO nohandler.
      " might need another try..catch below
      " or some other weird stuff like cl_abap_typedescr=>describe_by_object_ref + case...
     flexible_exception ?= nohandler->parent.
     "...
  ENDTRY.
ENDMETHOD.

which is my best approximation of what you describe.
When I deal with code I don't own. When I do I use CX_NO_CHECK

As said, IMHO there's no good choice available in ABAP

@flaiker

Exactly, which is why (as I understand it) you should never catch CX_SY_NO_HANDLER. It being raised indicates a programming error (either a RAISING or a CATCH is missing somewhere) which should be reported to the developer and not to the user. And a shortdump does exactly that.

In a dev system I would agree.
I usually have to make sure it performs acceptably on customer sites where I only know part of the problem, and fixing a code issue involves opening a ticket with us, going through our dev & QA cycle, backporting and finally sending a patch. Way too expensive and slow
Also I can't refactor all my legacy code on the spot, forget predicting what new exception a standard method will raise at the next SAP release. Some sort of catch-all is needed and as ugly as it can be unpacking no_handler is the nicest pig

[kjetil-kilhavn]

• cx_no_check also doesn't help. Although the method bodies work, the METHODS lower_method definition now is no longer allowed to declare cx_flexible_exception and compilation fails with a syntax error.

Any chance this is fixable in a finite amount of time so we can declare the CX_NO_CHECK exceptions?

[pokrakam]

Yes, I find the restriction on declaring cx_no_check based exceptions bizarre. Why can't we do something intended to help other developers? It's up to them whether to handle it or let it slide up the stack.
Also discussed here:
https://answers.sap.com/questions/407356/why-is-it-not-allowed-to-declare-an-exception-that.html

[pokrakam]

@flaiker - great post. However re. the NOT_FOUND problem, I tend to go a little more sparingly in my exceptions.
It's a semantic thing: Does not_found mean it's broken, or is the caller just doing an existence check? The first case warrants an exception, but as I may not know the (possibly future) caller's intent I will only use exceptions for "it's definitively broken" situations, or cases where I need to communicate different reasons for not_found.

Personally I also think handling a null result is also a little cleaner on the caller's side so I also prefer that for readability purposes. In many cases I prefer to get the error handling out the way and then move onto the meat of the code. I often find reading TRY-CATCH blocks in ABAP a bit disjointed, usually the exception handling is a line or two and the regular path somewhat longer, by the time you reach the end and see CATCH it takes more effort to remember what it relates to. So I prefer to put the short bit at the top.

Thus in most cases I prefer:

DATA(f) = obj->find_stuff( key ). 
IF f IS INITIAL. 
  "raise exception / set a status
ENDIF.
... "mainline code

to

TRY. 
    DATA(f) = obj->find_stuff( key ). 
     ... "do stuff
     ... "more stuff
     ... "and more
  CATCH cx_stuff_not_found. 
  "raise exception / set a status / whatever
ENDTRY.

Also, the lower method doesn't know how the caller wants to handle or communicate the situation - they might not care; or they might raise a different exception, making for awkward exception conversions. It also makes changing the exceptions more difficult.
So unless I absolutely want to force the caller to deal with it in MY way, I go with null results.

[fabianlupa]

@pokrakam

Counter argument to null results is chained method calls. With exceptions you can just do them without worrying and catch and handle the different exceptions. This results in cleaner more readable code in my opinion. With null values you get a cascaded if construct (there is no null safe operator .? like in other languages) where you also have to provide error messages yourself.

Your examples could just as well look like this:

DATA(f) = obj->find_stuff( key ). 
IF f IS NOT INITIAL. 
  DATA(g) = g->get_other_thing( something ).
  IF g IS NOT INITIAL.
    DATA(h) = g->do_something( ).
    IF h = abap_false.
      "raise exception / set a status
    ENDIF.
    ... "mainline code
  ELSE.
    "raise exception / set a status
  ENDIF.
ELSE.
  "raise exception / set a status
ENDIF.

to

TRY.
    IF obj->find_stuff( key )->get_other_thing( something )->do_something( ) <> abap_true.
      RAISE EXCEPTION NEW zcx_not_successful( iv_reason = 'some reason' ).
    ENDIF.
  CATCH cx_not_found INTO DATA(lx_ex1).
    RAISE EXCEPTION NEW cx_wrapped_error( previous = lx_ex1 ).
  CATCH cx_other_error.
    " Handle this one here
    ...
ENDTRY.

I have to admit though, if you have to have another TRY CATCH block in there it can get worse than the IF construct above.

You also have to somehow document if a value can be null or not (and again there is no @Nullable @NotNull etc. or even non nullable types or Optionals like in other languages).

For existence checks I usually provide a different method, that just does the existence check and simply returns a boolean value. Using get_customer_by_id might be a very expensive operation (possibly including RFC and webservice calls to load all the data).

But of course there is no definitive answer and it depends on the specific case at hand.

[HrFlorianHoffmann]

Kindly let me know your thoughts on #37. While it may not be possible to resolve the discussion, it may still be worth it outlining the reasoning to allow readers to come to their own conclusions.

[pokrakam]

Nicely explained!
Overall I've come to the conclusion that cx_no_check is the way to go if I don't expect the immediate caller to handle it. Since we're not allowed to declare it, the next best thing it to make sure it's noted in the ABAP Doc comment.

  "! Note: Raises exception zcx_foo
  METHODS lower.

Even if we could declare cx_no_check, if you're editing method upper and looking at method middle, you wouldn't see lower's exception declaration anyway. So from that point of view describing it in the ABAP Doc is almost equivalent to declaring it. Looking at a call to lower, you'll still immediately see it in Eclipse's F2:

but unfortunately won't see it in the signature if you're working in SE24. Then again, most people adopting Clean Code would be using ADT, right? :-)

So maybe we should promote the use of cx_no_check a bit more?

Regarding the ABAP language, removing the restriction on declaring cx_no_check's should theoretically be doable without affecting any existing code, so I don't see this as a risky endeavour..?

[kjetil-kilhavn]

I would say that is a fair and good explanation of the pros and cons - a very nice addition to this excellent project. I was not aware how utterly stupid the statically checked exception behave. I have assumed that they would be forwarded if declared in the middle methods - and I am assuming that's what happens with dynamically checked ones. From a code organization view it's hardly any better than a return code I would say. Never again will I declare one - and I am not sure if I ever have either :-)
cx_dynamic_check can make sense for the public interface, while anything else should be cx_no_check.

[pokrakam]

@kjetil-kilhavn I think you misread something, declared static-check exceptions are forwarded if the caller declares it.

[kjetil-kilhavn]

Well, that's a relief, @pokrakam - thanks for the clarification.

cx_static_check would force middle_method
to redeclare the /clean/flexible_exception.
Although the syntax check throws "only" a warning,
the ABAP Test Cockpit responds with an issue with Very High priority
that will prevent transport release in standard system setups.

Even if we were willing to accept this,
middle_method would not simply forward /clean/flexible_exception
if it actually occurred,
but trigger a cx_sy_no_handler exception
which ultimately leads to a dump.

I interpreted the 'even if we were willing to accept this' as pointing to the declaration in middle methods and understood this to mean that even if it was declared it would not be passed on in the call stack.
Perhaps that paragraph could be slightly rewritten then, as IMNSHO others too may misunderstand.

[HrFlorianHoffmann]

Suggest to close this for now. Kindly reopen or create a new issue with link to this one if you think we should continue the discussion. :-)

[niburi]

you also can make an exception cascade, for example:
try. "your code catch zyour_exception_class into data(exeption). raise exception new zyour_exception_class( textid = exception->IF_T100_MESSAGE~T100KEY ) endtry.
with this kind of call you can forward your exception to higher level methods. You also have to refer the exception class in your exceptions of the method.