Wie könnte ich die zyklomatische Komplexität reduzieren?

Question

Immer wenn ich ein Stück Code füge, an dem ich gerade arbeite, bekomme ich die This function's cyclomatic complexity is too high. (7). Aber ich bin ein wenig verwirrt darüber, wie ich es so umschreiben könnte, damit es funktioniert.

Dies würde die Funktion sein, die werfen die Nachricht hält:

function() { 
    var duration = +new Date() - start.time, 
    isPastHalf = Number(duration) < 250 && Math.abs(delta.x) > 20 || Math.abs(delta.x) > viewport/2, 
    direction = delta.x < 0; 

    if (!isScrolling) { 
    if (isPastHalf) { 
     if (direction) { 
     this.close(); 
     } else { 
     if (this.content.getBoundingClientRect().left > viewport/2 && pulled === true) { 
      this.close(); 
      return; 
     } 
     this.open(); 
     } 
    } else { 
     if (this.content.getBoundingClientRect().left > viewport/2) { 
     if (this.isEmpty(delta) || delta.x > 0) { 
      this.close(); 
      return; 
     } 
     this.open(); 
     return; 
     } 
     this.close(); 
    } 
    } 
} 

Ich mag würde einige Ratschläge hören, wie ich meinen Code in einer solchen Art und Weise strukturieren könnte, damit ich diese Art von Situationen zu vermeiden.

Answer 1

Nun, Sie haben nur zwei Aktionen in Ihrem Code, aber viel zu viele Bedingungen. Verwenden Sie eine einzelne if-else-Anweisung und boolesche Operatoren in der Bedingung. Wenn das nicht möglich war, konnte man zumindest

• die leeren Zeilen entfernen auf einer Bildschirmseite die volle Logik zu bekommen
• fügen Sie einige Kommentare auf das, was die Zweige tun (und warum)
• vermeiden früh zurückkehrt

Hier ist Ihre Funktion vereinfacht:

var duration = +new Date() - start.time, 
    isPastHalf = Number(duration) < 250 && Math.abs(delta.x) > 20 || Math.abs(delta.x) > viewport/2, 
    isFarRight = this.content.getBoundingClientRect().left > viewport/2, 
    direction = delta.x < 0; 

if (!isScrolling) { 
    if (isPastHalf) { 
     if (direction) 
      this.close(); 
     else { 
      if (isFarRight && pulled) 
       this.close(); 
      else 
       this.open(); 
     } 
    } else { 
     if (isFarRight) { 
      // Looks like the opposite of `direction`, is it? 
      if (this.isEmpty(delta) || delta.x > 0) 
       this.close(); 
      else 
       this.open(); 
     } else 
      this.close(); 
    } 
} 

und verkürzt:

var duration = +new Date() - start.time, 
    isPastHalf = Number(duration) < 250 && Math.abs(delta.x) > 20 || Math.abs(delta.x) > viewport/2, 
    isFarRight = this.content.getBoundingClientRect().left > viewport/2, 
    direction = delta.x < 0, 
    undirection = this.isEmpty(delta) || delta.x > 0; 

if (!isScrolling) { 
    if (isPastHalf && ! direction && !(isFarRight && pulled) 
    || !isPastHalf && !undirection && isFarRight) 
     this.open(); 
    else 
     this.close(); 
} 

Answer 2

Eigentlich sind alle diese return Anweisungen verwirren das Problem, aber sie bieten einen Hinweis auf die Lösung.

if (direction) { 
    this.close(); 
} else { 
    if (this.content.getBoundingClientRect().left > viewport/2 && pulled === true) { 
    this.close(); 
    return; // We'll never `this.open()` if this is true anyway, so combine the booleans. 
    } 
    this.open(); 
} 

Wie wäre:

if (direction || (this.content.getBoundingClientRect().left > viewport/2 && pulled === true)) { 
    this.close(); 
} else { 
    this.open(); 
} 

Und für:

