Class for median filter

Question

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

  end

end

Answer 1

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 input
  values = [2, 80, 6, 3]
  window_size = 3
  neighbors = (window_size / 2).floor
  
  # create an array with extra elements at the start and end
  temp = ([values.first] * neighbors) + values + ([values.last] * neighbors)
  
  # calculate medians
  medians = temp.each_cons(window_size).map { |window| window.sort[neighbors] }
  
  medians # => [2, 6, 6, 3]