styleguides icon indicating copy to clipboard operation
styleguides copied to clipboard

Stimulate API based programming by means BAPI Return and additionally simplify error handling

Open WouterTriesCoding opened this issue 4 years ago • 5 comments

I like to use BAPIRET2_T in all my methods. For the cases of error handling, I also favor it above using exceptions.

Relating to error handling and exceptions, some thoughts about common pitfalls:

  • Small or large codebases with too many Z/Y Exception classes cause confusion amongs developers
  • Differences in Exception class definitions and how they are raised is confusing ( T100, custom return table on top, MESSAGE, ... ) and causes wrong exception handling/catching and thus bugs

Other common pitfalls:

  • Sometimes only the error handling is tackled, while other messages for information, warning and success are equally important. If this is not done, it is not easily reusable
  • Sometimes all the messages are directly added to a application log instance, tightly coupling it and blocking reuse of methods. Only the main method should contain the responsibility of logging to the application log, all other methods should work independently

So two topics I'd like to discuss:

  1. Use of a RETURN table for Methods, always as best practcice! Returning Success, Information and Errors. Always think API based, that your method can be reused in other methods or later on for new technologies, e.g. a report class API converted to Fiori/Gateway can just be plug and play, if it uses a RETURN table
  2. For smaller developments, keep it simple and use a RETURN table instead of custom Exception class. I've only use custom exception classes for very large developments with a clear domain and logical naming. I like Gateway as an example, they have 2 exception classes for error handling, one for business and one for technical, both with a RETURN table attached to it.

Any thoughts? Which chapter would this belong to? I was thinking under 'Methods' for the use of RETURN a la API (S/W/I/E) and under 'Error handling'.

Edit: added 'Other common pitfalls'

WouterTriesCoding avatar Apr 14 '21 12:04 WouterTriesCoding

There are many nice solutions for this type of conversation. In case i need an BAPIRET2_T i simply use it as attribute in my own exception class with get and set. The exception is only raised when the return contains any EAX. In this example the caller could decide to read and handle it or simply handle the exception.

Just a quick and dirty example:

class bapi_exception definition
    inheriting from
    cx_static_check.

  public section.
    class-methods raise_by_bapiret
      importing
        bapiret type bapiret2_t.
    methods set_bapiret
      importing
        bapiret type bapiret2_t.
    methods get_bapiret
      returning
        value(bapiret) type bapiret2_t.

  private section.
    data bapiret type bapiret2_t.
endclass.

class bapi_exception implementation.

  method raise_by_bapiret.

    data exception type ref to bapi_exception.
    exception = new #( ).
    exception->set_bapiret( bapiret ).

  endmethod.

  method get_bapiret.
    bapiret = me->bapiret. "me-> only to avoid shadowing
  endmethod.

  method set_bapiret.
    me->bapiret = bapiret. "me-> only to avoid shadowing
  endmethod.

endclass.


class example definition.
  public section.
    methods do_sth
      raising
        bapi_exception.

endclass.

class example implementation.

  method do_sth.

    data return type bapiret2_t.

    "... call bapi

    loop at return transporting no fields where type ca 'EAX'.
      bapi_exception=>raise_by_bapiret( return ).
    endloop.

  endmethod.

endclass.

class caller definition.
  public section.
    methods do.
endclass.

class caller implementation.

  method do.

    data example type ref to example.

    example = new #( ).

    try.
        example->do_sth( ).
      catch bapi_exception into data(return).
        "do whatever
    endtry.
  endmethod.

endclass.

mAltr avatar Apr 14 '21 12:04 mAltr

@mAltr that's clean! When do you decide to create a custom class based exception? A new one for every report/transaction or per class? A new one for every technical purpose, e.g. 1 central ZCL_EXCEPTION_BAPIRET with a BAPIRET2 table?

WouterTriesCoding avatar Apr 14 '21 14:04 WouterTriesCoding

Quite interesting topic and a thing I spend much time in the past.

We do it pretty much like mAltr suggested.

I just like to suggest some code to make your excpetions still compatible to standard/generic use cases. I think one should have the most important message/error really in the exception by raising them as dynamic message first and just add bapiret in addition - like this:

METHOD raise_by_bapiret.
 TRY.
    "use a logic to determine the most important message from bapiret table and use it here
    RAISE EXCEPTION TYPE cx_demo_dyn_t100
      MESSAGE ID 'SABAPDEMOS'
      TYPE 'I'
      NUMBER '888'
      WITH 'Message'.
 CATCH cx_demo_dyn_t100 into DATA(lx_cx).
   lx_cx->set_bapiret( bapiret ).
   RAISE lx_cx.
ENDTRY.
ENDMETHOD. 

This assures in case your exceptions are catched by someone who doesn't know or care at least gets a meaningful message when displaying/logging the excpetion.

To make it vice versa compatible there is function module RS_EXCEPTION_TO_BAPIRET2 which can be used to transform any exception to bapiret.

Regarding exceptions inheritance / structuring of excpetions I am still not completely happy but maybe some discussion is helpful.

We decided to have at least one abstract base exception per main package and all the one's used there should inherit from this one.. More or less like it's said in the guide here.

In addition we discussed having a central base exception for all our custom exceptions, but decided against it to avoid messy (unintended) catches of this central exception and we found no consent what really should be implemented of this central exception if we would have one. To have some comfort and avoid duplicated code we have some lightweight reuse utils for exceptions like converting a exception to bapiret, t100 message or picking the most important message out of a bapiret table or simply collecting several exceptions or transforming all this to application log by resolving previous exceptions or locks.

Talking about really small things we have some common exceptions to be used directly from the reUse package. Just like "no data found", "no authority" or "foreign enqueue".

BUPA avatar Apr 14 '21 17:04 BUPA

Hmm, I tend towards the opposite and prefer exceptions over messages.

One simple issue blows the whole BAPIRET idea out of the water, and that is that a return value places the responsibility on the caller to deal with it. A message parameter can be (and often is) forgotten, but an exceptions MUST be handled.

Following similar reasoning, return messages also mean that every caller has to deal with them, adding sometimes unnecessary code. Bubbling exceptions with a CATCH high up the call stack make the code much cleaner IMHO.

pokrakam avatar Apr 14 '21 18:04 pokrakam

Also, regarding exception classes, I tend to be quite liberal with them. It only takes two lines of code to define a subclass that will make your technical errors more descriptive straight away. This is one case where inheritance really shines, the caller doesn't need to know or care.

Gateway classes are a perfect example, I might declare local exceptions like:

CLASS lcx_not_authorized INHERITING FROM /iwbep/cx_mgw_busi_exception.
ENDCLASS.

That's it, just two lines of code purely for readability and improved error information. Gateway will still catch the same exception, but the class name is visible in the response so it will add an additional bit of information in case there are unclear messages.

pokrakam avatar Apr 14 '21 18:04 pokrakam