[Prefer RAISE EXCEPTION NEW to RAISE EXCEPTION TYPE] In defense of RAISE EXCEPTION TYPE

[marco-haupt]

Relevant sections of the style guide
https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#prefer-raise-exception-new-to-raise-exception-type

Summary
The current style guide recommends using RAISE EXCEPTION NEW over RAISE EXCEPTION TYPE. However, this overlooks the significant difference in the instantiation of the exception object between these methods. This could potentially impact program behavior. I propose to reconsider this guideline.

Description
Greetings,

I'd like to propose a discussion regarding the existing guideline that favors RAISE EXCEPTION NEW over RAISE EXCEPTION TYPE. While I acknowledge that RAISE EXCEPTION NEW is indeed shorter and arguably easier to read, I believe that there is a significant aspect that has not been sufficiently addressed.

The key difference between these two methods lies in the instantiation of the exception object. RAISE EXCEPTION NEW instantiates an exception object immediately, whereas RAISE EXCEPTION TYPE leaves this to the handler. The latter only creates the exception object when a CATCH or CLEANUP clause uses the INTO addition. Here's an illustration:

try.
  raise exception NEW cx_sy_zerodivide( ). "is an explicit constructor expression
catch cx_sy_zerodivide.
endtry.

try.
  raise exception TYPE cx_sy_zerodivide. "does not execute the constructor -> no instantiation
catch cx_sy_zerodivide.
endtry.

try.
  raise exception TYPE cx_sy_zerodivide.
catch cx_sy_zerodivide INTO data(exception_object). "INTO causes the object to be instantiated here
endtry.

The reason for this difference can be traced back to the late inclusion of class-based exceptions in the language. To ensure competitiveness and allow for regression-free refactoring, the class-based exceptions were designed to be as efficient as their predecessors. Since object creation can be relatively time-consuming and many exceptions can be handled without creating objects, it was deemed prudent to only create objects when absolutely necessary. The RAISE EXCEPTION variant (without TYPE) is meant to pass on existing exceptions to the next program level(s).

If exceptions are only used in exceptional situations and not as a rule, the performance may be negligible. But this distinction is of crucial importance if exception constructors are not free of side effects. Although best practices dictate that constructors should not have side effects, this cannot always be guaranteed. Therefore, if not properly understood, the substitution of RAISE EXCEPTION TYPE with RAISE EXCEPTION NEW could potentially modify the behavior of the program.

Personally, I would always advise using RAISE EXCEPTION TYPE due to its superior capabilities. However, if specifying EXPORTING is considered detrimental to readability, the guide should at least caution users about this potential pitfall.

I'm eager to hear your thoughts on this suggestion and hope to foster a productive dialogue that will enhance our coding practices.

Examples
Constructors should never have side effects! Please do not imitate! I only provide the following example to illustrate my point.

class cx_custom_error definition inheriting from cx_no_check.
  public section.
    methods constructor.
endclass.

class cx_custom_error implementation.
  method constructor.
    super->constructor( ).
    raise shortdump type cx_sy_no_handler.
  endmethod.
endclass.

start-of-selection.
  "This try block will be completed successfully.
  try.
    raise exception TYPE cx_custom_error.
  catch cx_custom_error.
  endtry.

  "This try block will end in a runtime error.
  try.
    raise exception NEW cx_custom_error( ).
  catch cx_custom_error.
  endtry.

[fabianlupa]

Huh, how weird. I've seen people add logic to the exception constructor to log exceptions, unaware of that apparently not happening in all cases?

What happens if parameters are supplied? Are expressions in the actual parameters only evaluated if someone catches the exception with into when using RAISE EXCEPTION TYPE?

Also now the "always create exception objects" setting in the debugger makes some more sense.

[bjoern-jueliger-sap]

I think the passage in the guide is definitely worthy of improvement, but I don't think we should recommend using RAISE EXCEPTION TYPE instead of RAISE EXCEPTION object.

What should be improved is

• the "needlessly longer" argument here makes no sense: the variant with TYPE has to explicitly write EXPORTING, but otherwise it's exactly the same as constructing inline with NEW from a readability perspective. The actual argument is that everywhere else we use NEW instead of other ways of constructing objects (as per this rule), because there they indeed are shorter (inline constructors are shorter and more readable than a data declaration and a CREATE OBJECT that potentially can even be some distance apart). For consistency (and thus to ease mental load on the reader), all places where objects are constructed should use NEW. Whether you do this inline with RAISE EXCEPTION NEW #( ) or whether you have e.g. some factory that makes exceptions and then you do RAISE EXCEPTION factory->get_error( ) isn't really the point.
• we should mention that RAISE EXCEPTION TYPE has this behaviour of only constructing the exception when necessary

Now, as for why we shouldn't recommend TYPE: No one expects this behaviour, and in properly written applications it should not be necessary. Exceptions are supposed to represent exceptional circumstances, and if they're thrown so often that the instantiation of the exception objects has a measureable impact on performance, the code has much more fundamental problems and changing its throws to RAISE EXCEPTION NEW would not make it "clean" in any meaningful sense of the word. In other words: If the application is meaningfully changed by replacing all TYPE throws with RAISE EXCEPTION object, that's an anti-pattern in itself. Like many rules in the style guide, just trying to apply a single rule without changing anything else about the architecture may lead to worse results in pathological cases. This is not a reason not to have the rule.

