CHECK MATE

hardyp180 · ‎2019 Jun 13

Some organisations do not use the code inspector (or even the extended syntax check) at all. Others go bananas and insist every single warning be suppressed.

I tend to veer towards the latter approach. Naturally you can get false positives but you can suppress a true false positive with a pragma. Some people suppress everything with pragmas whether appropriate or not, but that is another story.

Now is the time to talk about the correct usage of the CHECK statement - mate.

One argument we have been having at work is about the level 3 (information) warning in the code inspector that says "Only use the CHECK statement right at the start of the routine, or in loops. Otherwise use RETURN". You see similar advice in the new "Clean ABAP" guidelines.

Presumably the logic is that the CHECK statement behaves differently in different positions - i.e. outside a loop a failure behaves like RETURN, inside of a loop a failure moves you on to the next entry in the loop.

I think the logic is that your average reader of the code is too dumb to understand the difference and might think a negative result of a CHECK in a loop might exit the entire routine.

Presumably the allowed exception - having CHECKS right at the start (what I call preconditions) are OK as it is more obvious that a failure will exit the routine before it has begun.

However the counter argument runs something along the lines that we have contradictory guidelines. Two other guidelines are:-

Keep the methods as short as possible. This makes the code easier to read/understand and hence to maintain.

Try to phrase your logical conditions in the positive e.g. IF MONSTER_IS_MAD( ) EQ ABAP_TRUE as opposed to IF MONSTER_IS_NOT_MAD( ) EQ ABAP_FALSE. Again the logic is that the positive form is easier to understand, and hence makes the code easier to maintain.

There is also an implicit rule which is never stated anywhere that code that reads like plain English is easier to understand than code that reads like machine code.

So I am looking at some code in my system right now, and near the end (line 11 out of 22 in the method) it says

CHECK lf_order_cancellation EQ abap_true.

Now as I understand it I am supposed to change this to:-

IF lf_order_cancellation EQ abap_false.

RETURN.

ENDIF.

That violates both of the other two rules - it makes the code longer and changes a positive check into a negative one.

It could be worse. I could be changing

CHECK i_want_to_cancel_the_order( ).

Into

IF i_want_to_cancel_ther_order( ) = abap_false.

RETURN.

ENDIF.

Which turns a statement that reads 100% like plain English into one filled with programming language constructs.

Secondly if the very start (first 3 lines) of my method reads something like

READ TABLE it_partner_details INTO DATA(ls_partner_details) WITH KEY parvw = 'AG'.

CHECK sy-subrc EQ 0.

Then I get a warning because the CHECK is not at the start.

The same if I have EXPORTING parameters that need to be cleared. Typically they get cleared on the very first line, and the CHECK comes on the next line, which also causes the warning.

And how about this one at the end of a method:-

rf_yes_it_has = abap_true.

CHECK lo_initial_state->ms_header_data = mo_sf->ms_header_data.
CHECK lo_initial_state->ms_item_overview_data = mo_sf->ms_item_overview_data.
CHECK lo_initial_state->ms_sales_tab_data = mo_sf->ms_sales_tab_data.
CHECK lo_initial_state->mt_item_details = mo_sf->mt_item_details.
CHECK lo_initial_state->mt_partners = mo_sf->mt_partners.
CHECK lo_initial_state->mt_internal_note = mo_sf->mt_internal_note.
CHECK lo_initial_state->mt_header_note = mo_sf->mt_header_note.

"Nothing has changed since the transaction started
rf_yes_it_has = abap_false.

To follow the rules this must become:-

rf_yes_it_has = abap_true.

IF lo_initial_state->ms_header_data NE mo_sf->ms_header_data.
RETURN.
ENDIF.

IF lo_initial_state->ms_item_overview_data NE mo_sf->ms_item_overview_data.
RETURN.
ENDIF.

IF lo_initial_state->ms_sales_tab_data NE mo_sf->ms_sales_tab_data.
RETURN.
ENDIF.

IF lo_initial_state->mt_item_details NE mo_sf->mt_item_details.
RETURN.
ENDIF.

IF lo_initial_state->mt_partners NE mo_sf->mt_partners.
RETURN.
ENDIF.

IF lo_initial_state->mt_internal_note NE mo_sf->mt_internal_note.
RETURN.
ENDIF.

IF lo_initial_state->mt_header_note NE mo_sf->mt_header_note.
RETURN.
ENDIF.

