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

 

 

[matt]

I had something similar. I shifted the complicated logic into it's own class, with things like

METHOD start_do_something.

  IF first_condition( ).

    this( ).

  ELSE.

    that( ).

  ENDIF.

ENDMETHOD.



METHOD first_condition.

  r_result = ... "first bit of logic

ENDMETHOD.



METHOD this( ).

  IF second_condition( ).

    the_other( ).

...

The actual AND / OR etc. are encapsulated in a method that returns abap_true or abap_false. The processing for happy/unhappy goes in their own methods two.

So I've a whole load of methods with just five lines of code, like the first fragment. It made debugging so much easier, and maintainance so much less error prone.

 

 

 

[Michael_Keller]

You make my day! So other developers experience something like this too and develop real solutions with added value. Overall, this "own class with small methods with good names" has had such a positive impact on my work...

[matt]

Meanwhile I'm back to figuring what this 300 line method does...

[thomas_jung]

Thanks for the entertaining and thoughtful blog post.  As I was reading it and before I reached the section on "Unwanted side effects", I was literally shouting at my monitor: Oh no, the state of sy-subrc isn't necessarily stable. I can only imagine what evil things might be happening inside the logic blocks of the first few checks that then change the value of sy-subrc.

[InvalidUsername]

... “Focus on the happy path or error handling, but not both” from Clean ABAP"...

 

"Clean ABAP" is not a good book on the subject.  Actually quite the opposite.

[Michael_Keller]

Unfortunately, every nightmare can come true inside the logic blocks. At first it looks harmless, then it gets complicated 😉

[Michael_Keller]

Can you cite good sources on the subject? I think other readers will be interested in this as well.

[InvalidUsername]

Paul C. Martins "Clean Code"  has some recommended advises about variable naming, commenting and formatting, (though they pretty basic), but the rest of the book is overrated. The part about functions is bad, the part of objects  - well, I don't know. Seems like a bunch of advise how to handle problems you wouldn't have when you wouldn't have started using OOP.

"Code Complete" by Steve McDonell is worth reading.

"The Pragmatic Programmer" by David Thomas and Andrew Hunt is recommended reading for developers although it is not strictly on the subject of clean code.

"Clean ABAP" is mostly a copy of Paul C. Martin enriched by the usage of "new" ABAP.  Most of the "new" ABAP does not automatically creates better code. Quite often it does the opposite.

One example where "Clean ABAP" gives bad advise: In the linked segment it uses "while" loops, in ABAP almost always an iterative loop is the better choice. In ABAP a while loop is an anti-pattern.

Lot of people believe because its the only book on the subject in ABAP must be a good book. But it isn't. Sad to say, "Clean ABAP" is full of bad advises.

[matt]

First of all Clean ABAP is a set of guidelines. Some will agree, others will disagree. I don't agree with everything in it, so I skip those bits. However, even as a developer for 33 years, I found parts of it very useful.

Secondly, if you're going to say things like "In ABAP a while loop is an anti-pattern", at least explain why so we can benefit from your wisdom. At the moment, we've just got some anonymous person making disparaging comments. It helps no one.

Thirdly, if you're going to reply to a comment, how about clicking on reply below the comment, so we can see the context?

 

[InvalidUsername]

Iterative loops have advantages over while loops

1. Iterative loops are simpler to use and understand than while loops. They have a clear structure and syntax that make it easy to understand how many times the loop will iterate, and what the loop will do on each iteration.


2. The iteration statement (e.g. increment of a counter) is hidden somewhere inside the body of the loop but is needed to understand the loop condition. Sometimes the iteration is nested into an "IF" statement making the loop behavior even more difficult to understand.


3. There is an inherent risk of creating endless loops using while statement which virtually does not exits using iterative loops.

While loops can be used when you have an unknown number of iterations or you need a non-sequential iteration. However, this is almost never the case in database programming. Almost all uses of loops in ABAP can be better solved using iterative loops using the iterative LOOP AT... ...ENDLOOP form. 

While loops have a negative impact on readability, understandability and thus maintainability of the code compared to iterative loops. Clean code is all about these things.

[Michael_Keller]

Thanks for the references. I have had very good experiences with the book "Clean ABAP" in German and English language. Of course also with the GitHub repository. For my way of programming with ABAP, there have been many impulses for positive changes. Thanks again to the authors, including florian.hoffmann und klaus.haeuptle 🙂

[BaerbelWinkler]

keller.m

Hi Michael,

thanks for this fun blog post! I had to laught at this, though: "About 72 lines of spaghetti code with conditions were definitely too much for me."

Here is why:

Just yesterday, I happened upon an old program from the initial setup of SAP in our systems more than 20 years ago. It contains a form routine with almost 1,300 lines of deeply nested, LOOP, CASE and IF-Statements, i.e. Scope: FORM xyz\LOOP\IF\DO\CASE\IF\IF\IF\IF\

As far as I'm concerned, such "Spaghetti-Code-Monsters" are no longer maintainable.

Chees

Bärbel

[matt]