As for expectations: I don't know of a single other language with object-oriented exceptions that would have such conditional creation of exceptions, and the ABAP documentation itself seems to think this is a detail people shouldn't really bother with because it doesn't even explain this in any detail. All I could find is the following:

If the addition TYPE is specified, an exception of exception class cx_class is raised and, if necessary, an exception object is created.

with no elaboration on what exactly "necessary" means. So the vast majority of ABAP programmers has no idea this feature exists, and no non-ABAP programmer would expect it. Let's follow the principle of least surprise and have the guide recommend RAISE EXCEPTION object, which does exactly what everyone expects it to do.

[pokrakam]

The performance argument in favour of TYPE is an incorrect assumption, as I believe some partial instantiation or other unnecessary overhead is happening here.
See blog I wrote on it over here: https://community.sap.com/t5/application-development-blog-posts/abap-tips-checking-existence-within-an-internal-table/ba-p/13371859

From the performance hit and Marco's tests, I'm guessing there may be a partial instantiation happening internally, so I extended the CATCH variant of my test program with CATCH ... INTO .... It added 12% performance overhead (3.5s -> 4s).

Here are my performance test results that include CATCH ... INTO, to search for a value that doesn't exist in a table. It is only marginally slower than a DB SELECT!

Lookup nonexisting value:
line_exists( )                         :  385952
READ TABLE ... TRANSPORTING NO FIELDS  :  373634
READ TABLE                             :  373681
line_index( )                          :  420263
ASSIGN itab[ ... ]                     :  378104
REF #( itab[ ... ] OPTIONAL )          :  409134
VALUE #( itabl[ ... ] OPTIONAL )       :  469587
VALUE #( itabl[ ... ] OPTIONAL ) w/o IF:  421745
VALUE #( itabl[ ... ] OPTIONAL w/o IF ):  423877
VALUE #( itabl[ ... ] DEFAULT w/o IF ) :  406948
TRY ... itab[ ... ] CATCH              : 3519558
TRY ... itab[ ... ] CATCH INTO ...     : 4006461
SELECT SINGLE FROM usr01 WHERE ...     : 4301420

Just on the comment "in properly written applications it should not be necessary. Exceptions are supposed to represent exceptional circumstances, and if they're thrown so often that the instantiation of the exception objects has a measureable impact on performance, the code has much more fundamental problems"

I don't think this is always the case, I believe an exception can be a clean way to handle row = itab[ f = somevalue ]., I have no issue with the readability of:

try. 
  row = itab[ f = somevalue ].
catch cx_sy_itab_not_found. 
  row = calculate_and_add_row( ).
endtry.

The presence of an exception gives a hint that we expect the value to be there in most cases, but the performance hit is still surprising.

[fabianlupa]

I don't think this is always the case, what about row = itab[ f = somevalue ].?
It's pretty clean to handle using an exception:

Imo there are way cleaner solutions for this exact example than try-catch.

row = VALUE #( itab[ f = somevalue ] DEFAULT calculate_and_add_row_to( itab ) ).

row = VALUE #( itab[ f = somevalue ] OPTIONAL ).
IF row IS INITIAL.
  row = calculate_and_add_row_to( itab )
ENDIF.

or even

READ TABLE itab WITH KEY f = somevalue INTO row.
IF sy-subrc <> 0.
  row = calculate_and_add_row_to( itab ).
ENDIF.

Edit: ah you edited a bit and also covered the alternatives in the blog as well.
Edit2: Edited for syntax

[bjoern-jueliger-sap]

@pokrakam I don't think we're disagreeing - if the exception is truly exceptional, then there should be no performance problem and your TRY...CATCH is fine by my statement. If it's so common that you get performance issues from the catch, it's not exceptional and @fabianlupa's suggestions should be used - note that there's a VALUE #( ... ) missing there and the syntactically correct version is

  data(row) = value #( itab[ f = somevalue ] optional ).
  if row is initial.
    row = calculate_row( somevalue ).
    insert row into itab.   
  endif.

Also, the calculate_and_add_row_to in your examples is also questionable, since it seems to mutate itab "under the hood". It's much more explicit to make the computation of the row and its insertion into the table (which is a kind of memoization buffer here?) two separate statements.

[pokrakam]

I admit it wasn't the best example, do_something( ) would make it more generic. The point was really about the readability of a try ... catch construct for something that isn't necessarily that exceptional.

The problem with is initial is that it won't distinguish between empty and not found. Again my example is a bit rubbish.

Maybe it's just me but though I understand x = value #( itab[ f = '0' ] default calculate_something( ) )., it is not intuitive, my brain needs to process it twice to see what's being done so I don't like using it. I find try ... catch or an if ... else construct much easier to read.

Anyhow, all of that was a side issue, my main point was the fact that over 80% the performance overhead is still there with EXCEPTION TYPE, so the performance aspect of this discussion can effectively be ignored.

It does seem quite odd though that an uninstantiated class should consume so much resource, so maybe there's something that needs tweaking in the ABAP kernel?

[GTo78-code]

Its more to this point : https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP_de.md#nur-einen-ausnahmetyp-werfen

On this point : How about the delegation of concret error messages ?

What i miss is the normal request to display the right error message.

There is no way i know to delegate the message. You can pass if_t100_message~t100key but you need to replace the attributes in it.

So you have object A, witch contains the clear message. You force this as PREVIOUS object to object B which es no error message defined. So to display a message to the user, you need always to iterate among the hierachy and collect all messages from previous object.