SAP / styleguides

This repository provides SAP style guides for coding and coding-related topics.
Other
1.67k stars 444 forks source link

Prefer EXPORTING parameters passed by value? #150

Open jordao76 opened 4 years ago

jordao76 commented 4 years ago

I'm reading the recommendations related to clearing the value of exporting parameters passed-by-reference and wondering, shouldn't it be preferred to have exporting parameters passed-by-value?

A pass-by-reference exporting parameter behaves like a changing parameter, and that's why you need to clear or assign it to make it behave like a true output parameter. Seems like a workaround.

A pass-by-value exporting parameter behaves like a returning parameter (which is always pass-by-value), which is a true output parameter.

I'm wondering if we can have a strong recommendation like "Make all your exporting parameters pass-by-value" or something less strong like "Prefer exporting parameters passed by value".

Any disadvantage of making exporting parameters pass-by-value? Does that recommendation make sense?

(Performance concerns are kind of already tackled in the recommendation that says that large tables are usually ok for returning parameters.)

mAltr commented 4 years ago

In general good point for a discussion. I think this is one of the reasons why the chapter "Prefer RETURNING to EXPORTING" was introduced. So instead of making a new rule, to make the parameter act like a returning, simply use returning. Or do you have a specific use case in mind, where you have to use exporting?

jordao76 commented 4 years ago

I agree, prefer returning to exporting is one of the rules. But if you do use exporting, my contention is that you should prefer by-value.

You have to use exporting when there's more than one parameter you need to return. If there was never a need to use exporting, then the rule would be "Don't use exporting, refactor to always use returning." Should that be the rule?

I'm trying to think in general terms, but I can think of a simple method where creating a structure to use as a returning parameter might be overkill:

METHODS try_parse_int
  IMPORTING
    text           TYPE string
  EXPORTING
    VALUE(success) TYPE abap_bool
    VALUE(result)  TYPE int4.

(Even though I can think of a returning type that might make sense, like a maybe_int, but that is besides the point.... Can anyone give me good examples of using exporting?)

You could also mix exporting and returning, but that would not be cleaner in my opinion (and it's also one of the rules).

I can think of the following advantages using exporting by-value:

Perhaps the disadvantage would be performance? But maybe limited to a very small number of cases.

fabianlupa commented 4 years ago

Perhaps the disadvantage would be performance?

I'd guess it also uses double the memory since two variables are allocated instead of one.

In your try_parse_int example I would have written parse_int that throws an exception on error and try_parse_int would return an initial value on failure.

I use exporting quite a bit still. You could refactor most usages to return structures as a single parameter. But it's a bit of work to create all these structure type definitions (even if they are not in DDIC). You can have the effect that the method uses a structure too big for its actual usage and some components aren't used at all because the developer was too lazy to create a perfectly fitting structure type.

One example is unit types (currency / type of amount). In lots of cases the variables come in pairs, amount_in_document_currency and document_currency or converted_amount and conversion_rate for example. I tend to use two exporting parameters for this instead of defining a structure type for every method.

But back to topic: Let's say there were valid use cases for using multiple exporting parameters so the prefer returning rule is ignored in these cases. Then, if there are no performance or memory concerns, I'd say you can always use pass by value to prevent having to clear the exporting parameters first. (I forget to do this quite often and only get reminded by the ATC check to add the clear statements.)

jordao76 commented 4 years ago

I'd guess it also uses double the memory since two variables are allocated instead of one.

Doesn't ABAP has optimizations for these kinds of scenarios, just like it is with returning parameters?

nununo commented 4 years ago

Doesn't ABAP has optimizations for these kinds of scenarios, just like it is with returning parameters?

As far as I know, even if a huge table is passed by value, an actual copy will only be done if something changes in one of them. Until then it will behave like if it was passed by reference. ABAP is supposed to be smart about this.

Unfortunately I wasn't able to find the place where this is documented but I'm quite sure I read it and experimented with it some years ago.

fabianlupa commented 4 years ago

Hmm, I was interested in the memory footprint since I kind of expected the ABAP compiler to not have these fancy optimizations.

Test report

REPORT z_fl_test_memory.

CLASS lcl_test DEFINITION.
  PUBLIC SECTION.
    TYPES:
      gty_table TYPE STANDARD TABLE OF char2000.
    CLASS-METHODS:
      test_1 EXPORTING et_tab TYPE gty_table,
      test_2 EXPORTING VALUE(et_tab) TYPE gty_table.
ENDCLASS.

CLASS lcl_test IMPLEMENTATION.
  METHOD test_1.
    et_tab = VALUE #( FOR i = 1 THEN i + 1 UNTIL i = 500 (
      repeat( val = 'A' occ = 2000 )
    ) ).
  ENDMETHOD.

  METHOD test_2.
    et_tab = VALUE #( FOR i = 1 THEN i + 1 UNTIL i = 500 (
      repeat( val = 'A' occ = 2000 )
    ) ).
  ENDMETHOD.
ENDCLASS.

START-OF-SELECTION.
  DATA: gt_tab TYPE lcl_test=>gty_table.

  gt_tab = VALUE #( FOR i = 1 THEN i + 1 UNTIL i = 500 (
    repeat( val = 'B' occ = 2000 )
  ) ).

  lcl_test=>test_1( IMPORTING et_tab = gt_tab ).