"Nothing has changed since the transaction started
rf_yes_it_has = abap_false.

How does that make me any better off in life? It has pretty much doubled the length of the method, turned the positive test to negatives and if at a future point I need to add something else that needs to be compared then i would not be able to see it all on one screen any more.

I know this sort of thing can evoke amazingly emotional responses, so i would be really interested to know what everyone else in ABAP programming world thinks about this matter.

Cheersy Cheers

Paul

fred_verheul · ‎2019 Jun 13

My 2 cts: I never take a second look at level 3 information messages from the code inspector (ATC). I don't consider myself being particularly high paid or valued (actually I have no clue), but this is a clear waste of time, hence money.

I'm totally with you with regard to how the code reads (and 'like English' = 'good').

Last but not least: I still hate the fact that we have to 'enhance' our Boolean expressions with 'EQ abap_true' / '= abap_false', but that is for another time 😉

Btw: nice post 😉

Fred

scottlawton · ‎2019 Jun 13

A small counter-point to the argument that CHECK reads more like plain English: While it may be true that the CHECK statement itself reads more like plain English, it is equally true that the CHECK statement does not tell you in plain English what will happen if the check fails.

Overall, even if the code ends up longer, I generally find it clearer to use RETURN when I want to leave a method. But then, I also rarely (never?) have any need to have 8 CHECK/IF-RETURN statements in a single method...

I always appreciate your desire to spark conversation about how to write clean, maintainable code, Paul!

Scott

pokrakam · ‎2019 Jun 14

Hey Paul,

Great blog, but on this occasion I’m not completely onside.

As Scott already suggested, the behaviour of CHECK is context-dependent and thus the statement is unclear. Essentially the use of CHECK is the same principle as EXIT vs RETURN vs the annoyingly named CONTINUE. CHECK can behave like all three and it’s not always immediately clear which.

It’s not so bad in short and simple methods, but if I see a CHECK in the middle of a 400 line module, I have to go and hunt around to see what it does.

And – this is a personal view – I’ve always found CHECK a bit odd. Like CONTINUE, it doesn’t quite express what it does. Check something. Then what?
I would prefer to see CHECK dropped completely and instead extend EXIT and RETURN with

RETURN IF <condition>.

So I’m with the guidelines on this, but I also think in certain instances it can be sensible to bend the rules a little. A READ / CHECK at the beginning of a method is clear and unambiguous. Perhaps the ATC check should validate that CHECK should have no more than n other statements before it (3?).

But then in other cases it makes sense to rewrite the code to make it more readable. You could tuck your multiple CHECK away into something like:

IF sf_contains_data( ).

  RETURN.

ENDIF

By the way fred.verheul: see, no boolean ‘enhancements’
(unfortunately this only works with functional expressions not with variables)

BaerbelWinkler · ‎2019 Jun 14

Hi Paul!

My take is that CHECK-statements on their own sometimes (or perhaps even often?) get overlooked in the code. Which is why I tend to add a clarifying comment above it - something like this:

  "Only continue if invoice items found

  CHECK gv_cnt_inv GT 0.

Combined with an empty line before and after such a CHECK-statement, this makes it a lot harder to get overlooked. It then also doesn't matter all that much if the CHECK-statement is not at or near the top of a routine. On the other hand, a CHECK-statement should obviously happen as soon as possible to avoid having the program do unnecessary work!

I do prefer a clearly stated CHECK-statement as opposed to a perhaps (too) long and nested IF-statement. And yes, I know that "clearly" is in the eyes of the beholder and not a precise definition, leaving quite some wiggle-room!

Cheers

Bärbel

hardyp180 · ‎2019 Jun 14

I have been rewriting some very old code, and have managed to reduce the complexity of some very deeply nested IF statements by using CHECK here and there.

Following the rules would reverse that process.

Peter_Inotai · ‎2019 Jun 14

Totally agree, RETURN IF would be a nice option.

There was already a discussion about this topic on the clean ABAP side:

https://github.com/SAP/styleguides/issues/17

Another option is this one:

IF something_wrong( ). RETURN. ENDIF.

I know it's not nice to have more statement in one line, but for me it's an exception, where it makes sense and makes code still readable and short.

Cheers,

Peter

Peter_Inotai · ‎2019 Jun 14

I think the mentioned statement could be compressed.

Instead of

IF lo_initial_state->ms_header_data NE mo_sf->ms_header_data.

RETURN.

ENDIF.



