come potrei ridurre la complessità ciclomatica?

Question

Ogni volta che rimuovo un pezzo di codice su cui sto lavorando ottengo il This function's cyclomatic complexity is too high. (7). Ma sono un po 'confuso su come potrei riscriverlo in modo tale che funzioni.

Questa sarebbe la funzione che continua a gettare quel messaggio:

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

Mi piacerebbe sentire qualche consiglio su come avrei potuto strutturare il mio codice in modo tale evito questo tipo di situazioni.

Answer 1

Bene, hai solo due azioni nel codice, ma troppe condizioni. Utilizzare una singola istruzione if-else e gli operatori booleani nella condizione. Se questo era impossibile, si poteva almeno

• rimuovere le righe vuote per ottenere la logica completa in una pagina schermo
• aggiungere alcuni commenti su ciò che i rami stanno facendo (e perché)
• Evita ritorni presto

Ecco la tua funzione semplificato:

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

e abbreviati:

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

In realtà tutte quelle istruzioni return confondono il problema, ma offrono un suggerimento alla soluzione.

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

ne dite:

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

E per quanto riguarda:

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

Semplificare:

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

(A parte:. Il post originale lasciato fuori una parentesi graffa di chiusura Se tu (OP) hai inteso che la funzione continua oltre il tuo posta, allora questa risposta è sbagliata (ma si dovrebbe aver fatto quella più chiara))

Risultato: Abbiamo eliminato due (ripetute) decisioni:

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

In primo luogo, ci sono tre risultati vostra funzione possono avere: non fare nulla, chiamare this.close() o chiamare this.open(). Quindi idealmente la funzione risultante avrà una sola istruzione if che determina quale risultato viene utilizzato.

Il passaggio successivo consiste nell'estrarre tutto il codice booleano in variabili. Ad esempio var leftPastCenter = this.content.getBoundingClientRect().left > viewport/2.

Infine, utilizzare la logica booleana per semplificare passo dopo passo.

Ecco come ho fatto:

In primo luogo, l'estratto di tutte le variabili booleane:

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

La parte più facile da tirare fuori se sta realizzando isScrolling è vero, non succede mai niente. Questo immediatamente si libera di un livello di nidificazione:

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

Ora guardiamo i casi this.open() sono chiamati. Se isPastHalf è vero, this.open() viene chiamato solo quando !direction e !(leftPastCenter && isPulled).Se isPastHalf è falso, quindi this.open() viene chiamato solo quando leftPastCenter e !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(); 
     } 
    } 

Sfogliando l'IFS (così this.close() viene prima), rende il codice un po 'più ordinato, e dà la mia versione finale:

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

È difficile per me fare di più, senza conoscere il codice base. Una cosa da notare è direction e la mia nuova variabile positiveDelta è quasi identica - è possibile rimuovere positiveDelta e utilizzare semplicemente direction. Inoltre, direction non è un buon nome per un booleano, qualcosa come movingLeft sarebbe meglio.

Answer 4

io preferirei un semplice codice e meno nidificato come di seguito:

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 ha già dato una risposta corretta, ma è ancora troppo complesso per i miei gusti. Dal momento che non siamo using fortran77, penso che sia meglio usare lo early return. Inoltre, il codice può essere ulteriormente chiarito introducendo variabili extra:

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