Magic numbers rule?

[DGRHicron]

I can't find a rule which checks if one uses hardcodes such as magic number comparisons.

For example:

IF moo > 2.
  WRITE 'boo'.
ENDIF.

Here value 2 is magic. Noone knows what it is. Is there a rule for that?

another example:

IF moo = 'SOME_TEXT'

[larshp]

currently no rule for that

[larshp]

@DGRHicron the examples are when the values are used in compare conditions? how about passing a value to a method, are these also considered magic?

[DGRHicron]

I found a piece in Clean Abap just about comparisons and naming constants meaningful.
https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#use-constants-instead-of-magic-numbers

But if you ask me, any hardcode is bad. Where used is very powerful and useful function. I hate scanning code by rs_abap_source_scan to find if a "constant" is used as hardcode.

Of course this rule should have the possibility to disable it for test classes.

[DGRHicron]

Ok. I see that I am mixing two rules.
One is the rule for magic numbers. And I think you are right that sometimes when you call a method number passed as parameter is not considered magic.

This example for me is ok.
write_n_lines( 5 ).

I tried to come up with a counter-example as the wrong one, but couldn't find one that was convincing enough.

The second rule is for checking hardcodes of text.

SELECT ... WHERE some_field = 'BLABLA'

[pokrakam]

I'm not convinced a rule is necessary, or would be very difficult to get right as it can be subjective and context-dependent.
SELECT ... WHERE status = 'ERROR' doesn't need a constant.
SELECT ... WHERE status = '74' does.

I also don't believe hardcoding is bad. We used to go by the principle of wrapping everything in constants, but with refactoring tools this is not necessary. A clear single-use value doesn't need a constant.
My approach is to start as local as possible (literal) and expand scope as needed:

• If used once and clear meaning -> literal
• ... if not clear -> constant
• When it is used a second time in module -> quickfix - extract to local constant
• When it is used in another method -> quickfix convert to class attribute
• When it is used in multiple classes -> convert to static or possibly DDIC if used in UI

[DGRHicron]

@pokrakam, I assumed that abaplint's origin is somehow connected with Clean Abap guide. That's why I opened this issue. Here is the link to the constants section. https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#constants

[larshp]

abaplint pre-dates SAP's Clean ABAP guide, abapOpenChecks pre-dates abaplint

[pokrakam]

@DGRHicron I am not contradicting Clean ABAP. A Magic Number is a literal token (not necessarily a number) where it is not obvious why that value is used or where it comes from, it appears derived by magic.
In my example 'ERROR' is not magic, '74' is.

The Clean Code book also explains that constants are not needed if it's clear:

Some constants are so easy to recognize that they don’t always need a named constant to hide behind so long as they are used in conjunction with very self-explanatory code. For example:
int dailyPay = hourlyRate * 8;
double circumference = radius * Math.PI * 2;

Martin, Robert C.. The Robert C. Martin Clean Code Collection (Collection) (Robert C. Martin Series) (p. 435). Pearson Education. Kindle Edition.

A method name can also be a valid way of describing a literal.

METHOD get_root_node. 
  result = nodes[ id = '0000' ].
ENDMETHOD.

All of which brings me to the opinion that it would be very difficult for abaplint to handle magic numbers properly and I'd probably just disable such a rule.

[DGRHicron]

It is implemented in code_pal for only few cases:
https://github.com/SAP/code-pal-for-abap/blob/master/docs/checks/magic-number.md