if (this.content.getBoundingClientRect().left > viewport/2) { 
    if (this.isEmpty(delta) || delta.x > 0) { 
    this.close(); 
    return; // Combine the booleans! 
    } 
    this.open(); 
    return; 
} 

Simplify:

if ((this.isEmpty(delta) || delta.x > 0) || !this.content.getBoundingClientRect().left > viewport/2) { 
    this.close(); 
} else { 
    this.open(); 
} 

(abgesehen. Der ursprüngliche Post eine schließenden Klammer weggelassen Wenn Du (OP) beabsichtigst, dass die Funktion an deinem vorbei geht Post, dann ist diese Antwort ist falsch (aber Sie sollten das deutlicher gemacht haben)

Ergebnis): Wir zwei beseitigt haben (wiederholt) Entscheidungen:

function() { 
    var duration = +new Date() - start.time, 
    isPastHalf = Number(duration) < 250 && Math.abs(delta.x) > 20 || Math.abs(delta.x) > viewport/2, 
    direction = delta.x < 0; 

    if (!isScrolling) { 
    if (isPastHalf) { 
     if (direction || (this.content.getBoundingClientRect().left > viewport/2 && pulled === true)) { 
     this.close(); 
     } else { 
     this.open(); 
     } 
    } else { 
     if ((this.isEmpty(delta) || delta.x > 0) || !this.content.getBoundingClientRect().left > viewport/2) { 
     this.close(); 
     } else { 
     this.open(); 
     } 
    } 
    } 
} 

Answer 3

Erstens gibt es drei Ergebnisse Ihre Funktion habe: nichts tun, this.close() anrufen oder this.open() anrufen. Im Idealfall wird die resultierende Funktion nur eine if-Anweisung haben, die bestimmt, welches Ergebnis verwendet wird.

Der nächste Schritt besteht darin, den gesamten booleschen Code in Variablen zu extrahieren. ZB var leftPastCenter = this.content.getBoundingClientRect().left > viewport/2.

Schließlich verwenden Sie boolesche Logik, um es Schritt für Schritt zu vereinfachen.

Hier ist, wie ich es tat:

Zunächst extrahieren alle Booleschen Variablen:

function() { 
    var duration = +new Date() - start.time, 
     isPastHalf = Number(duration) < 250 && Math.abs(delta.x) > 20 || Math.abs(delta.x) > viewport/2, 
     direction = delta.x < 0, 
     leftPastCenter = this.content.getBoundingClientRect().left > viewport/2, 
     positiveDelta = this.isEmpty(delta) || delta.x > 0, 
     isPulled = pulled === true; // I'll assume the test is needed rather than just using pulled. 

    if (!isScrolling) { 
     if (isPastHalf) { 
      if (direction) { 
       this.close(); 
      } else { 
       if (leftPastCenter && isPulled) { 
        this.close(); 
        return; 
       } 
       this.open(); 
      } 
     } else { 
      if (leftPastCenter) { 
       if (positiveDelta) { 
        this.close(); 
        return; 
       } 
       this.open(); 
       return; 
      } 
      this.close(); 
     } 
    } 
} 

Der einfachste realisierenden Teil zu ziehen, wenn isScrolling wahr ist, geschieht nichts jemals zuvor. Dies wird sofort von einer Ebene der Verschachtelung befreien:

// above same 
    if (isScrolling) { return; } 

    if (isPastHalf) { 
     if (direction) { 
      this.close(); 
     } else { 
      if (leftPastCenter && isPulled) { 
       this.close(); 
       return; 
      } 
      this.open(); 
     } 
    } else { 
     if (leftPastCenter) { 
      if (positiveDelta) { 
       this.close(); 
       return; 
      } 
      this.open(); 
      return; 
     } 
     this.close(); 
    } 
} 

in den Fällen sehen nun this.open() genannt werden. Wenn isPastHalf wahr ist, wird this.open() nur aufgerufen, wenn !direction und !(leftPastCenter && isPulled).Wenn isPastHalf falsch ist, dann wird this.open() nur aufgerufen, wenn leftPastCenter und !positiveDelta:

// above same 
    if (isScrolling) { return; } 

    if (isPastHalf) { 
     if (!direction && !(leftPastCenter && isPulled)) { 
      this.open(); 
     } else { 
      this.close(); 
     } 
    } else { 
     if (leftPastCenter && !positiveDelta) { 
      this.open(); 
     } else { 
      this.close(); 
     } 
    } 

die ifs Flipping (so this.close() kommt zuerst), der Code ein wenig sauberer macht, und gibt meine letzte Version:

function() { 

    var duration = +new Date() - start.time, 
     isPastHalf = Number(duration) < 250 && Math.abs(delta.x) > 20 || Math.abs(delta.x) > viewport/2, 
     direction = delta.x < 0, 
     leftPastCenter = this.content.getBoundingClientRect().left > viewport/2, 
     positiveDelta = this.isEmpty(delta) || delta.x > 0, 
     isPulled = pulled === true; // I'll assume the test is needed rather than just using pulled. 

    if (isScrolling) { return; } 

    if (isPastHalf) { 
     if (direction || (leftPastCenter && isPulled)) { 
      this.close(); 
     } else { 
      this.open(); 
     } 
    } else { 
     if (!leftPastCenter || positiveDelta) { 
      this.close(); 
     } else { 
      this.open(); 
     } 
    } 
} 

Es ist schwierig für mich, mehr zu tun, ohne Ihre Codebasis zu kennen. Eine Sache zu beachten ist direction und meine neue Variable positiveDelta sind fast identisch - Sie könnten positiveDelta entfernen und einfach direction verwenden. Auch direction ist kein guter Name für einen Boolean, etwas wie movingLeft wäre besser.

Answer 4

mag ich unten einen einfachen und weniger verschachtelten Code bevorzugen:

function() 
{ 
    var duration = +new Date() - start.time, 
     isPastHalf = Number(duration) < 250 && Math.abs(delta.x) > 20 || Math.abs(delta.x) > viewport/2, 
     direction = delta.x < 0; 

    if (isScrolling) 
    { 
     return; 
    } 
    if (isPastHalf) 
    { 
     if (direction) 
     { 
      this.close(); 
      return; 
     } 
     if (this.content.getBoundingClientRect().left > viewport/2 && pulled == = true) 
     { 
      this.close(); 
      return; 
     } 
     this.open(); 
     return; 
    } 
    if (this.content.getBoundingClientRect().left > viewport/2) 
    { 
     if (this.isEmpty(delta) || delta.x > 0) 
     { 
      this.close(); 
      return; 
     } 
     this.open(); 
     return; 
    } 
    this.close(); 
    return; 
} 

Answer 5

Bergi hat bereits eine richtige Antwort gegeben, aber es ist immer noch zu komplex für meinen Geschmack. Da wir nicht using fortran77 sind, denke ich, wir sind besser dran mit einem early return. Außerdem kann der Code durch Einführen zusätzlicher Variablen weiter geklärt werden:

function doSomething(isScrolling, start, delta, viewport) { 
    if (isScrolling) return; 

    var duration = +new Date() - start.time, 
     isPastHalf = Number(duration) < 250 && Math.abs(delta.x) > 20 || Math.abs(delta.x) > viewport/2, 
     isFarRight = this.content.getBoundingClientRect().left > viewport/2, 
     direction = delta.x < 0, 
     undirection = this.isEmpty(delta) || delta.x > 0; 

    // I'm not sure if my variable names reflect the actual case, but that's 
    // exactly the point. By choosing the correct variable names for this 
    // anybody reading the code can immediatly comprehend what's happening. 
    var movedPastBottom = isPastHalf && !direction && !(isFarRight && pulled); 
    var movedBeforeTop = isPastHalf && !undirection && isFarRight; 

    if (movedPastBottom || movedBeforeTop) { 
     this.open(); 
    else 
     this.close(); 
    } 
}