IF lo_initial_state->ms_item_overview_data NE mo_sf->ms_item_overview_data.

RETURN.

ENDIF.



IF lo_initial_state->ms_sales_tab_data NE mo_sf->ms_sales_tab_data.

RETURN.

ENDIF.

you could just write

IF lo_initial_state->ms_header_data NE mo_sf->ms_header_data OR

   lo_initial_state->ms_item_overview_data NE mo_sf->ms_item_overview_data OR

   lo_initial_state->ms_sales_tab_data NE mo_sf->ms_sales_tab_data.

     RETURN.

ENDIF.

Cheers,

Peter

SuhaSaha · ‎2019 Jun 14

I tend to stick to the rule mike.pokraka has mentioned.

  method CHECK_TAX_POSTINGS_EXIST.

    data: COMP_CODE_RNG type range of BUKRS.



    check I_COUNTRY  is not initial and

          I_TAX_CODE is not initial.



    select 'I' as SIGN, 'EQ' as OPTION, BUKRS as LOW from T001 where LAND1 = @I_COUNTRY

      into corresponding fields of table @COMP_CODE_RNG.



    if COMP_CODE_RNG is not initial. " <<< I will Use IF instead of CHECK  

      select single @ABAP_TRUE from ACDOCA as A

        inner join BSEG as B

        on A~RBUKRS = B~BUKRS and

           A~BELNR  = B~BELNR and

           A~GJAHR  = B~GJAHR

        where A~RBUKRS     in @COMP_CODE_RNG and

              A~XREVERSING = @ABAP_FALSE and

              A~XREVERSED  = @ABAP_FALSE and

              B~MWSKZ      = @I_TAX_CODE

        into @R_EXISTS.

    endif.

  endmethod.

I had encountered a trapdoor when using CHECK inside a macro definition (define... end-of-definition). My assumption was that CHECK would leave the macro & my program would continue as usual.

Suddenly my unit-tests start to fail & alert me about my shortsightedness 🙂

BR,

Suhas

pokrakam · ‎2019 Jun 14

Oooh, another one of those Big Programming Debates, some say a procedure should only have a single entry and a single exit point.

I see it as a nice to have but not if it messes up the flow. Your example works for me because there is no further nesting.

By the way, it's exactly these kind of unintended consequence failures that makes unit testing worthwhile, it's exactly the sort of thing that could easily sneak into production.

UweFetzer_se38 · ‎2019 Jun 14

According to Clean Code / Clean ABAP it should be

*---------------

CHECK number_of_invoice_items > 0.

*---------------

and of course without hungarian notation and without comment, because now it's obvious what the code does. You can decorate the code with lines or something if it's important for you, but one should not describe the code (in my opinion).

chaouki_akir · ‎2019 Jun 16

Taste two: IF / RETURN

  method CHECK_TAX_POSTINGS_EXIST.

    data: COMP_CODE_RNG type range of BUKRS.



    check I_COUNTRY  is not initial and

          I_TAX_CODE is not initial.



    select 'I' as SIGN, 'EQ' as OPTION, BUKRS as LOW from T001 where LAND1 = @I_COUNTRY

      into corresponding fields of table @COMP_CODE_RNG.



    if COMP_CODE_RNG is initial. " <<< I will Use IF instead of CHECK  

       return.

    endif.

    select single @ABAP_TRUE from ACDOCA as A

        inner join BSEG as B

        on A~RBUKRS = B~BUKRS and

           A~BELNR  = B~BELNR and

           A~GJAHR  = B~GJAHR

        where A~RBUKRS     in @COMP_CODE_RNG and

              A~XREVERSING = @ABAP_FALSE and

              A~XREVERSED  = @ABAP_FALSE and

              B~MWSKZ      = @I_TAX_CODE

        into @R_EXISTS.



  endmethod.

Taste three: CHECK

  method CHECK_TAX_POSTINGS_EXIST.

    data: COMP_CODE_RNG type range of BUKRS.



    check I_COUNTRY  is not initial and

          I_TAX_CODE is not initial.



    select 'I' as SIGN, 'EQ' as OPTION, BUKRS as LOW from T001 where LAND1 = @I_COUNTRY

      into corresponding fields of table @COMP_CODE_RNG.



    check COMP_CODE_RNG is not initial. " <<< I will Use IF instead of CHECK  

    select single @ABAP_TRUE from ACDOCA as A

        inner join BSEG as B

        on A~RBUKRS = B~BUKRS and

           A~BELNR  = B~BELNR and

           A~GJAHR  = B~GJAHR

        where A~RBUKRS     in @COMP_CODE_RNG and

              A~XREVERSING = @ABAP_FALSE and

              A~XREVERSED  = @ABAP_FALSE and

              B~MWSKZ      = @I_TAX_CODE

        into @R_EXISTS.



  endmethod.