*  lcl_test=>test_2( IMPORTING et_tab = gt_tab ).
  cl_abap_memory_utilities=>write_memory_consumption_file( ).

Executed first like this and then a second time with the commented line swapped.

First run: image

Second run: image

Comparison from memory inspector:

image

image

I am not very experienced with using the memory inspector. But doesn't the + 4.7k bytes total allocated in the roll area indicate that there was no optimization?

If I remove the gt_tab = VALUE ... then the memory snapshots are identical. Which does lead to the theory that there is no memory downside using EXPORTING VALUE( if the target variable was already empty -> if the CLEAR statement in pass-by-reference would not have done anything anyway?

(adding a CLEAR et_tab. to test_1 doesn't change anything)

Louis-Arnaud commented 3 years ago

(Even though I can think of a returning type that might make sense, like a maybe_int, but that is besides the point.... Can anyone give me good examples of using exporting?)

Hello, I may have one example : a method "create_sales_order", you want to get the sales order ID AND all the messages (even if everything is ok, in order to get warning messages for example). Even in case of error, it's much more easier to export a message table than set the message table into an exception.

new zcl_sales_order( )->create( 
            EXPORTING
               i_header = sales_order_header
               i_items = sales_order_items
           IMPORTING
               e_sales_order_id = created_sales_order_id
               e_messages = return_messages ).

I would be interested to know your opinion about this.

nununo commented 3 years ago

@Louis-Arnaud I always prefer RETURNING to EXPORTING. Nowadays I'm trying to create a TYPE of a STRUCTURE containing everything that I want to export and RETURN the structure instead EXPORT each variable independently.

pokrakam commented 3 years ago

I'm going to take it a step further and say that multiple parameters are a sign of a method might be doing more than one thing.

The example above should return a sales order object. There's another variant to this:

data(log) = document->post_some_change( ... ).

I really dislike RETURNING logs in this manner. To me Clean Code is not about minimising, sometimes extra code is also good. So I'd much rather do:

  TRY.
      document->post_some_change( ... ).
      data(messages) = document->get_messages( ).
  CATCH ... 
    ...
  ENDTRY.

Another angle to this is straight out of Clean Code: G34: Functions Should Descend Only One Level of Abstraction. Getting messages fulfils do one thing and can be called after create_delivery, post_goods_issue and so on. Raising exceptions is an automatic filter on errors vs. information messages, and most of the time we're only interested in failures so this also avoids people having extra data elements that are never used by the caller.

Louis-Arnaud commented 3 years ago

Thanks for your answers.

Fill all the exporting parameters in a returning structure is more a technical way to avoid multiple exporting parameters.

Obviously, what @pokrakam suggests is the best answer in my opinion, but it requires to create a new class for the sales order object. Take much more time comparing to 2 exporting parameters :)

pokrakam commented 3 years ago

Hmm, I didn't suggest a new class, just to provide non-exception messages of the last operation for those who are interested. This can be done by making messages to an attribute and adding a method. In case of exceptions the messages will always be available.

METHOD create_order. 
  CALL FUNCTION 'BAPI_...' 
    IMPORTING
      order_id = order_id
      messages = messages. 
  check_no_errors( messages ).
  result = new zcl_sales_order( order_id ).
ENDMETHOD.

METHOD post_some_change. 
  CALL FUNCTION 'BAPI_...' 
    IMPORTING
      messages = messages. 
  check_no_errors( messages ).
ENDMETHOD.

METHOD check_no_errors. 
  LOOP AT messages WHERE type CA 'AEX'. 
    RAISE EXCEPTION TYPE cx_bapi_error 
      EXPORTING messages = i_messages.
  ENDLOOP.
ENDMETHOD. 

METHOD get_last_messages.
  result = messages.
ENDMETHOD.

Although it could be argued that this adds a time-dependency, this is just a quick example. There are many ways to solve this. I've also seen folks use a CHANGING parameter which I find better than RETURNING.

nununo commented 3 years ago

Fill all the exporting parameters in a returning structure is more a technical way to avoid multiple exporting parameters.

It's true. I suggested it as it is the best practice used in Python (and others). In Python this is much simpler since it is not a strongly-typed language. Still, it is a much more elegant solution because don't even need to store the resulting structure in the cases in which you only need one of the resulting values:

a = f( b )-c.

I also agree with @pokrakam and have use that approach, especially for logs. When using a common logger, I usually send it to all the involved instances through their constructor and then make it available as a read-only attribute.

RolfMantel commented 3 years ago

There is one very good reason for using EXPORTING with 'pass value' not set: generic programming.

When I do EXPORTING xxx TYPE ANY I can query the DDIC type of xxx inside my method.

Pass by value always needs fully typed variables.

fabianlupa commented 3 years ago

There is one very good reason for using EXPORTING with 'pass value' not set: generic programming.

When I do EXPORTING xxx TYPE ANY I can query the DDIC type of xxx inside my method.

Pass by value always needs fully typed variables.

Wouldn't CHANGING be a better fit for that use case?

RolfMantel commented 3 years ago

When I write a "generic query" method, CHANGING often semantically wrong.
By using EXPORTING I document that I expect the table to be empty. By using CHANGING I document that I intend to append the queried data to an existing table.