2
\$\begingroup\$

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
\$\endgroup\$
  • \$\begingroup\$ Did you ask about it here? Do you mind adding a link to the original question for context? \$\endgroup\$ – RubberDuck Oct 25 '14 at 10:25
  • \$\begingroup\$ @RubberDuck, fix it. \$\endgroup\$ – Aleksandr K. Oct 25 '14 at 10:55
3
\$\begingroup\$

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]
    
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks so much for that great and helpful answer. I have question about 'it could be a method' - should I put this method in module or in class that using it? \$\endgroup\$ – Aleksandr K. Oct 25 '14 at 11:45
  • \$\begingroup\$ In the previous question (link in post) I got answer that parsers should be the classes TrackParser > GPXParser, TESParser, etc. Could you please write your opinion about it? \$\endgroup\$ – Aleksandr K. Oct 25 '14 at 11:47
  • \$\begingroup\$ @AleksandrK. Yes, the method should still be in a class somewhere - it should be where you need it. And you got some good advice in your last post; the parsers probably should be separate classes. Their job is to take a file, and return an array of points that follow a standard format. However, the median filtering is something that would happen after you've parsed the files, so that probably shouldn't be in the separate parser classes, but somewhere else. \$\endgroup\$ – Flambino Oct 25 '14 at 12:28
  • \$\begingroup\$ I've thought a lot about your snippet and modify it for my purposes, here what I've done: temp.each_cons(window_size).map do |window| keys.each do |key| window[neighbors][key] = window.map { |x| x[key] }.sort[neighbors] end window[neighbors] end could you please write your opinion? \$\endgroup\$ – Aleksandr K. Oct 26 '14 at 14:59
  • \$\begingroup\$ @AleksandrK. Looks OK to me \$\endgroup\$ – Flambino Oct 26 '14 at 19:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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