I'll agree with you that using a WHILE loop to iterate through an internal table, or a badly constructed while with lots of complicated logic are code smells, I do not think you've made your case that they are inherently an anti-pattern. Not all iteration involves internal tables. For example, to iterate through exception->previous, I could use:

    WHILE error IS BOUND.

      IF <message> IS NOT INITIAL.

        INSERT INITIAL LINE INTO TABLE x_messages ASSIGNING <message>.

      ENDIF.

      <message>-message = i_error->get_text( ).

      <message>-icon = icon_led_red.

      error = error->previous.

    ENDWHILE.

I supppose I could achieve the same thing using recursion, but that can also lead to eternal loops.

The WHILE examples in the clean code repository don't involve internal tables, so could not be replace by LOOP AT...

Where I've had complicated requirements, I've simply coded an interator in the style of Java.

WHILE iterator->has_next( ).

  DATA(my_object) = iterator->next( ).

   ...

ENDWHILE.

I keep my methods short, and clean. Similarly when I code in Java (where some people vehemently believe while loops are anti-patterns).

Btw, it's also possible to create an endless loop using LOOP AT...

[Michael_Keller]

You don't want to maintain something like that and you don't want to pass it on to your successors. "Spaghetti-Code-Monsters" are endured for far too long. Bitter... 😞

[matt]

With or without meatballs?

[InvalidUsername]

I do think I made my case.

 

"The WHILE examples in the clean code repository don't involve internal tables, so could not be replace by LOOP AT..."

This argument is very self referencing. The problem is that the author choose examples in a way that they use while loops.

Of course you can voluntarily create a case where you need a while loop in ABAP. I just do see any point in doing so in ABAP exactly they eventually require you to write bad code. A data type like a list as you do in your example is complex to handle and to traverse. In ABAP you have arrays with dynamic length aka as internal tables. In C you you don't have these (I don't know Java good enough to tell).

The point is: In other languages while loops might be acceptable, in ABAP they are normally not.

I think the problem is that things learned in Java are applied to ABAP here, as one one the problems in "Clean ABAP" as well, where authors were not willing to unlearn their Java Script way of thinking. When you only have a hammer, everything looks like a nail. So they tried to hammer the ABAP screw into the hole. The result can be seen.

Notes:

 DATA(my_object) = iterator->next( ).

1. I personally find the mixture of data declaration and execution code bad programming style. Clean code is not about developer convenience or using lots of syntactic sugar. Clean typing is essential for clean code. ABAP types are typically defined in SE11, not in the code. They are global. The advantage of this approach seems sometimes to be incomprehensible for developers coming from e.g. JS.


2. If your i_error>get_text() or iterator->next() methods are not idempotent I'd categorize them as smelly as well.

 

[InvalidUsername]

Oh no, the state of sy-subrc isn't necessarily stable.

 

The most typical remedy is

rc = sy-subrc.



If rc = 0.

   select ...

   ...

endif.

if rc = 4.

   ...

endif.

Unless it gets to long using a case statement is an option.

 

[InvalidUsername]

May I ask what your own class is actually abstracting? Or is this one of those ubiquitous "helper classes" which are anything but abstractions of real world objects?

[matt]

Does it matter? The point is to simplify complex logic so that it can be more easily understood, and thereby reducing technical debt.

That applies whether it's an abstraction of a cat, or what you refer to as a "helper class".

 

[matt]

I'd like to see a full critique.

[InvalidUsername]

When you don't have an abstraction of an object, you don't have a method but a procedure or function. When you have a procedure or a function, you should implement them as a a procedure or function, not as a method.

 

[matt]

I do not understand your comment

When you have a procedure or a function, you should implement them as a a procedure or function,

You mean that if it's a procedure it should use FORM/PERFORM and if it's a function it should use a FUNCTION MODULE? Or what do you mean?

 

[Michael_Keller]

Just as you like. The tomato sauce is a bit spicy to slightly hot... 1.641.183 Scoville 😉

[hardyp180]

Robert C. Martin not Paul C. Martin.

 

[Jelena]

Worst of all is when you see multiple SELECTs, READs, etc. and checking of SY-SUBRCs and cannot understand what was even the original intent. Did the author actually mean it as written or is it a bug / oversight? And now no one wants to touch that old code, so it just gets passed to the next generation.

Also, let's not forget the code checking sy-subrc when it's not even set. "Just in case".

[Jelena]

Did you know that one can actually go to GitHub and suggest changes to Clean ABAP or have a discussion with others? Mind-blowing! https://github.com/SAP/styleguides/issues

[ravi_rajput]

This is a fun thread.

[Michael_Keller]

Yes, catgeory "infotainment": some hard and soft facts, mixed with humor and fun. Hope you enjoyed it 🙂

[r_lindemann]

I've long been a fan of "Code Complete" by Steve McConnell (not McDonell), and to some extent I still am. However, the thing is somewhat dated: the 1st edition is from 1993, the 2nd from 2004, and some of the concepts are slightly outdated (e.g. you probably wouldn't advocate methods of 200 lines anymore).
Uncle Bob's "Clean Code" (2009) captures most of the essence of "Code Complete", although IMO the latter is still worth reading.

I do agree that "Clean ABAP" is largely the concretization of what is currently considered best coding practices (and yes, Uncle Bob is a prime source for these). What I do not understand: why should this be bad? In the past, every organization had its own internal version of ABAP standards, often driven by the preferences of a single individual in that organization. Having a platform that aims to standardize these standards, allowing and encouraging an open discussion, is great. So, if you don’t agree with its content, go and contribute!