==> For me the use of CHECK is not a problem and makes the intention clear.

chaouki_akir · ‎2019 Jun 16

Do you apply the Hungarian notation restriction to class method parameter name : no prefix to importing/exporting/changing/returning parameter of methods ?

Michael_Keller · ‎2019 Jun 17

"RETURN IF <condition>" would be a nice addition to ABAP.

UweFetzer_se38 · ‎2019 Jun 17

You got me. You are right, this is the only place where I force my collegues vie ATC to use hungarian notation. The reason is, that we are not quite there, where Clean Code uses ultra small methods (3, 4 or 5 lines) and the parameters where obvious.

I'm restricting coding blocks currently to max. 50 statements, but I'm planning to lower the bar from time to time.

Michael_Keller · ‎2019 Jun 17

I worked in own projects with this coding style. My experience: As long as methods have a manageable size it’s fine and better readable. For a global variable in a report or function group it could be a good idea to use a prefix as “gv_”. If you use the global variable as input for a method parameter, you can give up the prefix.

SuhaSaha · ‎2019 Jun 17

I would not use flavor #2, instead flavor #3 is much cleaner.

As i have mentioned it is a matter of personal taste. I don't use the CHECK because i tend to stick to the rule of using CHECK in LOOPs or at the beginning of a procedure.

Jelena_Perfiljeva · ‎2019 Jun 17

Exactly. First time I encountered CHECK command I was like "check and what?" It's not even clear what it does.

I avoid CHECK personally and yes, never encountered a need for so many, thankfully.

EnnoWulff · ‎2019 Jun 19

Maybe Exceptions are a way to make things cleaner and clearer?

CLASS do_return DEFINITION INHERITING FROM cx_static_check.

ENDCLASS.



CLASS please_return_if DEFINITION.

  PUBLIC SECTION.

    CLASS-METHODS different

      IMPORTING

        this     TYPE any

        and_that TYPE any

      RAISING

        do_return.

ENDCLASS.



CLASS please_return_if IMPLEMENTATION.

  METHOD different.

    IF this <> and_that.

      RAISE EXCEPTION TYPE do_return.

    ENDIF.

  ENDMETHOD.

ENDCLASS.





CLASS demo DEFINITION.

  PUBLIC SECTION.

    METHODS my_check_equal

      RETURNING

        VALUE(all_equal) TYPE abap_bool.



    METHODS my_check_different

      RETURNING

        VALUE(all_equal) TYPE abap_bool.

ENDCLASS.



CLASS demo IMPLEMENTATION.

  METHOD my_check_equal.



    DATA(one) = 'AAA'.

    DATA(two) = 'AAA'.



    DATA(str_lis) = VALUE t001w( werks = '0001' name1 = 'Lissabon' ).

    DATA(str_lon) = VALUE t001w( werks = '0001' name1 = 'Lissabon' ).



    TRY.

        please_return_if=>different( this = one and_that = two ).

        please_return_if=>different( this = str_lis and_that = str_lon ).

        all_equal = abap_true.

      CATCH do_return.

        all_equal = abap_false.

    ENDTRY.



  ENDMETHOD.



  METHOD my_check_different.



    DATA(one) = 'Stan'.

    DATA(two) = 'Laurel'.



    DATA(str_lis) = VALUE t001w( werks = '0001' name1 = 'Lissabon' ).

    DATA(str_lon) = VALUE t001w( werks = '0002' name1 = 'London' ).



    TRY.

        please_return_if=>different( this = one and_that = two ).

        please_return_if=>different( this = str_lis and_that = str_lon ).

        all_equal = abap_true.

      CATCH do_return.

        all_equal = abap_false.

    ENDTRY.



  ENDMETHOD.



ENDCLASS.







START-OF-SELECTION.



  cl_demo_output=>new(

    )->write( |my_check_equal: Everything is equal is {

         SWITCH string( NEW demo( )->my_check_equal( )

           WHEN space THEN `false` ELSE `true` ) }|

    )->write( |my_check_different: Everything is equal is {

         SWITCH string( NEW demo( )->my_check_different( )

           WHEN space THEN `false` ELSE `true` ) }|

    )->display( ).

