Problem with NestedTernaryCheck #167

Closed
KTannenberg opened this Issue Feb 18, 2014 · 3 comments

Comments

Projects
None yet
4 participants
@KTannenberg
Contributor

KTannenberg commented Feb 18, 2014

NestedTernaryCheck have false-positive warnings:
For example in case of string concatenation:

return Joiner.on(" and ").join(Collections2.transform(columns, new Function<String, String>() {
    @Override
    public String apply(String column) {
        return alias1 != null ? dialect.quote(alias1, column) : dialect.quote(column) 
              + " = "
              + alias2 != null ? dialect.quote(alias2, column) : dialect.quote(column);
    }
}));

If ternary expressions are wrapped into parentheses to explicitly indicate priority of operations no warning is triggered.

@romani

This comment has been minimized.

Show comment Hide comment
@romani

romani Feb 19, 2014

Owner

agree.

Owner

romani commented Feb 19, 2014

agree.

@daniilyar

This comment has been minimized.

Show comment Hide comment
@daniilyar

daniilyar Mar 1, 2014

Owner

Checks work correctly with code:

    String x = (getSmth() ? "A" : "B") + (getSmth() ? "B" : "C");
    x = getSmth() ? "A" : "B" + (getSmth() ? "B" : "C");

It fails only if there is simple == or != check clause, and so, when there is no "(" ")" brackets:
for this case Checkstyle builds tree which will contain one ternary operator as parent for another.

Example: code

public static void main(String[] args) {
    String x = args[0] != null ? "" : "" + args[1] == null ? "" : "";
}

produces the tree below:

screenshot from 2014-03-01 15 56 38

Moreover, plus for this case is placed under (!) subtree of second ternary operator:

screenshot from 2014-03-01 16 00 02

Owner

daniilyar commented Mar 1, 2014

Checks work correctly with code:

    String x = (getSmth() ? "A" : "B") + (getSmth() ? "B" : "C");
    x = getSmth() ? "A" : "B" + (getSmth() ? "B" : "C");

It fails only if there is simple == or != check clause, and so, when there is no "(" ")" brackets:
for this case Checkstyle builds tree which will contain one ternary operator as parent for another.

Example: code

public static void main(String[] args) {
    String x = args[0] != null ? "" : "" + args[1] == null ? "" : "";
}

produces the tree below:

screenshot from 2014-03-01 15 56 38

Moreover, plus for this case is placed under (!) subtree of second ternary operator:

screenshot from 2014-03-01 16 00 02

@daniilyar

This comment has been minimized.

Show comment Hide comment
@daniilyar

daniilyar Mar 1, 2014

Owner

The conditional operator (also called the ternary operator) ?: has the lowest-but-one precedence, so all other operations takes place before the ?
So, string concatenation is evaluated before conditional expression (link)

As result, such code as:

    String str = null;
    String x = str == null ? "A" : "B" + str == null ? "C" : "D";
    System.out.println(x);
    x = str != null ? "A" : "B" + str == null ? "C" : "D";
    System.out.println(x);

prints:

 A
 D

And not expected values:

AC
BC

So, I propose to make NestedTernaryCheck more general, i.e. we have to replace it with check which will just count number of ternary operators per expression. That check will cover all cases such as:

  • Nested ternary operators (with operators depth >= X);
  • String concatenation with calling more than X ternary calculations;
  • inline mathematical calculations based on more than one ternary operator (i.e, based on X ternary operators).

New check have to have only one option: number of allowed ternary operators per expression (i.e., X threshold above).

Owner

daniilyar commented Mar 1, 2014

The conditional operator (also called the ternary operator) ?: has the lowest-but-one precedence, so all other operations takes place before the ?
So, string concatenation is evaluated before conditional expression (link)

As result, such code as:

    String str = null;
    String x = str == null ? "A" : "B" + str == null ? "C" : "D";
    System.out.println(x);
    x = str != null ? "A" : "B" + str == null ? "C" : "D";
    System.out.println(x);

prints:

 A
 D

And not expected values:

AC
BC

So, I propose to make NestedTernaryCheck more general, i.e. we have to replace it with check which will just count number of ternary operators per expression. That check will cover all cases such as:

  • Nested ternary operators (with operators depth >= X);
  • String concatenation with calling more than X ternary calculations;
  • inline mathematical calculations based on more than one ternary operator (i.e, based on X ternary operators).

New check have to have only one option: number of allowed ternary operators per expression (i.e., X threshold above).

@daniilyar daniilyar assigned alexkravin and unassigned daniilyar Mar 1, 2014

@daniilyar daniilyar added the bug label Jul 13, 2014

@daniilyar daniilyar closed this Jul 22, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment