SAP / styleguides

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

[Use CHANGING sparingly, where suited] Is it ok to use IMPORTING REF instead of CHANGING? #331

Closed Yrfo closed 1 year ago

Yrfo commented 1 year ago

Relevant sections of the style guide Use CHANGING sparingly, where suited

Question I recently found that I really hate CHANGING parameters, mainly because changing forces to type it with interface parameter name during method call. I got the idea that I even can use importing by reference instead of using changing. But I'm curious if it is ok.

So here is example of use of both variants:

"definition

METHODS example_of_calls.
METHODS change_by_changing
  CHANGING interface_parameter_name TYPE string.
METHODS change_by_ref
  IMPORTING interface_parameter_name TYPE REF TO string.

"implementation

METHOD change_by_changing.
  interface_parameter_name = interface_parameter_name && |added|.
ENDMETHOD.

METHOD change_by_ref.
  interface_parameter_name->* = interface_parameter_name->* && |added|.
ENDMETHOD.

METHOD example_of_calls.
  DATA(variable) = |not initial|.

  change_by_changing( CHANGING interface_parameter_name = variable ).
  change_by_ref( REF #( variable ) ).
ENDMETHOD.

Advantage of the ref that calling is simpler and possible easier refactor for renaming interface parameters. But disadvantage is that inside of method need to work with ref. Both the advantage and the disadvantage are not big, so probably it is personal. What do you think?

Also another question comes from that discussion. For example variable is already reference (e.g. class instance) and method changes the content of the variable (e.g. even via it's methods). Can be that variable passed as importing, or should go to changing? and why?

ruthiel commented 1 year ago

"I'd prefer to use REF# as it offers a cleaner and simpler approach. However, I understand the argument that by using CHANGING, everyone will know that the variable will be modified within the method.

bjoern-jueliger-sap commented 1 year ago

I think in general it is better to avoid mutating data in this manner altogether. In your example, is there is really a benefit to using change_by_ref instead of a method

methods add_to_parameter_name
  importing parameter_name type string
  returning value(result) type string.

and replacing

change_by_ref( ref #( variable ) ).

by

variable = add_to_parameter_name( variable ).

? Personally I find the latter much more readable - it is immediately clear to me that this line changes the content of variable based on its previous content.

nununo commented 1 year ago

Fully agree with @bjoern-jueliger-sap. With the added bonus that method add_to_parameter_name can now be a pure function, making it much more straightforward to cover with unit tests.

Yrfo commented 1 year ago

@bjoern-jueliger-sap really nice way of doing that! I forgot about such variant. I completely agree that it's much cleaner in case of data types.

Would you do the same for object instances, when for example variable is instance of an object? Or just importing can be enough? I guess in the case of objects the argument about mutation is not so strong, they are expected to mutate at any time, isn't it?

bjoern-jueliger-sap commented 1 year ago

@bjoern-jueliger-sap really nice way of doing that! I forgot about such variant. I completely agree that it's much cleaner in case of data types.

Would you do the same for object instances, when for example variable is instance of an object? Or just importing can be enough? I guess in the case of objects the argument about mutation is not so strong, they are expected to mutate at any time, isn't it?

For objects, the better solution is often to turn this into a method on the object. A function that takes an object instance as input and returns an object instance as output is isomorphic to a method on the object instance itself. (On the level of abstract type signatures, a method of an object type obj is just a function obj -> obj)

Concretely, if variable is an object, then that object type should define a method:

  methods add_to_parameter_name.

and we'd call it as

variable->add_to_parameter_name( ).

instead.

bjoern-jueliger-sap commented 1 year ago

Closing this since there seems to be agreement that the guide's recommendation against CHANGING in most situations is appropriate as is.

Yrfo commented 1 year ago

@bjoern-jueliger-sap really nice way of doing that! I forgot about such variant. I completely agree that it's much cleaner in case of data types. Would you do the same for object instances, when for example variable is instance of an object? Or just importing can be enough? I guess in the case of objects the argument about mutation is not so strong, they are expected to mutate at any time, isn't it?

For objects, the better solution is often to turn this into a method on the object. A function that takes an object instance as input and returns an object instance as output is isomorphic to a method on the object instance itself. (On the level of abstract type signatures, a method of an object type obj is just a function obj -> obj)

Concretely, if variable is an object, then that object type should define a method:

  methods add_to_parameter_name.

and we'd call it as

variable->add_to_parameter_name( ).

instead.

Ah, yes, it is true. Thank you guys for help! I'll try to remember that way of thinking 🙂