SonarQube JavaScript
  1. SonarQube JavaScript
  2. SONARJS-31

Rule NestedIfDepthCheck should not produce false-positive in case of 'if-else-if'

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.0
    • Fix Version/s: 1.1
    • Component/s: None
    • Labels:
      None
    • Number of attachments :
      0

      Description

      I have noticed that I am getting a false positive with this rule. I will tell me that I have 4 levels of IF statements. This is wrong because I only have two.
      This is how it seems to work it out

      if(true) { ->+1
      
      } else if(true) { ->+1
          if(true){ ->+1 (This is where it will say is at level 4)
      
          } else if(true) { ->+1
      
          }
      }
      

      What I think is happening is that it seems to be adding an extra "if" if its an "else if" which it shouldn't be.

        Activity

        Hide
        Eriks Nukis added a comment -

        According to "https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Statements/if...else" "else if" is just a "nested if"

        Show
        Eriks Nukis added a comment - According to "https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Statements/if...else" "else if" is just a "nested if"
        Hide
        Evgeny Mandrikov added a comment -

        Hi guys,

        Formally speaking Eriks is right ( together with Mozilla ) - from an AST (abstract syntax tree) point of view - "else if" construction clearly is a nested "if". But from a developer point of view (i.e. user of Sonar) this seems weird.

        And indeed I had a discussion with another member of SonarSource language team - Dinesh Bolkensteyn. We came to conclusion that this rule with parameter max=1 should behave as shown below:

        if (...) { // no violation, because level is 1
          if (...) { // violation, because level is 2
          }
        } else if (...) { // no violation, because level still 1
          if (...) { // violation, because level is 2
          }
        } else {
          if (...) { // violation, because level is 2
          }
        }
        

        i.e. "else if" construction should not increase level.

        Show
        Evgeny Mandrikov added a comment - Hi guys, Formally speaking Eriks is right ( together with Mozilla ) - from an AST (abstract syntax tree) point of view - "else if" construction clearly is a nested "if". But from a developer point of view (i.e. user of Sonar) this seems weird. And indeed I had a discussion with another member of SonarSource language team - Dinesh Bolkensteyn . We came to conclusion that this rule with parameter max=1 should behave as shown below: if (...) { // no violation, because level is 1 if (...) { // violation, because level is 2 } } else if (...) { // no violation, because level still 1 if (...) { // violation, because level is 2 } } else { if (...) { // violation, because level is 2 } } i.e. "else if" construction should not increase level.
        Hide
        Evgeny Mandrikov added a comment -

        Also should be noted that current behaviour is exactly the same as behaviour of PMD rule for Java, whereas we want to behave as Checkstyle rule for Java.

        Show
        Evgeny Mandrikov added a comment - Also should be noted that current behaviour is exactly the same as behaviour of PMD rule for Java, whereas we want to behave as Checkstyle rule for Java.
        Hide
        Freddy Mallet added a comment -

        Works well Evgeny, could you just change the following thing in the description :

        • "Deeply nested if statements are hard to read." by "Deeply nested if statements are hard to read and so to maintain."
        • "The following code snippet illustrates this rule :" by "The following code snippet illustrates this rule with the default threshold of 3 :"
        Show
        Freddy Mallet added a comment - Works well Evgeny, could you just change the following thing in the description : "Deeply nested if statements are hard to read." by "Deeply nested if statements are hard to read and so to maintain." "The following code snippet illustrates this rule :" by "The following code snippet illustrates this rule with the default threshold of 3 :"
        Hide
        Evgeny Mandrikov added a comment -

        Done.

        Show
        Evgeny Mandrikov added a comment - Done.
        Hide
        Freddy Mallet added a comment -

        Manually tested !

        Show
        Freddy Mallet added a comment - Manually tested !

          People

          • Assignee:
            Evgeny Mandrikov
            Reporter:
            Anonymous
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: