false positives: Prefer IS NOT to NOT IS

[jmgrassau]

Hello,

this check creates false positives for code like
IF NOT is_loaded( ).
where is_loaded( ) has an ABAP_BOOL returning parameter; or, for a more complex example,
IF is_new( ) AND NOT is_loaded( ).

I don't think the code would be better readable if I changed it to
IF is_loaded( ) = abap_false.
IF is_new( ) =abap_true AND is_loaded( ) = abap_false.
or is that what Clean ABAP recommends in such a case?

Similarly, the check creates messages for code like
IF NOT ( a < b AND c < d OR a > b AND c > d ).

Of course, you could replace this with
IF ( ( a >= b OR c >= d ) AND ( a <= b OR c <= d ) ).
but usually when if saw the IF NOT ( … ) syntax, there was a good reason for it, and the logical expression was easier to understand that way.

Anyway, such cases do not quite fit to the error message "Prefer IS NOT to NOT IS". Is this check simply searching the code for "IF NOT"? Maybe it could at least restrict the findings to cases where an actual "IS" appears somewhere in the logical expression …

Kind regards,
Jörg-Michael

[lucasborin]

Hi Jörg-Michael,

this check creates false positives for code like
IF NOT is_loaded( ).
where is_loaded( ) has an ABAP_BOOL returning parameter; or, for a more complex example,
IF is_new( ) AND NOT is_loaded( ).

I don't think the code would be better readable if I changed it to
IF is_loaded( ) = abap_false.
IF is_new( ) =abap_true AND is_loaded( ) = abap_false.
or is that what Clean ABAP recommends in such a case?

If you check the Clean ABAP, it recommends you to make the conditions positive.

Therefore, IMO, it is not a false positive. You could make it easy to read if you do, for instance:

• IF is_unloaded( ). instead of IF is_loaded( ) = abap_false
• IF has_instance( ). instead of IF is_new( ) = abap_false.

Similarly, the check creates messages for code like
IF NOT ( a < b AND c < d OR a > b AND c > d ).

Of course, you could replace this with
IF ( ( a >= b OR c >= d ) AND ( a <= b OR c <= d ) ).
but usually when if saw the IF NOT ( … ) syntax, there was a good reason for it, and the logical expression was easier to understand that way.

Of course, it is no that simple to decide it without knowing the whole logic around. But, I bet in the make the conditions positive again, and also in the consider decomposing complex conditions to make it easy to read.

Anyway, we could enhance the documentation to suggest them.

Anyway, such cases do not quite fit to the error message "Prefer IS NOT to NOT IS". Is this check simply searching the code for "IF NOT"? Maybe it could at least restrict the findings to cases where an actual "IS" appears somewhere in the logical expression ...

Unfortunately, it is not that simple. There are some possible values for methods that return boolean. Please, take a look at the Method Name Misleading for Boolean Return Check. Plus, the above arguments.

Feel free to share your inputs.

[jmgrassau]

Hi Lucas,

thanks for your reply. So, are you really suggesting that if I have an is_loaded( ) method (and use it somewhere for an IF is_loaded( ) condition), and later, in a different place, I need logic for the unloaded case, I should then also add a

METHOD is_unloaded( ).
  IF is_loaded( ).
    result = abap_false.
  ELSE.
    result = abap_true.
  ENDIF.
ENDMETHO.

on top of it, just to avoid writing IF NOT is_loaded( ) and getting an error from the "Prefer IS NOT to NOT IS" check? So, Clean ABAP would mean to basically implement all your methods with Boolean return values as twins, in case someone needs the negation?? And ABAP itself would need extra predicate functions like line_does_not_exist( ), contains_no( ) and does_not_match( ), to prevent us from writing IF NOT line_exists( ) etc.?

The anti-pattern shown in the make conditions positive section is a double negation IF has_no_entries = abap_false., and that's of course to be avoided. However, the problem there starts with having an identifier like has_no_entries – which is negatively phrased, just like is_unloaded …

