anti-pattern “SY-SUBRC fragmentation”
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.
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
I had something similar. I shifted the complicated logic into it's own class, with things like
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.
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...
Meanwhile I'm back to figuring what this 300 line method does...
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?
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".
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.
I do not understand your comment
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?
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.
Unfortunately, every nightmare can come true inside the logic blocks. At first it looks harmless, then it gets complicated 😉
The most typical remedy is
Unless it gets to long using a case statement is an option.
"Clean ABAP" is not a good book on the subject. Actually quite the opposite.
Can you cite good sources on the subject? I think other readers will be interested in this as well.
I'd like to see a full critique.
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.
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?
Iterative loops have advantages over while 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.
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:
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.
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...
I do think I made my case.
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.
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
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 🙂
Robert C. Martin not Paul C. Martin.
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!
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.
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... 🙁
With or without meatballs?
Just as you like. The tomato sauce is a bit spicy to slightly hot... 1.641.183 Scoville 😉
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".
This is a fun thread.
Yes, catgeory "infotainment": some hard and soft facts, mixed with humor and fun. Hope you enjoyed it 🙂