Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
def divisible?(n)
  if n % 1 == 0 &&
    n % 2 == 0 &&
    n % 3 == 0 &&
    n % 4 == 0 &&
    n % 5 == 0 &&
    n % 6 == 0 &&
    n % 7 == 0 &&
    n % 8 == 0 &&
    n % 9 == 0 &&
    n % 10 == 0 &&
    n % 11 == 0 &&
    n % 12 == 0 &&
    n % 13 == 0 &&
    n % 14 == 0 &&
    n % 15 == 0 &&
    n % 16 == 0 &&
    n % 17 == 0 &&
    n % 18 == 0 &&
    n % 19 == 0 &&
    n % 20 == 0
    return true
  else 
    return false
  end
end
share|improve this question
add comment

2 Answers

up vote 13 down vote accepted

Ruby refactor: use Enumerable.all?:

def divisible?(n)
  (1..20).all? { |x| n % x == 0 }
end

Mathematical refactor:calculate the least common multiple of the integers in the range (Integer.lcm is available from Ruby 1.9):

def divisible?(n)
  n % (1..20).reduce(:lcm) == 0
end

This second snippet is, of course, more efficient once you pre-calculate (1..20).reduce(:lcm) only once and store it somewhere.

share|improve this answer
    
Thanks! This is so much cleaner. –  Jon Smooth Feb 2 '13 at 21:26
    
+1 for the reduce version. Note this requires 1.9. –  Mark Thomas Feb 4 '13 at 12:41
    
Would the first example benefit from 20..1 rather than 1..20? Less values of n are divisible by the higher divisors, leading to faster exit. Checking for factors seems like a case where premature optimisation is reasonable. (I don't know Ruby, maybe 1..20 is not equivalent to the big series of ifs in the question?) –  Sean Feb 4 '13 at 22:01
    
@Sean: yeah, good point, you could write (1..20).reverse_each.all? .... Anyway, one you've decided to do it in a mathematically sound way you might as well go for the second snippet, which is simple enough to understand and much faster. –  tokland Feb 4 '13 at 22:14
add comment

Note that you're doing this, which is a pretty egrigious use of if/else:

if boolean
  return true
else
  return false
end

If the branches of your if/else are return true and return false, you should just return the condition you're testing! In Ruby, you don't even need return, just let the condition "fall off" the end of the method.

Ignoring the ability to clean up your condition itself, a first pass at cleaning up your function could be:

def divisible?(n)
  n % 1 == 0 &&
  n % 2 == 0 &&
  n % 3 == 0 &&
  n % 4 == 0 &&
  n % 5 == 0 &&
  n % 6 == 0 &&
  n % 7 == 0 &&
  n % 8 == 0 &&
  n % 9 == 0 &&
  n % 10 == 0 &&
  n % 11 == 0 &&
  n % 12 == 0 &&
  n % 13 == 0 &&
  n % 14 == 0 &&
  n % 15 == 0 &&
  n % 16 == 0 &&
  n % 17 == 0 &&
  n % 18 == 0 &&
  n % 19 == 0 &&
  n % 20 == 0
end
share|improve this answer
add comment

Your Answer

 
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.