Jak mogę zmniejszyć złożoność cykliczną?

Question

Ilekroć linkuję kod, nad którym pracuję, otrzymuję numer This function's cyclomatic complexity is too high. (7). Ale jestem nieco zdezorientowany, jak mogę go przepisać w taki sposób, żeby działał.

Byłby to funkcja, która utrzymuje, że rzucanie wiadomość:

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(); 
    } 
    } 
} 

Chciałbym usłyszeć jakieś rady, jak mogłoby wyglądać struktura kodu w taki sposób, aby uniknąć tego rodzaju sytuacji.

Answer 1

Cóż, w kodzie są tylko dwie akcje, ale zbyt wiele warunków. Użyj pojedynczej instrukcji if-else i operatorów boolowskich w warunku. Jeśli to niemożliwe, można przynajmniej

• usunięcia pustych linii, aby uzyskać pełną logikę na jednej stronie ekranu
• dodać kilka uwag na co gałęzie robią (i dlaczego)
• uniknąć przedterminowych powroty

Oto Twoja funkcja uproszczona:

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(); 
    } 
} 

i skrócona:

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

W rzeczywistości wszystkie te oświadczenia return wprowadzają zamieszanie w błąd, ale stanowią wskazówkę dla rozwiązania.

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(); 
} 

Jak o:

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

A jeśli chodzi o:

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

Upraszczając:

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

(bok. Oryginalny post lewo na klamrę zamykającą Jeśli ty (OP) zamierzałeś, że funkcja będzie kontynuowana po twojej poczta, to odpowiedź jest nie tak (ale powinno dokonaniu że wyraźniejsze))

Wynik: Usunęliśmy dwa (powtarzane) decyzje:

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

Po pierwsze, istnieją trzy wyniki czynność może : nie rób nic, zadzwoń pod numer this.close() lub zadzwoń pod numer this.open(). Idealnie więc wynikowa funkcja będzie miała jedną instrukcję if, która określa, który wynik jest używany.

Następnym krokiem jest wyodrębnienie całego kodu boolowskiego do zmiennych. Np. var leftPastCenter = this.content.getBoundingClientRect().left > viewport/2.

Wreszcie, użyj logiki boolowskiej, aby uprościć ją krok po kroku.

Oto jak to zrobiłem:

Po pierwsze, należy wyodrębnić wszystkie zmienne logiczne:

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(); 
     } 
    } 
} 

najłatwiejsza część wyciągnąć realizuje jeśli isScrolling to prawda, nic się nie dzieje. Ten natychmiast pozbywa się jednego poziomu zagnieżdżenia:

// 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(); 
    } 
} 

Spójrzmy teraz na sprawach this.open() są nazywane. Jeśli isPastHalf jest prawdziwe, this.open() jest wywoływane tylko wtedy, gdy !direction i !(leftPastCenter && isPulled).Jeśli isPastHalf jest fałszywe, a następnie this.open() nazywa się tylko wtedy, gdy leftPastCenter i !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(); 
     } 
    } 

Rzut IFS (tak this.close() nastąpi wcześniej), sprawia, że ​​kod nieco schludniej i daje moją ostateczną wersję:

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(); 
     } 
    } 
} 

Trudno jest mi zrobić więcej, nie znając Twojego kodu. Warto zauważyć, że direction i moja nowa zmienna positiveDelta są prawie identyczne - można usunąć positiveDelta i po prostu użyć direction. Ponadto, direction nie jest dobrą nazwą dla boolowskiej, coś jak movingLeft byłoby lepsze.

Answer 4

wolałbym proste i mniej zagnieżdżony kod jak poniżej:

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 już dał prawidłową odpowiedź, ale to wciąż zbyt skomplikowane jak na mój gust. Ponieważ nie jesteśmy using fortran77 myślę, że lepiej nam się przy użyciu early return. Ponadto kod można dokładniej wyjaśnić, wprowadzając dodatkowe zmienne:

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(); 
    } 
}