4
\$\begingroup\$

I'm wondering how I could refactor this code. It may look a little reiterative and probably there's a shorter method to accomplish the same.

def self.location(distance=100,location)
  if distance.is_a? Integer 
    if distance.between?(1,5000)
      distance = distance
    elsif distance < 1
      distance = 1
    elsif distance > 5000
      distance = 5000
    end
  else 
    distance = 100
  end
  if location
    within(distance, :origin => location)
  else  
    find(:all)  
  end  
end
\$\endgroup\$

migrated from stackoverflow.com Apr 26 '12 at 3:30

This question came from our site for professional and enthusiast programmers.

  • 2
    \$\begingroup\$ begin by splitting in two methods: one with distance condition, other with conditionnal scope \$\endgroup\$ – apneadiving Apr 25 '12 at 22:11
  • \$\begingroup\$ I think checking if it's Integer is a smell. What if I pass 1.0 to the function? Why would it be replaced with 100? \$\endgroup\$ – Alexey Apr 29 '12 at 18:57
7
\$\begingroup\$

Shorter, and while min/max was tricky, IMO, this is easier to understand:

def self.location(distance=100,location)
    distance = 100 unless distance.is_a?(Integer)
    distance = 1 if distance < 1
    distance = 5000 if distance > 5000

    if location
        within(distance, :origin => location)
    else  
        find(:all)  
    end  
end
\$\endgroup\$
4
\$\begingroup\$

I've replaced the part where you take care that distance is between 1 and 5000 with distance = [1, [distance, 5000].min].max .

def self.location(distance=100,location)
  if distance.is_a? Integer 
    distance = [1, [distance, 5000].min].max
  else 
    distance = 100
  end
  if location
    within(distance, :origin => location)
  else  
    find(:all)  
  end  
end
\$\endgroup\$
  • \$\begingroup\$ whats exactly happening here [1, [distance, 5000].min].max? Is it picking the lower value from distance and 5000 then the highest value from the product of the earlier comparison and 1? \$\endgroup\$ – Benjamin Udink ten Cate Apr 25 '12 at 23:05
  • \$\begingroup\$ we are comparing the minimum between distance and 5000 with 1, and returning the maximum there. If distance is greater than 5000, we will compare 5000 with 1, and return 5000, else if distance is less than 5000, we compare it with 1. Now if distance is less than 1, we return 1, else we return the distance wich is between 1 and 5000 \$\endgroup\$ – gabitzish Apr 25 '12 at 23:08
2
\$\begingroup\$

First, I would not set the distance to 100 if it's not an integer. What if someone uses a Fixnum such as 150.0? I would check to see if it responds to to_i.

Second, I would break the distance out into a separate method because it's easier to follow outside the context of another method.

def self.location(distance, location)
  return find(:all) unless location

  distance = normalize_distance(distance)

  within(distance, :origin => location)
end

def self.normalize_distance(distance=100)
  return 100 unless distance.respond_to? :to_i
  return 1 if distance < 1
  return 5000 if distance > 5000

  distance
end
\$\endgroup\$
  • \$\begingroup\$ Just because an object responds to to_i doesn't mean it represents an integer. \$\endgroup\$ – Mark Thomas Apr 26 '12 at 1:24
  • 1
    \$\begingroup\$ @MarkThomas let me clarify the point I was trying to make. Checking an object's class rather than its behavior is going to make the code less flexible. If a Fixnum or Float were passed into the location method, why should it behave any different? 100.0 is equivalent to 100 but they are different classes and only one is an integer. This sort of flexibility is called duck typing: en.wikipedia.org/wiki/Duck_typing \$\endgroup\$ – Peter Brown Apr 26 '12 at 2:17

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.