Menu

#44 Problems with AvoidDeeplyNestedIfStmts

closed
pmd (543)
5
2012-10-07
2002-10-30
No

The rule AvoidDeeplyNestedIfStmts says this is against
the rule, but this isn't nested statements. I am not
saying this is good practise, but maybe some other rule
should handle this case.

This may be a factory method, and then this is the only
option.

if( i == 20 ) {
//something
}
else if( i == 21 ){
//something else
}
else if( i == 503 ) {
// and something else
}

Discussion

  • Tom Copeland

    Tom Copeland - 2002-10-30

    Logged In: YES
    user_id=5159

    The problem is that the AST for those if stmts has all the else
    clauses nested inside each other. When I first wrote the rule
    I wasn't sure how to get that right.... I'll look at it again.

    Thanks,

    Tom

     
  • Ole-Martin Mřrk

    Logged In: YES
    user_id=394445

    ok, so the AST generates something like this?
    if(...) {
    }
    else {
    if( ... ) {
    }
    else {
    if(...) {
    }
    }
    }

     
  • Tom Copeland

    Tom Copeland - 2002-10-30

    Logged In: YES
    user_id=5159

    You can tell I was in denial on this one:

    http://pmd.sourceforge.net/xref/test/net/sourceforge/pmd/rules
    /AvoidDeeplyNestedIfStmtsRuleTest.html

     
  • Tom Copeland

    Tom Copeland - 2002-10-30

    Logged In: YES
    user_id=5159

    Right, so this code:

    public class AvoidDeeplyNestedIfStmtsRule2 {
    public void bar() {
    if (true) {
    } else if (true) {
    } else if (true) {
    } else {
    // this ain't good code, but it shouldn't trigger this rule
    }
    }
    }

    produces this AST (minus some irrelevant stuff)

        BlockStatement
         Statement
          IfStatement
           Expression
           PrimaryExpression
            PrimaryPrefix
             Literal
              BooleanLiteral
           Statement
            Block
           Statement
            IfStatement
             Expression
             PrimaryExpression
              PrimaryPrefix
               Literal
                BooleanLiteral
             Statement
              Block
             Statement
              IfStatement
               Expression
               PrimaryExpression
                PrimaryPrefix
                 Literal
                  BooleanLiteral
               Statement
                Block
               Statement
                Block
    

    You can see how the AST is nested:

    IfStmt
    Stmt
    IfStmt
    Stmt
    IfStmt

    even though those are actually else blocks, not if blocks.
    Maybe we can add some stuff to the grammer to differentiate
    if blocks from else blocks....

    Tom

     
  • Tom Copeland

    Tom Copeland - 2002-10-30

    Logged In: YES
    user_id=5159

    By the way, a good way to view the AST is to run the
    astviewer utility. Just run that astviewer.bat batch file in the
    etc directory and type or paste some code into the left hand
    window and click the button. The AST will be displayed in
    the right hand pane.

    Tom

     
  • Tom Copeland

    Tom Copeland - 2002-10-30

    Logged In: YES
    user_id=5159

    Fixed. Required a tweak to the grammer IfStatement
    production and a minor twiddle of the rule.

    Thanks,

    Tom

     

Log in to post a comment.