2
\$\begingroup\$

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?
      "<span class='red_highlight pj_cat'>Declinado</span>"
    elsif invitation.cancelled_event_terms?
      "<span class='yellow_highlight pj_cat'>Cancelado</span>"
    end.html_safe
  end
\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – Noah Jul 27 '12 at 18:07
  • \$\begingroup\$ You're right, this would probably be cleaner, but the complexity would not be much reduced. \$\endgroup\$ – Pedro Nascimento Jul 30 '12 at 17:37
  • \$\begingroup\$ 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. \$\endgroup\$ – tokland Aug 3 '12 at 19:39
4
\$\begingroup\$

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
\$\endgroup\$
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – tokland Aug 3 '12 at 19:41
2
\$\begingroup\$

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
  }  
}

In your case:

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..

\$\endgroup\$
1
\$\begingroup\$

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?
    span['red_highlight', 'Declinado']
  elsif invitation.cancelled_event_terms?
    span['yellow_highlight', 'Cancelado']
  end
end
\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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