I really see the benefit of this check for examples like those provided in the check documentation, and the check has already helped us to find lots of IF NOT … IS BOUND in our code and change them to IF … IS NOT BOUND (which is much better readable!) But if people need to put pseudo-comments for IF NOT is_loaded( ), then I am afraid, this otherwise useful check will get deactivated.

Kind regards,
Jörg-Michael

[jdgx]

Hi Lucas, hi Jörg-Michael,

I also came across this "false positive" and I share Jörn-Michael's opinion. In particular a simple statement like

IF NOT boolean_method(...).

should not produce a positive check result. And I also think that this is such a common scenario, that developers should not be pushed to "decorate" it with a pseudo-comment or disable the check completely.

If there is a (relatively) simple solution to exclude IF NOT boolean_method(...) in general, I would definitely go for it. Implementing more elaborate checks (e.g. checking method/variable names for double negation, checking for IS, = or <> operators) could look like a nice-to-have feature at first glance, but I would not see it as essential.

Best,
Jonathan

[lucasborin]

Hi Jörg-Michael, sorry for my late reply here.

So, are you really suggesting that if I have an is_loaded( ) method (and use it somewhere for an IF is_loaded( ) condition), and later, in a different place, I need logic for the unloaded case...

The anti-pattern shown in the make conditions positive section is a double negation IF has_no_entries = abap_false., and that's of course to be avoided. However, the problem there starts with having an identifier like has_no_entries – which is negatively phrased, just like is_unloaded …

Based on the Clean ABAP:

• We should avoid the "negation twice" like IF has_no_entries( ) = abap_false;
• We should avoid the empty if like IF has_entries( ) = abap_true. 'nothing' ELSE. 'logic' ENDIF;
• We should use the "negation" at the end of the condition instead of at the beginning like IF variable = abap_false instead of IF NOT variable;
• We should use abap_true and abap_false for comparisons.

Ref: Try to make conditions positive, Prefer IS NOT to NOT IS, and Use ABAP_TRUE and ABAP_FALSE for comparisons.

Therefore, let me fix my last reply. I mean, instead of IF NOT is_loaded( ) we could use...

• IF is_loaded( ) = abap_false
• IF is_unloaded( ) = abap_true

on top of it, just to avoid writing IF NOT is_loaded( ) and getting an error from the "Prefer IS NOT to NOT IS" check? So, Clean ABAP would mean to basically implement all your methods with Boolean return values as twins, in case someone needs the negation?? And ABAP itself would need extra predicate functions like line_does_not_exist( ), contains_no( ) and does_not_match( ), to prevent us from writing IF NOT line_exists( ) etc.?

Personally, I agree with you. We should not create unnecessary methods if we have the option to validate the condition using the boolean types so that we do not grow the class unnecessarily.

I really see the benefit of this check for examples like those provided in the check documentation, and the check has already helped us to find lots of IF NOT … IS BOUND in our code and change them to IF … IS NOT BOUND (which is much better readable!)

Glad to see/hear it. :)

But if people need to put pseudo-comments for IF NOT is_loaded( ), then I am afraid, this otherwise useful check will get deactivated.

How about the options mentioned above? I prefer the IF is_loaded( ) = abap_false..

Ps: I am not a fan of IF is_loaded( ) and IF NOT is_loaded( ). My brain expects a comparison with the equals sign, hehe.

Kind Regards,
Lucas

[lucasborin]

Hi Jonathan,

I also came across this "false positive" and I share Jörn-Michael's opinion. In particular a simple statement like

IF NOT boolean_method(...).

should not produce a positive check result. And I also think that this is such a common scenario, that developers should not be pushed to "decorate" it with a pseudo-comment or disable the check completely.

I totally agree with you. We should not use the pseudo-comments to bypass "possible bugs" in the Checks, but so the exempt once it is really necessary (ex: team agreement).

How about if we open a new thread in the Clean ABAP?
We could ask their perspective on the NOT vs abap_false.

