Clean code: should we replace macros with methods, and why?

[sandraros]

Recently, there was some little refactoring in a few abap2xlsx programs or classes, to replace macros (DEFINE ... END-OF-DEFINITION) with methods:

• zcl_excel_common: unit tests cleanup #806
• zcl_excel_style_number_format: refactor macro to method #818

My question:

• Is that a best practice in ABAP community? Discussion links?
• Could it be decided once for all and write the decision in the abap2xlsx contribution guidelines, so that to have coherent code?

Thanks!

This issue can be used as support to collect future modifications. These are the macros left in abap2xlsx:

• https://github.com/sapmentors/abap2xlsx/blob/d19ac690c6089dcdfe9d2895baab2e8cd7279399/src/demos/zdemo_excel27.prog.abap#L104-L126
• https://github.com/sapmentors/abap2xlsx/blob/d19ac690c6089dcdfe9d2895baab2e8cd7279399/src/demos/zdemo_excel27.prog.abap#L136-L157
• https://github.com/sapmentors/abap2xlsx/blob/ac5c95ace70a958bcd401e51e190054ece888978/src/demos/zdemo_excel_outputopt_incl.prog.abap#L188-L193
• https://github.com/sapmentors/abap2xlsx/blob/ac5c95ace70a958bcd401e51e190054ece888978/src/not_cloud/zcl_excel_ole.clas.abap#L251-L296
• https://github.com/sapmentors/abap2xlsx/blob/ac5c95ace70a958bcd401e51e190054ece888978/src/not_cloud/zcl_excel_ole.clas.abap#L301-L309
• https://github.com/sapmentors/abap2xlsx/blob/ac5c95ace70a958bcd401e51e190054ece888978/src/zcl_excel_worksheet.clas.abap#L1409-L1422
• https://github.com/sapmentors/abap2xlsx/blob/ac5c95ace70a958bcd401e51e190054ece888978/src/zcl_excel_worksheet.clas.abap#L1424-L1439
• https://github.com/sapmentors/abap2xlsx/blob/ac5c95ace70a958bcd401e51e190054ece888978/src/zcl_excel_worksheet.clas.abap#L1592-L1597
• https://github.com/sapmentors/abap2xlsx/blob/372fe61c759257c8d49913e1924db9bdce7aa83d/src/zcl_excel_writer_2007.clas.abap#L2285-L2299
• https://github.com/sapmentors/abap2xlsx/blob/372fe61c759257c8d49913e1924db9bdce7aa83d/src/zcl_excel_writer_2007.clas.abap#L3119-L3133

[larshp]

https://help.sap.com/doc/abapdocu_752_index_htm/7.52/en-US/abenmacros_guidl.htm

[larshp]

plus the linter and transpiler doesnt work very well with macros, and I'm too lazy to fix it

the linter and transpiler I see as a prerequisite for unit testing, unit testing is a prerequisite for keeping abap2xls stable and adding more features

[sandraros]

I'm not the devil's advocate, just to say, here's an example of "exceptional case" (c.f. macro guidelines), which is about avoiding a huge number of lines, in standard method GET_METADATA of CL_SHM_RIS_METADATA (https://launchpad.support.sap.com/#/corrins/3006480/41), with 15,000 lines of below kind of code, combined with the context which requires the code to be compatible with ABAP < 7.40, i.e. VALUE expressions are not possible (which is currently the case of abap2xlsx):

    CLEAR ls_md.
    insert_risdata '1' 'PROG' '' 'P' ls_md-where_used.
    insert_srchgrp 'STANDARD' '' 'TEXT-S01'.
    insert_srchgrp 'EINSTELLUNG' '' ''.
    insert_srchelm 'TITLE-PROGNAME' 'STANDARD' '1' '' '' '0' 'KEY1X' '' 'PA'.
    INSERT ls_search_elements_fb INTO TABLE ls_md-search_elements.
    insert_srchelm 'TITLE-PROGNAME' 'STANDARD' '2' '' '' '0' 'KEY1' '' 'SO'.
    INSERT ls_search_elements_fb INTO TABLE ls_md-search_elements.
    ...
    insert_ris_md 'PROG' '' 'PZ'.

the 4 macros are:

    DEFINE insert_risdata.
      CLEAR ls_risdata.
      ls_risdata-group            = to_upper( &1 ).
      ls_risdata-trobjtype        = to_upper( &2 ).
      ls_risdata-subtype          = to_upper( &3 ).
      ls_risdata-legacy_type      = to_upper( &4 ).
      INSERT ls_risdata INTO TABLE &5.
    END-OF-DEFINITION.

    DEFINE insert_srchgrp.
      CLEAR ls_search_group_fb.
      ls_search_group_fb-name            = &1 .
      ls_search_group_fb-parent_group    = &2 .
      ls_search_group_fb-text_ref        = &3 .
      INSERT ls_search_group_fb INTO TABLE ls_md-search_groups.
    END-OF-DEFINITION.

    DEFINE insert_srchelm.
      CLEAR ls_search_elements_fb.
      ls_search_elements_fb-for         = &1 .
      ls_search_elements_fb-group       = &2 .
      ls_search_elements_fb-index       = &3 .
      ls_search_elements_fb-label_for   = &4 .
      ls_search_elements_fb-label_ref   = &5 .
      ls_search_elements_fb-line        = &6 .
      ls_search_elements_fb-name        = &7 .
      ls_search_elements_fb-rb_group    = &8 .
      ls_search_elements_fb-type        = &9 .
    END-OF-DEFINITION.

    DEFINE insert_ris_md.
      ls_md-trobjtype        = to_upper( &1 ).
      ls_md-subtype         = to_upper( &2 ).
      ls_md-legacy_type     = to_upper( &3 ).
      INSERT ls_md INTO TABLE lt_metadata_fb.
    END-OF-DEFINITION.

[larshp]

yea

this is INSERT VALUE #( something something ) INTO TABLE lt_foobar.

which we cant use in 702 syntax

on long term I propose to develop abap2xlsx in 740 syntax, and then automatically downport it to 702, a prerequsite for this is using abaplint, which requires the macros to be refactored. After refactoring the macros, we can use the upport rules in abaplint to upport it to 740. But again, this is long term

[sandraros]

Another point which makes a macro more easy than a method is IF &1 IS SUPPLIED. You may have a look at the draft PR I have just proposed, all macros are removed (NB: I didn't test at all so it's a draft), which shows an example with SUPPLIED. I think the macros in zcl_excel_worksheet are more easy to read than the methods. If you are okay to keep the methods, I'm fine with it.

[AndreaBorgia-Abo]

Reopened, then closed again after double-checking they're all gone, sorry for the noise.

[sandraros]

Is it a problem to reopen when needed? (and keep "fix" as text of a PR if it's a fix)

[AndreaBorgia-Abo]

Is it a problem to reopen when needed? (and keep "fix" as text of a PR if it's a fix)

It's more of an inconvenience than a real problem, especially with long-running cleanups like the DDIC, since we can always reopen as needed.

More to the point, "fix" gives the idea that that specific commit actually solves the issue, whereas "WIP" clearly marks this as a partial solution toward the goal.

[sandraros]

Okay, fine, I'll keep it in mind. Agreed for "WIP".

Another solution is to always have a dedicated issue/enhancement per PR, even if its goal is to facilitate the fix of another issue. This way, "fix" can be kept. For instance, enhancement #524 is difficult to do without first refactoring change_cell_style with a dedicated PR, so I would create another enhancement to support that.

[AndreaBorgia-Abo]

Yup, I agree that having a dedicated intermediate issue as stepping stone, where possible, is a good idea.