Overview
In the last few posts we have talked about why to use meaningful identifier names, why to avoid magic numbers, the benefits of organizing our code with functions, and the role of formatting.
Now, I want to talk about the last part of my readability puzzle, the comments.
Comments serve as annotations in our source code, usually we leave these kind of notes to make ourselves and the others remember what the given portion code is actually doing.
However there are cases when these notes creates misunderstandings, for example when it's not up to date with the associated code.
Today Message
Try to refactor before writing comments!
Poor Identifier names
Let's check the following simple example that shows a conditional branch about checking a variable's value, called ms, is '01' and another variable's value, called tb, is 'CH'. In this case, we put the value 30 into the variable, called dp???
What??? Oh, we have a comment that says: "if month of sale is january and type of the boook is children, then the discount percentage is going to be 30 percent". Ok, now I get it!
...
" if month of sale is january and type of the boook is children,
" then the discount percentage is going to be 30 percent.
IF ms = '01' AND tb = 'CH'.
dp = 30.
ENDIF.
...
But I have problems with this solution above. First, usually I hear from developers: I don't struggle with fancy meaningful variable names, it's too much wasted time. Ok, instead write 2 lines of comment for nothing!!! :) So, you saving time with giving poor variable names, but you lose this time on explaining it later with comments and speech.
Secondly, what if the business logic changes to ms = '02', will you adjust the comments also? Maybe. But let's be honest, in most cases we forgot about comments.
Instead, Robert C. Martin's advice is when you feel that you would write an explanation comment (clarifying its behaviour), stop for a second, think a bit, and try to refactor the code (using more meaningful names, and so on), because it can mean that the code smells a bit.
...
IF month_of_sales = january AND type_of_book = children.
discount_percentage = 30.
ENDIF.
...
Dead Code
Another usual comment is the dead code. Alright when we wrote code in Assembly, it was unavoidable to write these kind of comments, but today in 2015 we have fantastic source code and version control systems like SVN, Github, or the SAP's built-in Version Management feature.
So, we don't need to create notes about what we have changed in the source code, the system does it automatically. It's a waste of time, and makes the reader blind during reading the code, because he/she can easily lose the focus.
...
START-OF-SELECTION.
CALL FUNCTION 'GUI_UPLOAD'
EXPORTING
filename = 'C:\customers_with_attribute.xml'
TABLES
data_tab = xml_content_table.
* it's OBSOLETE don't use!!!!
* Mike, 2008.07.01, USA, Boston, ...
* CALL FUNCTION 'WS_UPLOAD'
* EXPORTING
* filename = 'C:\customers_with_attribute.xml'
* TABLES
* data_tab = xml_content_table.
PERFORM convert_to_flat_string USING xml_content_table
CHANGING xml_content_flat.
...
Dead codes usually frustrates the other developers (they don't know why did you leave this comment). When you realize that you are just about leaving dead code, stop for a second, think about SE39, and delete that dead code :).
Explanation Comments
In the case of the following code, we used meaningful variable names, but we still feel that it's not enough and for 100% sure we left an additional explanation comment. Again the problem is with the change, because the code usually changes, but what about the comments? Okay, maybe I could agree with this solution, but there is a much better approach!
...
" convert string table to flat string
LOOP AT xml_content_table INTO xml_line.
CONCATENATE xml_content_flat
xml_line
INTO xml_content_flat.
ENDLOOP.
...
What to do? Simple! Extract the code above into a function (method, function module, subroutine, or something else), called convert_to_flat_string. Now, it's much more readable and transparent! We perform the subroutine, convert_to_flat_string that converts something into flat string. Ok, in ABAP we have limits in namings, but we also can play with the formal parameters: convert_to_flat_string converts a string table into a simple flat string.
...
PERFORM convert_to_flat_string USING xml_content_table
CHANGING xml_content_flat.
...
...
...
*&---------------------------------------------------------------------*
FORM convert_to_flat_string USING xml_content_table TYPE string_tt
CHANGING xml_content_flat TYPE string.
*&---------------------------------------------------------------------*
DATA xml_line LIKE LINE OF xml_content.
LOOP AT xml_content_table INTO xml_line.
CONCATENATE xml_content_flat
xml_line
INTO xml_content_flat.
ENDLOOP.
ENDFORM. " CONVERT_TO_FLAT_STRING
...
Note
When you realize that you are just about writing explanation comments to explain a very complex business logic, then maybe it's time to refactor your code, and extract the different steps, logic into separate functions, because every complex logic can be built up using small building blocks (functions).
Decorator Comments
Use them very sparingly, and only when the benefit is significant! For example I used them in case of complex procedural ABAP programs, or doing tutorials, demos, and trainings, because it can help the reader to understand the code easier.
+ It can help to localize the right place to define a variable, a constant, or write a subroutine (it drives your hand) in case of complex procedural programs.
...
************************************************************************
* GLOBAL DATA DEFINITIONS *
************************************************************************
*----------------------------------------------------------------------*
* CONSTANT - definitions *
*----------------------------------------------------------------------*
CONSTANTS january TYPE string VALUE '01'.
CONSTANTS july TYPE string VALUE '07'.
CONSTANTS september TYPE string VALUE '09'.
*----------------------------------------------------------------------*
* REFERENCE definitions *
*----------------------------------------------------------------------*
DATA alv TYPE REF TO cl_salv_table.
*----------------------------------------------------------------------*
* INTERNAL TABLE definitions *
*----------------------------------------------------------------------*
DATA flight_schedule TYPE STANDARD TABLE OF spfli.
************************************************************************
* GLOBAL DATA DEFINITIONS - END *
************************************************************************
************************************************************************
* SELECTION SCREENS *
************************************************************************
************************************************************************
* SELECTION SCREENS - END *
************************************************************************
************************************************************************
* EVENTS *
************************************************************************
*----------------------------------------------------------------------*
* INITIALIZATON event *
*----------------------------------------------------------------------*
*----------------------------------------------------------------------*
* AT SELECTION-SCREEN ON VALUE-REQUEST FOR event *
*----------------------------------------------------------------------*
*----------------------------------------------------------------------*
* START-OF-SELECTION event *
*----------------------------------------------------------------------*
START-OF-SELECTION.
PERFORM get_flight_schedule.
PERFORM initialize_alv.
PERFORM display_alv.
************************************************************************
* EVENTS - END *
************************************************************************
************************************************************************
* SUBROUTINES *
************************************************************************
*&---------------------------------------------------------------------*
FORM get_flight_schedule.
*&---------------------------------------------------------------------*
SELECT * FROM spfli INTO TABLE flight_schedule UP TO 100 ROWS.
ENDFORM. " GET_FLIGHT_SCHEDULE
...
************************************************************************
* SUBROUTINES - END *
************************************************************************
Summary
Alright, we have reached the end of this post, and also the end of the series. I hope you get the feelings of the readability, and could show you new methods, and approaches!
My readability principles once more:
Stay tuned, keep reading! If you want to get notification about the newest posts, follow me or subscribe to our newsletter!
If you liked it, please share it! Thanks!