Implementing more elaborate checks (e.g. checking method/variable names for double negation, checking for IS, = or <> operators) could look like a nice-to-have feature at first glance, but I would not see it as essential.

Hm, why not? If the variable contains not or un<something> (negation) and the statement after the equals sign is <>, NP, etc, then it informs the user.

Regards,
Lucas

[lucasborin]

Ps: I tested it here, and the Check does not report the IF is_loaded( ) = abap_false. :)

[jdgx]

Hi all,

Therefore, let me fix my last reply. I mean, instead of IF NOT is_loaded( ) we could use...

IF is_loaded( ) = abap_false
IF is_unloaded( ) = abap_true

Personally, I would not prefer these options. I'm not sure why, but maybe that's because IF NOT is_loaded( ). is a bit closer to natural language:

• "If something is not loaded, do this and that."
• "If something is unloaded, do this and that."

In both cases in natural language we use "not" (or "un-") before the attribute "loaded". And keeping this ordering is to me more important than keeping the ordering of "is" and "not".

Ps: I am not a fan of IF is_loaded( ) and IF NOT is_loaded( ). My brain expects a comparison with the equals sign, hehe.

For me it's exactly the oppostite. When I see IF method( ). or IF NOT method( )., everything is clear to me. As soon as I see IF [NOT] method( ) = abap_... I have to read the statement at least twice in order to convince my brain that the statement does what it does. And every time I see IF boolean_variable = abap_... I think "why, oh why didn't ABAP just support IF boolean_variable. in the first place!", but that's another topic.

My personal list of preferences is as follows:

• IF positive_method( ). is always to be preferred over IF positive_method( ) = abap_true.
• IF NOT positive_method( ). is always to be preferred over IF positive_method( ) = abap_false.
• IF negative_method( ). is always to be preferred over IF negative_method( ) = abap_true.
• IF NOT negative_method( ). is ugly, but even here I'm not sure if IF negative_method( ) = abap_false. makes it any better... This is the situation where I would look for alternatives or accept a pragma if no better alternative can be found.

How about if we open a new thread in the Clean ABAP?

I will be busy this week and I will probably have no time to open (and discuss) a thread on Clean ABAP. If you have time, feel free to go ahead, then I can also share my opinion there. Otherwise I would open a thread there at beginning of next week.

Hm, why not? If the variable contains not or un<something> (negation) and the statement after the equals sign is <>, NP, etc, then it informs the user.

There are two aspects:

1. KISS principle/prevention of feature creep. I see this check only as nice-to-have and not as essential. If in doubt, stick to the essentials.
2. It would be possible to cover most scenarios, but it will not be possible to cover all scenarios correctly. And the more scenarios you cover, the higher are the chances of introducing new false positives.

This is why I am not sure if there would be a net benefit from implementing such checks (but maybe I'm too pessimistic).

Best,
Jonathan

[pokrakam]

I don't know if such a check exists, but we should only allow boolean types with implied booleans (or whatever they're called).

e.g. IF is_loaded( ). will evaluate to true if the method returns no.

So a check should verify the return type abap_bool (probably should be an independent check in it's own right), and if so then NOT is_loaded( ). should be considered valid.

[PeregrinTooc]

Hi all,

I'm also very unhappy with this check for methods. I was so happy when finally methods with boolean return values were usable in conditional expressions, because we actually come nearer to natural language. In our team, we don't want any equal signs in logical comparisons if we can avoid them to make the code more readable. So, in my opinion, this check should not be applicable for method calls.

Is there already a thread in clean ABAP about this? Could you then link it here?

Thanks and regards,
Philipp

[jdgx]

Hi Philipp, As far as I know, there is no related issue in Clean ABAP. I wanted to create one, but I didn’t yet find the time to do so. Feel free to go ahead, I would fully support to always prefer `if [not] boolean_method( ).` over `if boolean_method( ) <operator> true/false.` Best, Jonathan