Skip to Content
Technical Articles

CHECK MATE

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_detailsWITH KEY parvw ‘AG’.

CHECK sysubrc 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

23 Comments
You must be Logged on to comment or reply to a post.
  • 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

  • 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

    • 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.

  • 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)

  • 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

     

    • 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.

    • 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).

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

        • 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.

        • 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.

        • 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…).

  • 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

  • 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

    • 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.

    • 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.

      • 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.

  • 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( ).
    • 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.

      • 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.

    • 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.

  • 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.