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
: areturn
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
when !evergreen && origin && !origin.active && origin.is_a?(Crm::Status) &&
!origin.indicates_closed
false
when sequenceable.is_a?(Crm::Case) &&
sequenceable.sales_stage_id.to_i > Crm::SalesStage.id('Placed')
false
when template &&
(status_opted_out? || con_mktg_opted_out? || user_mktg_opted_out?)
false
else
true
end
end