Help Refactoring This Nasty Ruby If/Else Statement

Help refactoring this nasty Ruby if/else statement

Try this. I rewrote it using case and regular expressions. I also used :symbols instead of "strings" for the return values, but you can change that back.

tracking_service = case number
  when /^.Z/ then :ups
  when /^Q/ then :dhl
  when /^96.{20}$/ then :fedex
  when /^[HK].{10}$/ then :ups
else
  check_response(number) if num_length == 18 || num_length == 20
  case num_length
    when 17 then :dhlgm
    when 13, 20, 22, 30 then :usps
    when 12, 15, 19 then :fedex
    when 10, 11 then :dhl
    else false
  end
end

Help me refactor this nasty Ruby if/else statement

def update
    rayon = Rayon.find(params[:id])

    unless rayon.update_attributes(params[:rayon])
        flash[:error] = "Rayon not updated."
        return redirect_to :back
    end

    puid = params[:user_id]
    empty = rayon.users.empty?

    if puid == "" or (not empty and rayon.users.last.id.eql?(puid))
        msg = "Rayon updated.",
    else
        msg = "Rayon #{rayon.name} assigned to #{rayon.actual_user.name}.",
        rayon.colporteur_in_rayons.last.update_attributes(
            :until_date => Time.now) unless empty
        Rayon.assign_user(rayon.id, puid)
    end

    flash[:success] = msg[msg_i]
    return redirect_to rayons_path
end

Refactor ugly if/else statement in Rails View

Sometimes templates are just messy and you can only clean up detail. Refactoring into a parameterized partial will help. For goodness sake, use a case. And consider switching to HAML. It eliminates a lot of the visual clutter.

<%= render 'question_field', f: f, type: question.field_type %>

Then in _question_field.erb,

<%= case type %>
  <% when 'text_area' %>
    <% f.text_area :content, class: 'form-control question-field', %>
        <% data: { question: question.id, filter: @filter }, %>
        <% value: question.answer(@assessment).try(:content) %>
  <% when ... %>
<% end %>

Note common industrial practice is to pick a max line length and stick to it: 100 and 120 are pretty common. Also, use the new symbol key notation for hashes. The old hook-and-arrow is too noisy.

In HAML:

= case type
  - when 'text_area'
    - f.text_area :content, class: 'form-control question-field',
      data: { question: question.id, filter: @filter },
      value: question.answer(@assessment).try(:content)
  - when ...

How to refactor long if statements

You might find it's neater if you take advantage of Ruby's various if/unless syntaxes and the case statement. If you were to wrap is in a method you could also take advantage of return.

def check_record(record)
  return unless record.valid?

  case record.some_property
  when 1
    do_something
  when 2
    do_whatever
  when 3
    do_a_dance
  end
end

Need help refactoring and speeding up if statement for view

instead of these double checks

    document.admin_url && document.admin_url.length > 0

you can use

   document.url.to_s.blank?

it will check for nil and empty strings

figuring out/refactoring this if/else for a voting system

There's lots of ways to do it. Here's one. I presume up_or_down will be passed as +1 for upvote and -1 for downvote. Don't overcomplicate things.

def update_vote(up_or_down)
  self.value = self.value == up_or_down ? 0 : up_or_down
end

It's simple if you think of the logic this way:
If user clicks same thing, reset to zero. Otherwise set it to the value clicked.

Refactoring a nested if statement: first check if block exists, then apply another if statement on block.call

I can offer tricks, but what I think this code really needs is some object oriented programming. However, without knowing the semantics of what you're doing it's difficult to come up with an improved design. So, a trick instead.

You might consider giving the block a default value. As noted in comments, there's currently some ambiguity in what you want to do. Here I assume the semantics of your second snippet:

def top_method(&block)
  block ||= lambda {}
  if block.call == 1
    another_method_1
    another_method_2
  else
    another_method_3
  end
end

If no block is passed in, then block is set to lambda {}. Lambda behaves, in this case, just like a block: it responds to call, and has a return value. In this case, having an empty body, it returns nil. Since nil is not equal to 1, the else part of the if will be executed.

Why are else statements discouraged in Ruby?

Let me begin by saying that there isn't really anything wrong with your code, and generally you should be aware that whatever a code quality tool tells you might be complete nonsense, because it lacks the context to evaluate what you are actually doing.

But back to the code. If there was a class that had exactly one method where the snippet

if Rails.env.test? || Rails.env.development?
    # Do stuff
else
    # Do other stuff
end

occurred, that would be completely fine (there are always different approaches to a given thing, but you need not worry about that, even if programmers will hate you for not arguing with them about it :D).

Now comes the tricky part. People are lazy as hell, and thusly code snippets like the one above are easy targets for copy/paste coding (this is why people will argue that one should avoid them in the first place, because if you expand a class later you are more likely to just copy and paste stuff than to actually refactor it).

Let's look at your code snippet as an example. I'm basically proposing the same thing as @Mik_Die, however his example is equally prone to be copy/pasted as yours. Therefore, would should be done (IMO) is this:

class Foo
  def initialize
    @target = (Rails.env.test? || Rails.env.development?) ? self : delay
  end

  def deliver_email_verification_instructions
    @target.deliver_email_verification_instructions!
  end
end

This might not be applicable to your app as is, but I hope you get the idea, which is: Don't repeat yourself. Ever. Every time you repeat yourself, not only are you making your code less maintainable, but as a result also more prone to errors in the future, because one or even 99/100 occurrences of whatever you've copied and pasted might be changed, but the one remaining occurrence is what causes the @disasterOfEpicProportions in the end :)

---

Another point that I've forgotten was brought up by @RayToal (thanks :), which is that if/else constructs are often used in combination with boolean input parameters, resulting in constructs such as this one (actual code from a project I have to maintain):

class String
  def uc(only_first=false)
    if only_first
      capitalize
    else
      upcase
    end
  end
end

Let us ignore the obvious method naming and monkey patching issues here, and focus on the if/else construct, which is used to give the uc method two different behaviors depending on the parameter only_first. Such code is a violation of the Single Responsibility Principle, because your method is doing more than one thing, which is why you should've written two methods in the first place.

---

Related Topics