styleguides icon indicating copy to clipboard operation
styleguides copied to clipboard

Ensure that messages remain where-used-capable

Open HrFlorianHoffmann opened this issue 6 years ago • 3 comments

What I immediately noticed and would like to bring to attention: For maintainability, it is extremely important to have clean error handling and in case of incidents, being able to identify the source of the error / error message. With message classes, we have (ever since) the possibility to do this, using the Get-Where-Used-List feature of the system. However, this works if, and only if, the message can be tracked back to its message class. I noticed the bad habit that people create messages with literals, that cannot be found even with full text search. Clean Code for me here would mean that we keep the relation to the (SYST-)MSGNO.

This issue is a mirror of an issue that was originally created in the SAP-internal predecessor (SAP-internal link) to this Open Source repository.

I think it's still valid and should be added to this guide.

HrFlorianHoffmann avatar May 07 '19 15:05 HrFlorianHoffmann

I think this should be extended to exception classes and the use of IF_T100_DYN_MSG, encouraging the use of

RAISE EXCEPTION TYPE cx_fail
  MESSAGE ID ...
  ...

Incidentally, this is an area that some additional clarification from SAP could be useful. When exception classes were first introduced, the documentation hinted that the T100 model was an endangered species and instead encouraged customers to use exception classes pretty much across the technical and application domains due to their much richer functionality and bells and whistles such as OTR texts, attributes and inheritance.

But the approach seems to have shifted back, nowadays the doco recommends to only use exception classes in technical contexts and use T100 as our primary error concept for application-level errors.

pokrakam avatar May 07 '19 19:05 pokrakam

A common pattern for this is the following:

MESSAGE e055(oo) WITH 'VAR' INTO DATA(lv_dummy) ##NEEDED.
li_logger->add_msg_from_sy( ).

(ADT code template MESSAGE ${msgtyno}(${msgid}) WITH ${msgv} INTO DATA(${lv_dummy}) ##NEEDED).

With something like this:

METHODS:
  add_msg_from_sy IMPORTING VALUE(iv_msgid) TYPE syst_msgid DEFAULT sy-msgid
                            VALUE(iv_msgno) TYPE syst_msgno DEFAULT sy-msgno
                            VALUE(iv_msgty) TYPE syst_msgty DEFAULT sy-msgty
                            VALUE(iv_msgv1) TYPE syst_msgv DEFAULT sy-msgv1
                            VALUE(iv_msgv2) TYPE syst_msgv DEFAULT sy-msgv2
                            VALUE(iv_msgv3) TYPE syst_msgv DEFAULT sy-msgv3
                            VALUE(iv_msgv4) TYPE syst_msgv DEFAULT sy-msgv4

It is very important to use pass by value here as these variables can otherwise change in the implementation of add_msg_from_sy before they are read.

Unfortunately there is no MESSAGE ... TRANSPORTING NO FIELDS, so once you have more message statements you need to move the dummy variable declaration. I also saw code where global variables of the runtime environment are used, like INTO %_PRINT or INTO sy-some_hopefully_not_used_component which does not seem ideal to me.

There is also this pattern which seems way worse than the dummy MESSAGE statement as it has dead code and duplication (maybe MESSAGE ... INTO didn't exist yet?).

cl_feb_appl_log_handler=>add_message( i_msgid    = 'FEB_BSIMP'
                                      i_msgty    = 'E'
                                      i_msgno    = '056'
                                      i_msgv1    = lv_os_error_msg
                                      i_detlevel = iv_detlevel ).
IF 1 = 2. MESSAGE e056(feb_bsimp). ENDIF.       "#EC MG_PAR_CNT

Edit: And by the way, keeping the static reference to the message class is not only important for the where used list but also for code inspector / syntax checks, like does the message even exist and is the correct number of variables supplied. And I guess "is the message class even accessable?" if you are using package interfaces.

fabianlupa avatar May 08 '19 06:05 fabianlupa

MESSAGE e055(oo) WITH 'VAR' INTO DATA(lv_dummy) ##NEEDED. li_logger->add_msg_from_sy( ).

This is how I normally do it, but it is not perfect. If your logger is (for example) the BAL logger, you loose the reference to the message ID/Number. Any long-text is gone as well as the possibility to lok afterwards where this message was thrown.. you only have the text.

So it is a consideration if you prefer the user to have longtext or the developer to have a where-used-reference.

To "have both", you would have to somehow "do both" things (duplicate code).

openPhiL avatar Nov 25 '19 16:11 openPhiL

Was fixed in #88

bjoern-jueliger-sap avatar Apr 18 '23 11:04 bjoern-jueliger-sap