Some thoughts about commenting ABAP code
Do your write code only for machines, or also for your fellow developers and curious functional consultants?
I believe that readability of program code is an important long-term aspect. I try to have it as a big priority and I’m constantly trying to improve my ability to use naming and composition to make the code as self-explanatory as possible. Comments still have their place though, but should not be what the code is based upon.
The online Clean ABAP style guides has a good section on comments, as does the new Clean ABAP book.
I recommend both those sources with all my heart, but let me elaborate here as well how we can write code for humans and machines at the same time…
Four quadrants of code and comments
So if we take for one aspect the naming of things in our code, like variables, classes, methods, etc. We define a very simple 2 grade scale grading how well the naming explains the thing, from ‘Nope’ to ‘Very much so’.
For another aspect, we take the degree to which comments are used to explain the coding. Again with the same 2 grade scale.
Let’s put this in four quadrants for a simplified world.
Let’s get to it then…
1. Nope – Nope
First example is really really common – no comments and no good naming. Probably work ok right when you type it but even after a week, you yourself might not have a clue what your were up to:
lr_tma = go_asmnt->todo( ). ln_pt = 2. lr_tma->find_impnt( ln_pt ). WHILE NOT lr_tma->complete( ln_pt ). go_asmnt->fn_main( lr_tma ). ENDWHILE.
Hey, I made this up myself and I’m not sure I get it… 😀
2. Very much so – Nope
All right, so clearly we need to improve on that one… Lets add comments to explain the code:
" Get pending tasks for assignment lr_tma = go_asmnt->todo( ). " Sort the tasks in order of priority (sort type = 2) ln_pt = 2. lr_tma->find_impnt( ln_pt ). " Execute all of the tasks, one at a time, in order WHILE NOT lr_tma->complete( ln_pt ). " Execute go_asmnt->fn_main( lr_tma ). ENDWHILE.
Now, finding this code 6 months later, at least we know where to look for maintenance and what it’s supposed to do… It’s much better…
But are we there yet? No, there are still better ways in my subjective opinion…
3. Very much so – Very much so
Now, if we instead fix it up a bit and rename our variables, objects and methods…
" Get pending tasks for assignments tasks = assignments->get_pending_tasks( ). " Sort the tasks in order of priority tasks->sort_by_priority( ). " Execute all of the tasks, one at a time, in order WHILE NOT tasks->is_empty( ). " Execute assignments->execute_next( tasks ). ENDWHILE.
Now the naming is explaining the code and the comments are explaining the code. Ok, all good? Twice as good? Well. Yes but no. Since the coding basically says the same thing as the comments now, we need to read the same thing twice… And it also has to be written twice… And changed twice when making adjustments or adding features…
And what if the comment and the code is not saying the same thing? Do we trust the comment or the code?
Let’s carry on to the final one and my personal favorite…
4. Nope – Very much so
tasks = assignments->get_pending_tasks( ). tasks->sort_by_priority( ). WHILE NOT tasks->is_empty( ). assignments->execute_next( tasks ). ENDWHILE.
Ah! The cleanliness. Quick to read. Quick to get an overview. Understandable code.
Just as Jules is trying real hard to be the shepherd in Pulp Fiction, I try real hard to write this kind of code. I’m not where I want to be yet, but I’m trying real hard…
5. What, that’s it? Just like that? No comments ever?
Well no. First, there are a couple of important prerequisites for this… One prerequisite is a good composition of the code, the division of code into clearly defined and scoped classes and methods. If your code is 1500 lines in a procedural style, then naming things well certainly help, but you still risk getting lost anyway.
And a prerequisite to do that well is to use ABAP OO, which we all should whenever possible!
Another is context. I mean in the above example there are still many things we can’t say other than that this code is supposed to be prioritizing and executing. Hopefully it’s clear from the context where the code is located what an assignment is and what kind of tasks we’re talking about, for instance.
Comments are still a very good way to explain WHY something is done in a certain way, that might not be obvious… Something like this, for example:
" For some reason, the BAPI_CREATE_SOMETHING is not accepting item_id " to be '0001' when creating a new object and assigning items at the " same time. We use '1337' to prepare the data instead. This is fine. items = VALUE #( ( item_id = '1337' description = some_data->title value = some_data->value ) ).
(It’s funny how the styling of the code blocks here are syntax highlighting inside the comments…)
Now, if only the reality was this simple… 🙂
Stay safe everybody and let’s hope to understand each others code…
Addition: To keep reading on this topic in the SAP Community, I recommend continuing with I See Dead Code after getting the extra value from the comments below.
My favorite subject 🙂 So much can be achieved with little effort if you choose names appropriately.Although choosing the right name sometimes takes a long time. At least for me. But it's worth it.
I cannot live completly without comments in my source code. I don't want that either. But I know that a comment is really important when I see it 😉
By the way: I work a lot with the ABAP Development Tools. If you write methods in it, the short text is missing in the SE24 and also in the ADT via F2 (element information). That's why I now use ABAP Doc to give some methods a synchronized english short text. But only short text and it must fit in one line 🙂
Yes, simple isn't easy... 🙂
Good point - save the comments for when they're needed. "Don't cry wolf", that's a global saying, right?
Exactly, as I typically joke "there should be no comments in the code", above illustrates this perfectly 🙂
I've seen cases where I wonder if there's any code among the comments... At least the intention there was right, though...
Very well written. Unfortunately with comment to code relation checks the ‘what is the following code doing’ (or worse method history with change log.. Brrrr…) comment style is enforced. Let’s hope for the day where the last one was convinced that this measure doesn’t make any sense
Thanks. Yes let's work on having that day come sooner!
I remember being told I had to comment a bit of code not much more complex than this.
I asked "what comment would you like me to put in"?
Change logs -> yes, they became part of the standard in my last year at my previous place of work. I'm glad I left.
Can relate. I ran into this requirement with the ATC check in one project: code must have X comments per X lines or something. Honestly, I don't understand why this is necessary and what is the logic behind it. Sometimes short code may need a comment and sometimes longer code is self-explanatory. Whether to comment or not has nothing to do with the code size. (I have yet to see any research that would find otherwise.)
I wish more SAP customers would look into practicality and "cleanliness" of their ATC checks instead of just blindly adopting something. Otherwise it defeats the purpose of having any quality check.
My comment to your code without comment would be "ante up, use an iterator":
Even in this code, I would like to add a comment asking if assignment is really the right object to be responsible for executing a task. So maybe I would add a dimension "comment explaining the abstraction".
But I like your quadrants simplification very much.
Yes, Jacques, you have a perfect case-in-point here, thank you! Context is really necessary for understanding the code. Depending of what assignment is and what tasks are, your implementation with the iterator pattern might be the perfect solution. And assignment and tasks may be totally wrong names. 🙂
Interesting discussion, thanks for starting it Jörgen!
I like to put comments into code (and may be overdoing it sometimes!), mostly to explain why something is done. Here are two examples from a program I recently wrote utilizing the RTTS-framework for the first time.
Without comments like these, I wouldn't remember for long what and why I did there!
Note: I had copied the RTTS-logic from one of the DEMO-programs which used prefixes and form-routines and as that already required some "wrapping my head around" about what's going on there, I didn't dare try to convert this to ABAP-OO and mess with the names used. And, I wasn't even sure if ABAP-OO would have been an option for RTTS given the dynamic nature of the structures used.
Thank you for the real world actual examples.
I totally agree with the why-comments... And I really like you also adding the links in there for more information, to give that extra added value.
I had only two axis in the simplified worldview I described, but I guess another one could be "complexity" or something. Like "Is this understood by the majority of ABAPers or do I need to explain this command or what I'm doing here?". For instance, many developer are not used to the VALUE operator yet, but I think that one is pretty straight forward. Just a F1 away. But I guess it can be easy to get carried away and writing an overly complex statement using the REDUCE operator...
With your example here, (notwithstanding your caveats), I would have modularised like this:
And then adjusted the WHY comments accordingly.
I see a comment before a block of code as an indicator that perhaps the code should be in its own method. I generally try to do that whenever it turns up. It’s no always quite so easy, so with established code, sometimes I just have to leave it.
I do put comment lines above SE91 messages used in code with the text. It's handy to have some indication. Of course that leaves the possibility that the message will change and the code won't...
There is a specific rule against that in the clean code guidelines, but I agree with you on this one (as well). It's true that the message details and phrasing might change. But I consider those as explaining the why rather than indicating the exact error message text. "Ok, so we're issuing a MESSAGE here, why is that?". It's not always obvious for instance if no match from reading a table is totally fine or if it's really really bad.
And if messages are repurposed to mean a totally different thing later on, well, then there are bigger problems... 🙂
This seems like a nice opportunity to pimp my not so old blog I See Dead Code. The "Begin of changes... end of changes" fall somewhere near 0 in your quadrant. 🙂
"Narration" of the code is my second pet peeve ("*Here I'm selecting data from database" followed by SELECT). This argument: "the comment and the code is not saying the same thing" should, hopefully, hit the last nail in its coffin.
"Program is a living organism and dead code is as useful as a gangrene. Let it rest in peace in the version control."
I'll actually go ahead and add a link to your post above, in case somebody misses these comments... But I hope nobody does, as there is great extra value in the comments both here and in your post. Thanks!
Maybe I would use this:
assignments->execute_pending_tasks( im_sort = by_priority ).
If the code is well organized and named then comments can become easily just a duplicate of code and then I would rather omit them. Sometimes it is not easy to create such a code and some help from comments is needed. One problem with SAP I see is that I have only 30 chars long names, which is short if we take into account that we need to use some prefix also (like ZCL_ ...here 4 chars are lost...and there are even teams which use much longer prefixes because of their local dev standards and it can happen that you have only 20 chars remaining for some meaningful name). One reason why it is difficult to create meaningful names is that this require much of thinking to create good design. It is close to impossible to find a meaningful name if ta code design is low quality.
Yes, it's not easy...
And even harder is naming database tables... 16 characters is not much in the vast open global space where they all share space...
I find that teams that require long prefixes need to be told that sub-packages exist in SAP. Other than ZCL... or ZCX... that are mandated by SAP there really shouldn't be anything else required.
Don't forget that a lot of companies have coding conventions & guidelines. Some are more outdated as others, but in my experience, there is no easy way to convince those customers to change the guidelines..
So often the team has not a lot of room to maneuver, and often more important battles to fight. Little that can be done about that I guess.
Can relate to that. 🙂
Since object namespace isn't available in ABAP repository, many times adding a (short) prefix is inevitable in order to avoid of a complete mess.
Good point, thanks!
I agree to 100%. - I am using Regex very often. Regex is a great example where we need comments for better understanding.
Hah! Yes, it doesn't take many characters for a regular expression to be hard to understand.
Agreed on the necessity to comment that one if it's not encapsulated in a super well named single method, which will probably never be the case.
Out of curiosity, do you tend do document only what it does, or also the explanation like
I mean I see the benefit of the explanation but for a longer string, and if in need of maintenance, how about this? 😀
Jörgen, thanks for info about regex101.com. I pasted the regex above there and it provided nice explanation.
Oh I liked the four quadrant approach. Never thought about it this way and it's so easy to visualize and use it. I think much of this variable naming and commenting discipline starting with when we start learning the programming language. I remember the times in my QA reviews when naming a variable as l_count meant that we are counting something and then invariably getting into l_count1, l_count2 and so on often forgetting what that count was meant for. Same for l_flag. I also think that on occasions where our only requirement is do a bug fix in some incident management engagement and our scope is maybe writing 10-15 lines of code, the programmer should make an attempt to write them in as clean manner as possible ignoring how the remaining program was written.
Couldn't agree more on the "WHY something is written" instead of the narrative.
Thanks for sharing your thoughts
Yes, old habits die hard. And if we don't think about it, we'll probably just keep going...
Thanks for your thoughts!
Fully agree about choosing a meaningful class/method name, however the 30 character limit (minus prefixes and underscores) is usually an issue.
On the side note, I still see lots of "comments" like this:
Yes, and even harder is naming database tables in max 16 characters...
On one of my previous projects there was a 14 character prefix for DB tables. I had to rename a DB table twice, because the client said, I was not able to find a meaningful name for the remaining 2 characters 🙂
Here is an example for a comment where I really would have appreciated some clues as to "Why"?!?
ZEHS_ITAB_NEU is a dictionary defined structure but a where-used didn't turn up anything apart from the code I was in, namely a form-routine in an enhancement related to delivery processing (VL01/02/03). For the change I needed to apply, a new field was needed in this structure which for various reasons I couldn't quickly do right away in the dictionary. As there was no explanation for the "don't remove!" comment, I tried with putting the structure directly into the form routine as a local data definition. This at least didn't cause any syntax errors but it caused some information on the delivery note to go AWOL, namely some long texts. Digging deeper, it turned out that the structure was used in SO10-long text definitions with some SAPScript logic thrown in for good measure. This logic only works with the structure being a global work area defined via the TABLES-statement.
Would have been nice to see this spelled out in a comment (I think)!