Return to Answer

Some comments on your code:

• Maximum line length: 80-100 is ok, more is unreadable.
• self[:active] == false -> !self[:active]
• Checks should be terser, encapsulate login in separate methods.
• return true: 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, IMO.

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

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

On a second refactor, I'd probably go all the way and abstract everthing to methods. Now you could write:

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

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

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

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

Some comments on your code:

• Maximum line length: 80-100 is ok, more is unreadable.
• self[:active] == false -> !self[:active]
• Checks should be terser, encapsulate login in separate methods.
• return true: 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, IMO.

I'd write:

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

Note that that you could go all the way and abstract all logic to methods. Then you could write:

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

Some comments on your code:

• Maximum line length: 80-100 is ok, more is unreadable.
• self[:active] == false -> !self[:active]
• Checks should be terser, encapsulate login in separate methods.
• return true: 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, IMO.

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

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

On a second refactor, I'd probably go all the way and abstract everthing to methods. Now you could write:

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

Some comments on your code:

• Maximum line length: 80-100 is ok, more is unreadable.
• self[:active] == false -> !self[:active]
• Checks should be terser, this is done by creating methods that abstract code. At least, the code within if self.template should be abstracted.
• return true: a return at the end of a code block is neither needed nor idiomatic.
• Subjective: I usually prefer full-fledge conditionals to guards, indentation helps a lot to see what's going on, IMO.

I'd write:

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

Some comments on your code:

• Maximum line length: 80-100 is ok, more is unreadable.
• self[:active] == false -> !self[:active]
• Checks should be terser, encapsulate login in separate methods.
• return true: 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, IMO.

I'd write:

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

Note that that you could go all the way and abstract all logic to methods. Then you could write:

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