Good Constant / Bad Constant

[raghug]

Hi Folks,

I think I can get fairly rapid consensus on this here, but I run into this so often I thought I will bring this up for "discussion" again. I tried searching around and while there has been a lot of generic discussions, I haven't seen many robust examples yet. I have also seen so many code reviewers missing the point of the existing discussions and SAP documentation (Literals) - I want to see if I can word this question / discussion point in such a way that it provides enough examples - good and bad - about how constants should be used and when literals are alright.

First here is some real code from a productive system (I have obfuscated some of the code)...

CONSTANTS: c_enhname   TYPE z(obfuscated) VALUE 'OBFUSCATED',

             c_b TYPE c VALUE 'B',

             c_e TYPE c VALUE 'E',

             c_n TYPE c VALUE 'N',

             c_x TYPE c VALUE 'X',

             c_y TYPE c VALUE 'Y',

             c_08(2) TYPE c VALUE '08',

             c_12(2) TYPE c VALUE '12',

             c_261(3) TYPE c VALUE '261'.



...

" Example 1 

FOR ALL ENTRIES IN lt_vekp

                WHERE handle = lt_vekp-handle AND

                      ( object = '08' OR object = '12' ).



...

" Example 2

SELECT * FROM (ztable)        "Obfuscated for Public consumption

         INTO TABLE lt_zrf

         WHERE aufnr = process_order AND

               ( status = c_y OR status = c_b ) AND

               zflag = ''.



...

" Example 3

SELECT (obfuscated field list)

  FROM resb INTO TABLE lt_resb

       WHERE aufnr = process_order AND

             bwart = c_261. 

I think the statement about declaring constants with meaningful names will not be met with much resistance. In example 1, the object '08' and '12' are used several times in this code. The fact that C_08 and C_12 mean nothing and the code has not been commented anywhere means that no one knows what this means at all!

In this case especially since the constants are used in several other places I would absolutely recommend that the programmer use a properly named constant like below.

CONSTANTS: 

             c_work_order_link(2)      TYPE c VALUE '08',

             c_unused_handling_unit(2) TYPE c VALUE '12'.

...

" Example 1 

  FOR ALL ENTRIES IN lt_vekp

                WHERE handle = lt_vekp-handle AND

                ( object = c_work_order_link OR object = c_unused_handling_unit ).

In example 2, the constants are used only in 1 place in the entire code. Frankly, I don't know what the Y and B mean any more, I just found this in an old email - I guess that is a perfect example of poorly documented code. In this case - queue first debatable point - I argue that there are 2 ways that are equally good. There are no real benefits either way if you know this code is never going to use that constant ever again.

CONSTANTS: 

             c_baffled TYPE c VALUE 'B',

             c_yoyoing TYPE c VALUE 'Y'.



" Example 2

SELECT * FROM (ztable)        "Obfuscated for Public consumption

         INTO TABLE lt_zrf

         WHERE aufnr = process_order AND

               ( status = c_yoyoing OR status = c_baffled ) AND

               zflag = ''.

"... OR...

SELECT * FROM (ztable)        "Obfuscated for Public consumption

         INTO TABLE lt_zrf

         WHERE aufnr = process_order AND

               ( status = 'Y' OR status = 'B' ) AND   "Yoyoing or Baffled

               zflag = ''.

The 3rd example, I believe is similar to the above - but is closer to an example of where the semantic meaning is pretty obvious to anyone who knows enough to deal with goods issues to an order. Pretty much everyone in this space would know what a 261 movement is. Of course, one could argue that a finance centric ABAP'er might be poking around and not know what a 261 is - I would argue back at that point that telling them it is a goods issue doesn't mean much more to that person without providing a lot more context to them.

The last and most egregious aggravation is the C_X. The same constant is then used in several places in the program to mean True, Deleted, Empty and Used up. I think this is worst example of constant usage. Though using a literals in multiple places is not the best practice, I would argue that literal usage is the lesser of two evils. Of course, the best approach would be to have separate constants for each usage.

CONSTANTS:

  c_true    TYPE c VALUE 'X',

  c_deleted TYPE c VALUE 'X',

  c_empty   TYPE c VALUE 'X',

  c_used_up TYPE c VALUE 'X'.

This way, the code in individual places is made more readable. In future, for what ever reason, if one of those constants has to change, then just the one can be changed without effecting any of the other code. For instance if one of the tables grows to have additional meanings on the flag, and C_EMPTY becomes 'E' and the 'X' in that case comes to mean C_DO_NOT_USE. You will be able to make that change without changing all the other constants.

Note: I know there have been several wonderful discussions about naming conventions and the use of classes / interfaces for constants. I have a feeling that somebody will try to bring that up in the comments - I am going to try to pre-empt that by saying, I believe those are different discussions of equal importance, but not what I am trying to discuss here.