Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Chained Methods/Constants #256

Open
lucasborin opened this issue Oct 22, 2021 · 5 comments
Open

Chained Methods/Constants #256

lucasborin opened this issue Oct 22, 2021 · 5 comments

Comments

@lucasborin
Copy link
Member

lucasborin commented Oct 22, 2021

In short, we should not chain up-front variable declarations.
But, how about METHODS, CLASS-METHODS, TYPES, and CONSTANTS?

For instance:

METHODS:
  get_name,
  get_age,  
  set_name,
  set_age. 
CONSTANTS:
  min_name_size TYPE i VALUE 3,
  min_age_allowed TYPE i VALUE 18.
@pokrakam
Copy link
Contributor

pokrakam commented Oct 24, 2021

Personally I follow the same principle and only chain closely related entities if it aids readability (i.e. short declarations). In practice I don't chain often, and usually break up the generated declarations that quick-fix insists on chaining. In Unit Tests is where chaining is probably the most useful:

"OK
METHODS setup. 

METHODS: 
  doc_type_a_is_accepted FOR TESTING,
  doc_type_b_fails FOR TESTING.

METHODS: 
  positive_qty_adds_item FOR TESTING,
  zero_qty_no_item FOR TESTING,
  negative_qty_raises_exception FOR TESTING.

I prefer horizontal density, and find wading through pages of long vertical declarations becomes unhelpful. We should definitely not chain when the METHODS part is more than 10-20 lines or so away from the declaration as you have no idea whether it's a static or public component or even an alias if you navigate there from somewhere else and have to hunt for the start of the chain.

"anti-pattern
    METHODS: 
      add
        IMPORTING
          num1 TYPE i
          num2 TYPE i
        RETURNING
          result TYPE i
        RAISING
          zcx_not_a_number,
      subtract
        IMPORTING
          num1 TYPE i
          num2 TYPE i
        RETURNING
          result TYPE i
        RAISING
          zcx_not_a_number,
      multiply
        IMPORTING
          num1 TYPE i
          num2 TYPE i
        RETURNING
          result TYPE i
        RAISING
          zcx_not_a_number,
      divide
        IMPORTING
          num1 TYPE i
          num2 TYPE i
        RETURNING
          result TYPE i
        RAISING
          zcx_not_a_number
          zcx_not_a_multiple.

For a good antipattern, imagine class CL_WB2_REBATE_SETTLEMENT with chained declarations...

@jordao76
Copy link
Contributor

Curiously enough, the identifiers are in plural mode: METHODS and CONSTANTS for example. They imply chaining. I always find it awkward to find a lone method declared with METHODS.... Either way I think your idea of grouping related methods to be most ideal.

@JSB-Vienna
Copy link

JSB-Vienna commented Jul 17, 2022

Since the inline declarations reduce the amount of necessary up-front declarations immensely I see no disadvantages in chaining declarations. A grouping of the declarations using comments or empty lines in between seems to me also a proper way to do that. What do you think?
Furthermore I don't understand the statment of complicating the reformatting in chapter "Do not chain up-front declarations" as the pretty printer handles this nicely.

@ConjuringCoffee
Copy link
Contributor

Do you handle the use of BEGIN OF & END OF differently? Personally, I prefer to chain these types of declarations. They definitely belong together, so chaining them makes sense to me.

CONSTANTS:
  BEGIN OF languages,
    german  TYPE spras VALUE 'D',
    english TYPE spras VALUE 'E',
  END OF languages.

@Ennowulff
Copy link

Ennowulff commented May 15, 2023

I only use chaining with TYPES. For all others (DATA, CONSTANTS, METHODS) I never use a the colon.
Reasons: much easier refactoring, when deleting, copying or moving things.

Examples:

METHODS: 
  one,
  two,
  three.

Deleting or moving method ONE leads to

  • add METHODS to mehod ONE or the rest of methods (depends on if METHODS stands in single line or directly in front of ONE).
  • adjust comma for method ONE when moving

Otherwise declarations can easily be deleted using CTRL+D or moving lines using ALT+ <cursor up/down>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants