How to refactor this Ruby code?

Question

I created the following, that works, but it seems very cryptic. Is there a way to write it in a more Ruby-esque or understandable way?

This method removes the lower factors below a number. So, 10.high_factors returns [6,7,8,9,10]. 6 is divisible by 2, so 2 is removed. There are no multiples greater than 6 on the list, so it stays.

class Fixnum
  def high_factors
    # Get the numbers that are not divisible by lower ones below self
    list = (2..self).to_a
    2.upto(self).each do |i|
      ((i+1)..self).each { |j| list.delete i if j.is_divisible_by? i }
    end

    list
  end

  def is_divisible_by? divisor
    self % divisor == 0
  end
end

Ruby 1.9.3

I don't understand this... why not just return [self/2+1 .. self] — halfelf
– halfelf, Commented Sep 10, 2012 at 2:52

fgp · Accepted Answer · 2012-09-10 17:31:17Z

5

The result of your method will always be a list of the numbers from (N/2) + 1 to N.

For every i<=(N/2), 2*i will also be on the list.

For every j >= (N/2)+1 on the list, there will not be a k=x*j on it where x is an integer greater than 1, because 2*j > N.

Therefore, if your method returns just ((self/2 + 1)..self).to_a it will also work as you wish.

edited Sep 10, 2012 at 17:31

answered Sep 10, 2012 at 2:56

fgp

4282 silver badges6 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

B Seven Over a year ago

Although this did not refactor any Ruby code, it really is the best answer because it uses math to prove correctness.

Benoît · Accepted Answer · 2012-09-10 02:49:20Z

2

What about this? Just removing numbers that are divisible by upper ones.

class Fixnum
  def high_factors
    # Get the numbers that are not divisible by lower ones below self
    (2..self).reject do |i|
      (i+1..self).any? { |j| j.divisible_by?(i) }
    end
  end

  def divisible_by?(divisor)
    self % divisor == 0
  end
end

ps: in ruby, it is common to omit the 'is_' at the beginning of boolean function since we can add the ?

edited Sep 10, 2012 at 2:49

answered Sep 10, 2012 at 2:44

Benoît

14.5k7 gold badges65 silver badges89 bronze badges

Comments

AJcodez · Accepted Answer · 2012-09-10 09:51:02Z

1

Heres mine

def high_factors
  ary = (2..self).to_a
  ary.reject do |factor|
    ary.index {|num| num != factor and num % factor == 0}
  end
end

It works cause Array#index returns nil if it can't find a suitable match.

answered Sep 10, 2012 at 9:51

AJcodez

34.5k21 gold badges88 silver badges119 bronze badges

Collectives™ on Stack Overflow

How to refactor this Ruby code?

3 Answers 3

1 Comment

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

1 Comment

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related