CHECK MATE

[hardyp180]

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