I wrote a Rails model instance method that checks many separate conditions, and returns a single boolean. It does not employ any nesting of branching structures. Each check has its own comments describing its reasoning. Every check is necessary for the method to return an accurate value, and none of the checks are likely to be reused independently of the other checks. The function is 34 lines long including comments. A colleague reviewing the code suggested that the method may be too long, and therefore would be more readable if broken out into multiple helper methods for greater readability.

def active?
  #Check columns on self first, so if those dont pass,
  #no other records will need to be loaded.
  return false if self[:active] == false && !ignore_column
  return false if self.suspended # here for thoroughness. tasks should never be marked active and suspended.
  return false if self.completed_at # here for thoroughness. tasks should never be marked active and completed.
  if failed_attempts >= MAX_FAILED_ATTEMPTS # here for thoroughness. tasks should never be marked active after the max failed attempts.
    if !suppress_error_emails
      SystemMailer.error message:"Too many failed attempts for task execution. #{failed_attempts}/#{MAX_FAILED_ATTEMPTS} failed attempts for Task #{id}."
    return false
  #If the origin is inactive and not because it is a withdrawn status, then the task should be inactive.
  return false if !evergreen && origin && !origin.active && origin.is_a?(Crm::Status) && !origin.indicates_closed # Status or Subscription

  return false if sequenceable.is_a?(Crm::Case) && sequenceable.sales_stage_id.to_i>Crm::SalesStage.id('Placed') #placed policies shouldn't fire tasks

  #Recruits are always marked inactive because they are users that can't log in.
  #If the person is inactive but not a recruit, the task should be inactive.
  is_a_recruit=person.try(:is_a?,Usage::User) && person.try(:role)==Usage::Role['recruit']
  return false if !person.try(:active) && !is_a_recruit

  if self.template
    status_opted_out=self.person.status_unsubscribe && template_purpose_id==purposes["Status"][:id]
    con_mktg_opted_out=self.person.is_a?(Crm::Connection) && self.person.marketing_unsubscribe && template_purpose_id==purposes["Consumer Marketing"][:id]
    user_mktg_opted_out=self.person.is_a?(Usage::User) && self.person.marketing_unsubscribe && template_purpose_id==purposes["Agent Marketing"][:id]
    if status_opted_out || con_mktg_opted_out || user_mktg_opted_out
      return false

  return true

Is there a magic number of lines where a function or method becomes too long and should be broken out into multiple functions? If so, what is the magic number? Does it depend on the type of code (model/view/controller), or some other circumstance?

  • \$\begingroup\$ My question is so broad that it seems to me like the particular code is irrelevant. It may be that the question is too broad for this forum, but I figured it was worth a shot. I'll add my code now... \$\endgroup\$
    – maurice
    May 5, 2016 at 22:47
  • \$\begingroup\$ @maurice When you have this anti-pattern of nested if ... else you can refactor it by reducing the depth of the branches and breaking out your loops and conditions into smaller functions. Read: programmers.stackexchange.com/questions/205803/… \$\endgroup\$ May 5, 2016 at 23:19
  • 2
    \$\begingroup\$ Long lines are frustrating and they hide bugs. Split long lines at sensible points. You should be able to have a large complex text editor and debugger on-screen with your code. Your entire screen width is not the limit. \$\endgroup\$
    – doug65536
    May 5, 2016 at 23:29
  • 4
    \$\begingroup\$ Does it take more than 20 seconds top to understand what the function does? Yes? Then it is too long. By far. In this case, it's at least 5 minutes until you have grasped the code flow in this function. And the chance of changing this function without introducing a new bug is rather low. Pick any arbitrary period for the upper bound, but going by "time-to-comprehension" is usually a good estimation from my experience. \$\endgroup\$
    – Ext3h
    May 5, 2016 at 23:42
  • \$\begingroup\$ Please be sure to include a language tag on your questions. I added ruby for you. \$\endgroup\$
    – RubberDuck
    May 6, 2016 at 0:26

1 Answer 1


Some comments on your code:

  • Keep the line length below 80-100 or the code becomes unreadable.
  • self[:active] == false -> !self[:active]
  • Checks should be terser, encapsulate logic in methods.
  • A return at the end of a code block is neither necessary nor idiomatic.
  • [Subjective] I usually prefer full-fledge conditionals to guards, indentation helps a lot to see what's going on.

In a first refactor, I'd write a more readable case:

def active?
  when (!self[:active] && !ignore_column) || suspended || completed_at
  when failed_attempts >= MAX_FAILED_ATTEMPTS
    if suppress_error_emails
      message = "Too many failed attempts for task execution. " + 
        "#{failed_attempts}/#{MAX_FAILED_ATTEMPTS} failed attempts for Task #{id}."
      SystemMailer.error(message: message)
  # ...
  # more conditions here, you get the idea
  # ...
  when template && 
       (status_opted_out? || con_mktg_opted_out? || user_mktg_opted_out?)

On a second refactor, I'd probably move all logic to separate methods. Now you could write:

def active?
  condition1? && condition2? && ... && conditionN?

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.