Resolving Cyclomatic and Perceived Complexity

Question

I have the following method in my test app:

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

When I run the rubocop tool against the file containing this method, I get the following responses:

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

What I don't get is what it thinks is "too complex" thus I'm having trouble figuring out how to resolve the issue. Ideally I'd rather not just tell rubocop to avoid the warning since it's no doubt telling me something useful.

As far as the method being complex, as you can see, I have a couple of if calls and then I have to work with the @page object in order to make sure it is set up correctly. (This example, in context, is a Watir-WebDriver object.)

I do agree the method is complex in that it requires checking on if @page is already existing and set to something as well as checking if the @page should be the same as the @context. But -- again, I'm not sure what to do about it.

The full code for the module that this method is inside is here:

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

I was initially thinking I could just break this up into different method calls, which might reduce the complexity of each method. But then that means someone reading my code has to hop to a series of different methods to understand what on is doing. Just moving things around wouldn't seem, to me, to be removing complexity overall; rather, it would just be shuffling it around. Or am I wrong?

Any advice here is appreciated.

UPDATED CODE

I have gotten this reduced somewhat. Here's what I now have:

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

Based on feedback, I removed an unless qualifier that did seem to be useless. I also removed two lines that I found I could put to better use somewhere else (checking the title and the url).

This has removed the "perceived complexity" entirely and has left me with just this:

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

I appear to be "one point" (or whatever the terminology is) too complex.

Answer 1

There are places where your code is redundant. For example, you have this:

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

which means that, in any part to follow this, you can assume that @page.is_a?(definition) is not true, unless you modify @page after this part. Nevertheless, you have:

if @context.is_a?(definition)
  ... unless @page.is_a?(definition)
end