Разрешение цикломатической и воспринимаемой сложности

У меня есть следующий метод в моем тестовом приложении:

def on(definition, visit = false, &block)
  if @page.is_a?(definition)
    block.call @page if block
    return @page
  end

  if @context.is_a?(definition)
    block.call @context if block
    @page = @context unless @page.is_a?(definition)
    return @context
  end

  @page = definition.new(@browser)
  @page.view if visit

  @page.correct_url? if @page.respond_to?(:url_matches)
  @page.correct_title? if @page.respond_to?(:title_is)

  @model = @page

  block.call @page if block

  @page
end

Когда я запускаю инструмент rubocop для файла, содержащего этот метод, я получаю следующие ответы:

C: Cyclomatic complexity for on is too high. [10/6]
C: Perceived complexity for on is too high. [10/7]

Чего я не понимаю, так это того, что он считает «слишком сложным», поэтому мне трудно понять, как решить проблему. В идеале я бы предпочел не просто говорить rubocop, чтобы он избегал предупреждения, так как он, без сомнения, сообщает мне что-то полезное.

Что касается сложности метода, как видите, у меня есть пара вызовов if, а затем мне нужно поработать с объектом @page, чтобы убедиться, что он настроен правильно. (Этот пример в контексте представляет собой объект Watir-WebDriver.)

Я согласен, что метод сложен в том смысле, что он требует проверки того, существует ли уже @page и установлено ли что-то, а также проверки того, должно ли @page совпадать с @context. Но -- опять же, я не знаю, что с этим делать.

Полный код модуля, внутри которого находится этот метод, находится здесь:

https://github.com/jnyman/symbiont/blob/master/lib/symbiont/factory.rb

Сначала я думал, что могу просто разбить это на разные вызовы методов, что может уменьшить сложность каждого метода. Но тогда это означает, что кто-то, кто читает мой код, должен использовать ряд различных методов, чтобы понять, что делает on. Мне кажется, что простое перемещение вещей не устраняет сложность в целом; скорее, это было бы просто перетасовкой. Или я ошибаюсь?

Любые советы здесь ценятся.

ОБНОВЛЕННЫЙ КОД

У меня это несколько уменьшилось. Вот что у меня сейчас есть:

def on(definition, visit = false, &block)
  if @page.is_a?(definition)
    block.call @page if block
    return @page
  end

  if @context.is_a?(definition)
    block.call @context if block
    @page = @context
    return @context
  end

  @page = definition.new(@browser)
  @page.view if visit

  @model = @page

  block.call @page if block

  @page
end

Основываясь на отзывах, я удалил квалификатор unless, который казался бесполезным. Я также удалил две строки, которые, как мне показалось, можно было бы лучше использовать в другом месте (проверив заголовок и URL-адрес).

Это полностью устранило «воспринимаемую сложность» и оставило мне только это:

C: Cyclomatic complexity for on is too high. [7/6]

Мне кажется, что "одна точка" (или какая там терминология) слишком сложна.


person Jeff Nyman    schedule 03.10.2015    source источник
comment
Ваш код кричит, что он не может определить тип объекта/класса во всем нем. Ты постоянно спрашиваешь, эй, ты это? Вы знаете, как это сделать? Это нормально для защиты, но, вероятно, вам следует проверить свой ввод где-то еще, прежде чем этот метод может быть вызван.   -  person Anthony    schedule 03.10.2015
comment
Хм. Хотя я не понимаю, как я мог. (Я не говорю, что это невозможно!) Проблема в том, что метод фабрики контекста on вызывается так: on(SomePage). Это может произойти в любом месте тестового сценария. Кроме того, вы можете иметь on(SomePage), а затем on(SomeOtherPage). Для логики важно знать, что SomeOtherPage не является SomePage. Но вы также можете сделать on_new(SomePage), что означает, что SomePage — это SomePage, но новый — другой — экземпляр. Итак, проблема в том, что я не знаю, что собирается делать автор тестов. Тем не менее, приняв ваши слова близко к сердцу, я посмотрю, смогу ли я его очистить.   -  person Jeff Nyman    schedule 03.10.2015


Ответы (1)


Есть места, где ваш код избыточен. Например, у вас есть это:

if @page.is_a?(definition)
  ...
  return ...
end

это означает, что в любой части, следующей за этой, вы можете предположить, что @page.is_a?(definition) не является true, если только вы не измените @page после этой части. Тем не менее, у вас есть:

if @context.is_a?(definition)
  ... unless @page.is_a?(definition)
end
person sawa    schedule 03.10.2015
comment
Ах! Хорошая точка зрения. Да, если только квалификатор не кажется нужным. Если я уберу это, я уберу один балл из своей сложности согласно rubocop. Это ставит меня на Cyclomatic complexity for on is too high. [9/6] и Perceived complexity for on is too high. [9/7]. Интересно. Так что кажется, что сложность основана на моих условных выражениях, по крайней мере, в некоторой степени. - person Jeff Nyman; 03.10.2015
comment
Прошло некоторое время, и я понимаю, что ничего не принимал в этом. Этот ответ, безусловно, привел меня к пониманию того, на чем мне нужно было сделать акцент; следовательно, принятие (сразу же просрочено). - person Jeff Nyman; 13.05.2017