up vote 2 down vote favorite

Im wondering how could I 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
share|improve this question
2  
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

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

3 Answers

up vote 6 down vote accepted

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
share|improve this answer
up vote 4 down vote

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
share|improve this answer
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
up vote 2 down vote

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

Your Answer

 
or
required, but never shown
discard

By posting your answer, you agree to the privacy policy and terms of service.

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