I'm new to ruby, later I asked about ways to refactor code (Parsing GPS data), one man gave me direction, so I made class for Median filter and it will be great to read review for my work. What guys are you thinking about it? Is it still smells?class MedianFilter  attr_accessor :step  def initialize(step, keys)    raise ArgumentError.new('Step must be odd and higher than 3') if step < 3 && step % 2 != 1    raise ArgumentError.new('Keys must be array of symbols') unless keys.kind_of?(Array) && keys.count > 0    @step = step    @keys = keys  end  def filter(param_values)    process_values param_values.clone  end  def filter!(param_values)    process_values param_values  end  private  def process_values(values)    half_step = (@step - 1) / 2    values.each_index do |i|      item = values[i]      median_points = [ values[ [0, i - half_step].max ],                        item,                        values[ [values.count - 1, i + half_step].min ] ]      @keys.each do |key|        item[key] = median_points.map { |x| x[key] || 0 }.sort[half_step]      end    end    values  endend
Class for median filter
ruby
Your code has some bad side-effects. There's no real difference between filter and filter! since they both modify the input values directly.It seems that your input is something like:[{foo: 2}, {foo: 80}, {foo: 6}, {foo: 3}]in other words, an array of hashes.When you call filter, you duplicate the array - but the hash objects are still the same objects. So when you calculate the median, and set item[key] = ..., you're modifying the same hash objects. Duplicating the array has no real effect; you're not modifying the array itself, only the objects inside the array. And those objects are the same even if you duplicate the array.An example:filter = MedianFilter.new(3, [:foo])original = [{foo: 2}, {foo: 80}, {foo: 6}, {foo: 3}]modified = filter.filter(values)modified # => [{:foo=>2}, {:foo=>6}, {:foo=>6}, {:foo=>3}]original # => [{:foo=>2}, {:foo=>6}, {:foo=>6}, {:foo=>3}]In the example, modified is a different array object than original, but the contents of both arrays have been changed. In order words: Your filter method has side-effects.And I doubt that's what you want.Anyway, about the code:Another bug: Your exception says that step must be odd and higher than 3, but that's not what you actually check. You code checks whether step is below 3 and even. So I can pass in 4 and not get an error, even though it's an even number. Or I can pass in -3 and not get an error, even though is less than 3.Your error message is also a little misleading. It says higher than 3 when it fact it can be 3.You check of keys only checks for a non-empty array - it doesn't check that the array only contains symbols. I could pass in [foo, 42, Date.new] without the initializer raising an error.There are a few built-in methods that would make the checks more idiomatic and readable. For instance, step % 2 != 1 can be written as step.even?, and keys.count > 0 can be written as !keys.empty?I'd probably rename step to window_size, since I think that's a better name. Or you might simply redefine it as the number of neighbors to include - i.e. what you currently call half_step.I'd simply skip the keys argument, and instead provide the input data in a different way. It would keep this class simple and generic.I'm not sure this actually needs to be a class, since it doesn't really need to maintain state. It could just be a method.As for the process method, you can use each_cons to get the windows you want to find the median for. You just need to pad the array a little. Here's a simplified example:# example inputvalues = [2, 80, 6, 3]window_size = 3neighbors = (window_size / 2).floor# create an array with extra elements at the start and endtemp = ([values.first] * neighbors) + values + ([values.last] * neighbors)# calculate mediansmedians = temp.each_cons(window_size).map { |window| window.sort[neighbors] }medians # => [2, 6, 6, 3]