Как я могу уменьшить цикломатическую сложность?

Question

Всякий раз, когда я нажимаю кусок кода, над которым я работаю, я получаю This function's cyclomatic complexity is too high. (7). Но я немного смущен тем, как я могу переписать его таким образом, чтобы он работал.

Это будет функция, которая продолжает бросать это сообщение:

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

Я хотел бы услышать несколько советов о том, как я мог бы структурировать свой код таким образом, чтобы я избежать такого рода ситуаций.

Answer 1

У вас есть только два действия в вашем коде, но слишком много условий. Используйте один оператор if-else и логические операторы в условии. Если это невозможно, вы могли бы по крайней мере

• удалить пустые строки, чтобы получить полную логику на одном экране страницы
• добавить некоторые комментарии о том, что делают ветви (и почему)
• избежать досрочного возвращения

Вот ваша функция упрощается:

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

и укорачивается:

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

На самом деле все эти заявления return путают проблему, но они предлагают намек на решение.

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

Как насчет:

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

А также для:

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

(кроме:. Оригинальный пост покинул из закрывающую скобку Если вы (OP) предполагали, что функция продолжается пост, то этот ответ неверен (но вы должны сделали что четкому))

Результата: Мы ликвидировали два (неоднократные) решений:

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

Во-первых, есть три результата ваша функция может имеют: ничего не делать, звоните this.close() или звоните this.open(). Поэтому в идеале результирующая функция будет иметь только один оператор if, определяющий, какой результат используется.

Следующим шагом является извлечение всего булева кода в переменные. Например, var leftPastCenter = this.content.getBoundingClientRect().left > viewport/2.

Наконец, используйте логическую логику, чтобы упростить ее шаг за шагом.

Вот как я это сделал:

Во-первых, извлечь все булевы переменные:

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

Простейшим часть вытаскивать реализуется, если isScrolling правда, никогда ничего не происходит. Это сразу же избавляется от одного уровня вложенности:

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

Теперь рассмотрят случаи this.open() называется. Если isPastHalf истинно, this.open() вызывается только тогда, когда !direction и !(leftPastCenter && isPulled).Если isPastHalf ложно, то this.open() вызывалась только когда leftPastCenter и !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(); 
     } 
    } 

Подавать сослагательное наклонение (так this.close() приходит первым), делает код немного аккуратнее, и дает мой окончательный вариант:

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

Мне сложно делать больше, не зная своей кодовой базы. Стоит отметить direction, и моя новая переменная positiveDelta почти идентична - вы можете удалить positiveDelta и просто использовать direction. Кроме того, direction не является хорошим именем для логического, что-то вроде movingLeft было бы лучше.

Answer 4

Я предпочел бы простой и менее вложенный код, как показано ниже:

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

Бергов уже дали правильный ответ, но это все еще слишком сложно для моего вкуса. Поскольку мы не using fortran77 Я думаю, что нам лучше использовать early return. Кроме того, код может быть дополнительно уточнен путем введения дополнительных переменных:

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