# Printing flight statuses

I have a serious nesting of ifs in a helper code and I would like to making cleaner. I would like to avoid case if possible as well.

I know there is probably a more object-oriented approach to this but I can't seem to know how.

I'm flooded with stuff like this:

  def print_flight_options_status(invitation)
if invitation.group.travel_class == 'none'
not_allowed
elsif invitation.refused_flight_options?
not_needed
elsif invitation.selected_flights?
waiting_reservation
elsif invitation.flight_options.empty?
not_sent_yet
elsif invitation.requested_more_flight_options?
rejected
else
waiting_guest_input
end
end

def print_event_terms_status(invitation)
if invitation.event_terms_status.nil?
"<span class='grey_highlight pj_cat'>Aguardando</span>"
elsif invitation.accepted_event_terms?
"<span class='green_highlight pj_cat'>Aceito</span>"
elsif invitation.rejected_event_terms?
elsif invitation.cancelled_event_terms?
end.html_safe
end

• If the return values are mutually exclusive, it seems like it would make more sense to have print_flight_options_status contain a symbol that is updated by the various things you are querying. – Noah Jul 27 '12 at 18:07
• You're right, this would probably be cleaner, but the complexity would not be much reduced. – Pedro Nascimento Jul 30 '12 at 17:37
• print_flight_options_status looks fantastic as it is, I wouldn't write it as in-line ifs. At most, to reduce the line-count, I'd use if condition then value ... , but it won't look as nice. In print_event_terms_status I'd just use helpers for the HTML tags instead of building them by hand. – tokland Aug 3 '12 at 19:39

I prefer this style:

  def print_flight_options_status(invitation)
return not_allowed         if invitation.group.travel_class == 'none'
return not_needed          if invitation.refused_flight_options?
return waiting_reservation if invitation.selected_flights?
return not_sent_yet        if invitation.flight_options.empty?
return rejected            if invitation.requested_more_flight_options?

waiting_guest_input
end


UPD

About second code-snippet. I think in this case you can use decorators (for example draper) for invitations, so:

def print_event_terms_status(invitation)
InvitationDecorator.decorate(invitation).status_in_html
end


Where InvitationDecorator:

class InvitationDecorator < Draper::Base
def status_in_html
if invitation.event_terms_status.nil?
"<span class='grey_highlight pj_cat'>Aguardando</span>"
...
end
end

• The problem with inline return + conditionals is that the exit points of the method grow up... I know it's a common pattern in Ruby but personally I consider it dubious. – tokland Aug 3 '12 at 19:41

There is a case-variant which may be a bit better:

x = ''
case
when x == 'xxy'
puts '=xxy'
when x.empty?
puts 'is empty'
else
puts "well, I don't know, what it is"
end


In your case, you could use:

def print_flight_options_status(invitation)
case
when invitation.group.travel_class == 'none'
not_allowed
when invitation.refused_flight_options?
not_needed
when invitation.selected_flights?
waiting_reservation
when invitation.flight_options.empty?
not_sent_yet
when invitation.requested_more_flight_options?
rejected
else
waiting_guest_input
end
end


You may also define a kind of with-statement:

module With
def with(&block)
self.instance_eval &block
end
end

['a', ''].each{|test|
test.extend(With)
p test.with{
if empty?
:empty
elsif self == 'a'
:a
else
:else
end
}
}


def print_flight_options_status(invitation)
invitation.extend(With)
invitation.with{
if group.travel_class == 'none'
not_allowed
elsif refused_flight_options?
not_needed
elsif selected_flights?
waiting_reservation
elsif flight_options.empty?
not_sent_yet
elsif requested_more_flight_options?
rejected
else
waiting_guest_input
end
}
end


Advantage: You don't need to repeat invitation..

IMHO both variants look ok regarding ifs. They clearly express business logic and well formatted. Maybe I'd separate a bit markup from UI Logic:

def print_event_terms_status(invitation)
span =
->(cls, text) { "<span class='#{class} pj_cat'>#{text}</span>".html_safe }
if invitation.event_terms_status.nil?
span['grey_highlight', 'Aguardando']
elsif invitation.accepted_event_terms?
span['green_highlight', 'Aceito']
elsif invitation.rejected_event_terms?
elsif invitation.cancelled_event_terms?
end
end


The point of CSS is to decouple the presentation from the logic. Therefore, you should not hard-code colours into the CSS class names. Each colour should be mentioned just once, in the style rule only:

.pj_cat.pending {
background-color: gray;
}
.pj_cat.accepted {
background-color: green;
}
.pj_cat.rejected {
background-color: red;
}
.pj_cat.cancelled {
background-color: yellow;
}


Otherwise, if the site's colour theme ever changes, the code is going to be confusing.