matt · ‎2019 Jun 21

I believe the restriction is not to get rid of Hungarian notation, but rather to dispose of Hungarian notation for describing the type of the variable (internal table, structure, simple variable). Using it for the usage of the variable is fine (importing parameter, global, constant...).

matt · ‎2019 Jun 21

But some folk say only use exceptions for unexpected events – not for business logic.

Exceptions (in the way you seem to suggest), return, check, continue are all a bit GO TO like. Not quite as bad but apparently “to be avoided”. Which is my approach – not a ban, just avoidance unless it really makes the code easier to understand.

EnnoWulff · ‎2019 Jun 21

As so often: It depends. I see your point. In my opinion, everything that helps making (the main) code shorter and/ or more understandable, is legal. This example with only two "checks" of course is a bit oversized for this "framework". I just wanted to try/ discuss a different way for avoiding CHECK.

matt · ‎2019 Jun 26

One of my favourite errors is to get the sy-subrc check wrong after reading from a table.

READ TABLE meaningful_table_names REFERENCE INTO DATA(table_name)

    WITH TABLE KEY name = 'itab3'.

CHECK sy-subrc IS INITIAL.

MESSAGE 'Hey, numpty, that's not a good table name' TYPE 'X'.

When what I really want is

READ TABLE meaningful_table_names REFERENCE INTO DATA(table_name)

    WITH TABLE KEY name = 'itab3'.

CHECK sy-subrc IS NOT INITIAL.

MESSAGE 'Hey, numpty, that's not a good table name' TYPE 'X'.

And vice versa, and the same if I use an IF statement.

I now have a solution to that:

  METHOD it_was_found. " IMPORTING imp_rc TYPE sysubrc 

    ret_result = xsdbool( imp_rc EQ 0 ).

  ENDMETHOD.



  METHOD it_was_not_found. " IMPORTING imp_rc TYPE sysubrc

    ret_result = xsdbool( imp_rc NE 0 ).

  ENDMETHOD.

So now I can use the much clearer.:

READ TABLE meaningful_table_names REFERENCE INTO DATA(table_name)

    WITH TABLE KEY name = 'itab3'.

CHECK it_was_not_found( sy-subrc ). 

MESSAGE 'Hey, numpty, that's not a good table name' TYPE 'X'.

...

READ TABLE meaningful_table_names REFERENCE INTO DATA(table_name)

    WITH TABLE KEY name = 'fishmongers'.

CHECK it_was_found( sy-subrc ). 

MESSAGE 'Hey, superstar, great table name!' TYPE 'S'.

Works with IF as well.

Edit: It just occurred to me that the methods don't need parameters. I can just test sy-subrc directly. That makes it even more readable.

mauriciolauffer · ‎2019 Jul 10

It's over-complicated and semantically incorrect. You're using exceptions to replace simple conditionals, which doesn't make much sense. We should write clean maintainable code.

joachimrees1 · ‎2019 Nov 27

Nice discussion and insights!
I have in fact recently wondered if check really is to be avoided – I do like it, and would like to keep using it.

I would make a small change: instead of:

check I_COUNTRY is not initial and

      I_TAX_CODE is not initial.

I would write:

check I_COUNTRY is not initial. 

check I_TAX_CODE is not initial.

Why? -> Now it’s two statements, and thus easier to read in debugger.

When the debugger reaches the 2nd line (F5), I know that I_COUNTRY is not initial.

I the initial example, if the debugger jumps out of the method, I will not know if I_COUNTRY or I_TAX_CODE caused that!).

Former Member · ‎2019 Dec 29

I personally like the idea of avoiding CHECK statements at all and use them only at the beginning of a method or a loop as suggested earlier.

In the particular case of the READ statement I prefer to use

IF line_exists( some_table[ key = my_key ] ).

  DATA(line_of_table) = some_table[ key = my_key ].

"... 

ENDIF.

I am not quite sure whether the line_exists() functionality is mentioned in the ABAP Clean Code guidelines. However, at least for me this way works fine.

vonglan · ‎2022 May 11

Related: I recently realized that at the end of every method call, the sy-subrc is set to 0.

CHECK MATE

Get Started with the ABAP Development Tools for SAP NetWeaver

Become an ABAP in Eclipse Feature Explorer and earn the Explorer Badge

Six kinds of debugging tips to find the source code where the message is raised