rule idea: conditional-expressions-should-be-in-brackets

[ccecvb]

I'd like to propose a new rule : conditional-expressions-should-be-in-brackets.

• conditional expressions can easily get confusing, explicitly adding brackets makes them clearer to read
• the antlr parser has to work hard to resolve them, see Single statement taking minutes to be parsed #1151

Non-compliant code

define variable xx as integer no-undo extent 20.
define variable sep as character no-undo initial ';'.
put unformatted "header" +
            if xx[1] = ? then "" else string(xx[1]) + sep +
            if xx[1] = ? then "" else string(xx[1]) + sep
   .

Compliant code

define variable xx as integer no-undo extent 20.
define variable sep as character no-undo initial ';'.
put unformatted "header" +
            (if xx[1] = ? then "" else string(xx[1])) + sep +
            (if xx[1] = ? then "" else string(xx[1])) + sep
   .

[stefandrissen]

Compliant and incompliant are functionally different.

[gquerret]

Compliant and incompliant are functionally different

Output is identical

[ccecvb]

@stefandrissen

Your remark kind of proves my point ;-) . It's confusing.

It's not my code, I don't know which version is functionally correct, looking at the indentation I think the developer assumes this to behave like the compliant version, even if it behaves as

Functionally equivalent compliant code

define variable xx as integer no-undo extent 20.
define variable sep as character no-undo initial ';'.
put unformatted "header" +
            (if xx[1] = ? then "" else (string(xx[1]) + sep +
            (if xx[1] = ? then "" else string(xx[1]) + sep)))
   .

[stefandrissen]

Assuming that the actual case was attempting to use x[1] and x[2], watch how different the results are (extended to 3 to make it even more obviously different):

var char[3] xx = ['one',?, 'three'].
var char sep = '|'.

message
    ( if xx[1] = ? then "" else string(xx[1]) ) + sep +
    ( if xx[2] = ? then "" else string(xx[2]) ) + sep +
    ( if xx[3] = ? then "" else string(xx[3]) ) + sep skip(1)

    if xx[1] = ? then "" else string(xx[1]) + sep +
    if xx[2] = ? then "" else string(xx[2]) + sep +
    if xx[3] = ? then "" else string(xx[3]) + sep
view-as alert-box.
            

The output is different.

[gquerret]

I was initially focused on the parser execution time, and I assumed the code of the developer was right. Given how that was formatted, I assumed the code was right and tested. It clearly wasn't...