anti-pattern "SY-SUBRC fragmentation"

[Michael_Keller]

Dear community, I recently had to deal with an older source code because of an error. The understanding of the code was unnecessarily difficult for me due to some "strange" SY-SUBRC comparisons. The following pseudo-code will hopefully give you a better understanding of what I had to deal with.

SELECT SINGLE * FROM ztable WHERE value EQ 'X'.



IF sy-subrc = 0 AND ... " complex comparisons

... " 1 line of code

ENDIF.



IF sy-subrc = 0 AND ... " more complex comparisons

... " 43 lines of code

ENDIF.



IF sy-subrc = 0. " surprising simple comparison

... " 1 line of code

ENDIF.



IF sy-subrc <> 0 AND ... " insane comparisons

... " 10 lines of code

ENDIF.



IF sy-subrc <> 0 AND ... " please don't ask

... " 6 lines of code

ENDIF.

Thanks to the collapse/expand function of conditions in the development environment, I was able to better understand the many lines and sections of code.

Now what was so bad and what could be done better?

So if we can talk about it... 😉

Unnecessarily frequent querying of SY-SUBRC

In this context, 5 comparisons to SY-SUBRC are clearly too many and also not necessary. The following variant would have been easier to understand because it focuses on the variable SY-SUBRC and requires only two mental distinctions.

IF sy-subrc = 0.

" happy path 

ELSE.

" unhappy/sad/bad path

ENDIF.

Please note that the differentiation between "happy and unhappy path" is not necessarily a good/bad distinction. Both paths can be legitimate flow paths. In this constellation, it must be clarified whether an entry is required in the database table ZTABLE or whether this is optional.

Another point in this connection is the recommendation "Focus on the happy path or error handling, but not both" from Clean ABAP.

Unwanted side effects

In the example above, the SY-SUBRC of the SELECT does not decide on the program flow as it may seems. The frequent querying of SY-SUBRC as shown always refers to the last successfully or unsuccessfully executed ABAP statement that influences SY-SUBRC.

In my example, the code worked out of sheer luck. For a developer who adds code here, however, there can be more than unpleasant surprises from side effects.

The situation can be illustrated using the following code (there is no language with the language key "k" but "d" exists). As result, variable SWITCH is set to ABAP_FALSE. Just add some lines that bring SY-SUBRC not equal 0 after the second SELECT and things will be different 🙂

DATA switch TYPE abap_bool VALUE abap_true.

DATA language TYPE I_Language.



SELECT SINGLE * FROM I_Language WHERE Language = 'k' INTO @language.



IF sy-subrc <> 0 AND switch = abap_true.

  SELECT SINGLE * FROM I_Language WHERE Language = 'D' INTO @language.

ENDIF.



IF sy-subrc = 0 AND switch = abap_true.

  switch = abap_false.

ENDIF.

Far too much code at once

In my experience, you can work much more effectively every day if you work with small pieces of code that are quick and easy to understand. I can look at many of them during the day. On the other hand, a piece of source code like the example above costs me an incredible amount of time and energy, and in the end I'm more exhausted.

About 72 lines of spaghetti code with conditions were definitely too much for me. Method calls with clear names and signatures would have helped me here to quickly identify the relevant information for understanding.

Insane comparisons

It might be a good sign that something is wrong when conditions are written across multiple lines, have more parentheses than an onion has layers and are combined with AND, OR and more. This topic is described in chapter "Consider decomposing complex conditions" of Clean ABAP. To be fair, the recommendation from Clean ABAP used to be difficult to implement in older SAP NetWeaver releases: You had to work with several conditions one after the other (increasing the nesting depth).

 

Many thanks for reading and stay healthy

Michael