# Finding locations with a distance between 1 and 5000

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


## migrated from stackoverflow.comApr 26 '12 at 3:30

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

• begin by splitting in two methods: one with distance condition, other with conditionnal scope – apneadiving Apr 25 '12 at 22:11
• 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? – Alexey Apr 29 '12 at 18:57

## 3 Answers

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


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

• 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? – Benjamin Udink ten Cate Apr 25 '12 at 23:05
• 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 – gabitzish Apr 25 '12 at 23:08

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

• Just because an object responds to to_i doesn't mean it represents an integer. – Mark Thomas Apr 26 '12 at 1:24
• @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 – Peter Brown Apr 26 '12 at 2:17