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.
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( ).
IF i_want_to_cancel_ther_order( ) = abap_false.
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.
IF lo_initial_state->ms_item_overview_data NE mo_sf->ms_item_overview_data.
IF lo_initial_state->ms_sales_tab_data NE mo_sf->ms_sales_tab_data.
IF lo_initial_state->mt_item_details NE mo_sf->mt_item_details.
IF lo_initial_state->mt_partners NE mo_sf->mt_partners.
IF lo_initial_state->mt_internal_note NE mo_sf->mt_internal_note.
IF lo_initial_state->mt_header_note NE mo_sf->mt_header_note.
“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.
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 😉
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!
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.
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
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:
By the way Fred Verheul: see, no boolean ‘enhancements’
(unfortunately this only works with functional expressions not with variables)
Totally agree, RETURN IF would be a nice option.
There was already a discussion about this topic on the clean ABAP side:
Another option is this one:
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.
"RETURN IF <condition>" would be a nice addition to ABAP.
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:
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!
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
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.
you could just write
I tend to stick to the rule Mike Pokraka has mentioned.
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 🙂
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
Taste three: CHECK
==> 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.
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:
I would write:
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!).
Maybe Exceptions are a way to make things cleaner and clearer?
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.
When what I really want is
And vice versa, and the same if I use an IF statement.
I now have a solution to that:
So now I can use the much clearer.:
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.
Related: I recently realized that at the end of every method call, the sy-subrc is set to 0.
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
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.