1
\$\begingroup\$

Task:

"Implement a method 'gruppiere', in a way that it can be invoked on all enumerable objects (Enumerable). The method receives a block and returns a hash. The items of the enumerable a grouped within the hash according to the return-value of the block."

What they like to have is an own implementation of Ruby's "#group_by"-method: Ruby-Docs Enumerable

My solution:

module Enumerable
    def gruppiere()
        ret = {}

        self.each { |item| 
            key = yield item

            if ret[key] == nil
                tmp = []
                tmp << item
                ret[key] = tmp
            else
                ret[key] << item
            end
        }

        ret
    end
end

puts [1, 2, 3, 4].gruppiere { |i| i % 2 == 0 } # Result: {false=>[1, 3], true=>[2, 4]}

Works well. But I'm sure it could be done better.

Is there a less verbose way to solve the task?

Is my code written in a good way and manner? What could be improved?

\$\endgroup\$
0
0
\$\begingroup\$

Linting

You should run some sort of linter or static analyzer on your code. Rubocop is a popular one, but there are others.

Rubocop was able to detect almost all of the style violations I am going to point out (and even some more), and was able to autocorrect almost all of them.

Testing

There is no automated testing in your code. Apart from the single example at the very end (which is not automated), there is no testing at all.

You should always strive to have as close to 100% test coverage as possible. It doesn't really matter if you have unit tests, functional tests, integration tests, end-to-end tests, or a mix of them, but you should have tests, and they should be automated.

In this particular case, since you are implementing a Ruby core method, there are already plenty of tests written for you in the Ruby/Spec project as well as the YARV test suite.

Running the Ruby/Spec tests against your code yields 3 errors, 1 failure, and only 3/7 passing tests.

The YARV test suite has 1/2 passing assertion and 1 error.

Indentation

The standard indentation style in the Ruby community is 2 spaces, not 4.

Empty parameter list

When you define a method without parameters, don't write out an empty parameter list. Just leave out the parameter list completely.

Instead of

def gruppiere()

you should have

def gruppiere

Naming

ret and tmp aren't really good variable names. Try to make them more expressive so that they reveal their intent. Okay, so it's a temporary variable, but what does it do, what is it for, why is it there?

Normally, the reason to introduce a temporary variable is to give an intention-revealing name to some sub-expression. But tmp is not very intention-revealing.

At least, spell them out. You are not going to wear out your keyboard by writing temp instead of tmp, I promise.

Unnecessary self

self is the implicit receiver in Ruby if you don't explicitly provide one. There is no need to explicitly provide self as the receiver (except in some very limited special circumstances).

Instead of

self.each

just write

each

Block delimiters

The standard community style for block delimiters is to use { / } for single-line blocks and do / end for multi-line blocks.

There is a small minority that follows a different style: { / } for functional blocks and do / end for imperative blocks

Whichever style you follow, your block should use do / end since it is both multi-line and imperative.

Explicit equality check against nil

You should not check for equality with nil. There is a method Object#nil? which returns false for all objects, and the only override of this method is NilClass#nil?, which returns true. In other words: the only object that will ever respond with true to nil? is nil.

Instead of

ret[key] == nil

you should write

ret[key].nil?

Unnecessary array mutation

In this piece of code:

tmp = []
tmp << item

You assign an empty array to tmp, then immediately append item to the empty array. That's exactly the same as assigning an array with one item to tmp in the first place:

tmp = [item]

Unnecessary temporary variable

Once we have made the above change, this piece of code:

tmp = [item]
ret[key] = tmp

doesn't really need the temporary variable anymore:

ret[key] = [item]

See? The reason why you didn't find a good name for that variable, is that it shouldn't even be there!

Hash default value

Actually, we can get rid of that whole conditional expression by instead making sure our result hash automatically initialized non-existent keys with an empty array the first time the key is accessed:

def gruppiere
  ret = Hash.new {|hash, key| hash[key] = [] }

  each do |item|
    key = yield item
    ret[key] << item
  end

  ret
end

This, by the way, also gets rid of one of the things Rubocop was complaining about but was unable to auto-correct: the method was too long.

Higher-level iteration methods

each is a very low level iteration method. It is usually barely needed in Ruby. As a general rule, in Ruby

  • When you are writing a loop, you are definitely doing something wrong.
  • When you use each, you are very likely doing something wrong.

The pattern you use in your code looks like this: you create a result object, then accumulate results in this object, and at the end return it. This pattern is actually a Fold. In Ruby, fold is provided by Enumerable#inject (and its alias Enumerable#reduce) and Enumerable#each_with_object.

Here is what the method would look like using Enumerable#each_with_object:

def gruppiere
  each_with_object(Hash.new { |hash, key| hash[key] = [] }) do |element, result|
    key = yield element
    result[key] << element
  end
end

Iteration protocol

It is standard that iterator methods return an Enumerator when called without a block. We can use the Object#enum_for method to create an Enumerator for our method. We just put the following code as the first line of our method:

return enum_for(__callee__) { size if respond_to?(:size) } unless block_given?

This actually fixes all of the test errors we had.

Test failures

Unfortunately, we have introduced one new test failure with our refactoring to auto-initialize the hash. group_by should not return a Hash that has default_proc set.

We have two choices:

  • Set default_proc to nil.
  • Create a new hash.

I opted for the latter, to create a new empty hash and Hash#merge onto it, to be 100% sure that the default_proc as well as any internal flags are reset to defaults:

def gruppiere
  return enum_for(__callee__) { size if respond_to?(:size) } unless block_given?

  {}.merge(
    each_with_object(Hash.new { |hash, key| hash[key] = [] }) do |element, result|
      key = yield element
      result[key] << element
    end
  )
end

Hash#fetch

There is actually a better option than using a default_proc. Hash#fetch will get the value corresponding to the key if the key exists and otherwise return a value of our choosing:

def gruppiere
  return enum_for(__callee__) { size if respond_to?(:size) } unless block_given?

  each_with_object({}) do |element, result|
    key = yield element
    result[key] = result.fetch(key, []) << element
  end
end

Monkey patching core classes / modules

Monkey patching core modules is generally frowned upon. If you do it, it is good practice to put your monkey patches in a separate mixin with a clear name, and mix that into the class or module you want to monkey patch. That way, it shows up in the inheritance chain, and people can use the name in the inheritance chain to make a guess at the filename, when they find this strange method in their array that they have no idea where it comes from.

Refinements

NOTE! This advice is controversial.

When monkey patching, it is a good idea to wrap your monkey patch into a Refinement, so that consumers can only pull it in when they need it, and it doesn't pollute other parts of your code.

Unfortunately, most Ruby implementations don't implement Refinements, so as nice as the benefits are, it essentially makes your code non-portable.

The Result

If we put all of the above together, we end up with something roughly like this:

module EnumerableGruppiereExtension
  def gruppiere
    return enum_for(__callee__) { size if respond_to?(:size) } unless block_given?

    each_with_object({}) do |element, result|
      key = yield element
      result[key] = result.fetch(key, []) << element
    end
  end
end

module EnumerableWithGruppiere
  refine Enumerable do
    include EnumerableGruppiereExtension
  end
end

using EnumerableWithGruppiere

puts [1, 2, 3, 4].gruppiere(&:even?)
#=> { false => [1, 3], true => [2, 4] }

Addendum: Functional Programming

You tagged your question with , but there is nothing functional about your code. There's looping, there is mutation, there are side-effects.

It is, however, not easy to program in a functional way in Ruby. Neither the core and standard library data structures nor the core and standard library algorithms really lend themselves to Functional Programming.

Here is a purely functional version that does not use mutation, side-effects, or looping:

def gruppiere
  return enum_for(__callee__) { size if respond_to?(:size) } unless block_given?

  inject({}) do |result, element|
    key = yield element
    result.merge({ key => result.fetch(key, []) + [element] })
  end
end

Now, you might ask yourself: that actually doesn't look that bad. Why did I say that Ruby is not amenable to Functional Programming?

The reason for this is performance.

Because Hash and Array are mutable, operations such as Hash#merge and Array#+ can only be implemented by copying the entire data structure. Whereas if Hash and Array were immutable, as they are in a collections library for a functional language, these operations could be implemented by what is called structural sharing, which means that Hash#merge and Array#+ would not return a full copy of the original but rather would return only the updated data and a reference to the old version. This is much more efficient.

For example, here is what the same code would look like in Scala:

def [A, B](seq: Iterable[A]).gruppiere(classifier: A => B): Map[B, Iterable[A]] = 
  seq.foldLeft(Map.empty[B, IndexedSeq[A]]) {
    (result, element) => {
      val key = classifier(element)
      result updated(key, result.getOrElse(key, IndexedSeq.empty[A]) :+ element)
    }
  }

Iterable(1, 2, 3).gruppiere { _ % 2 == 0 }
//=>Map(false -> Iterable(1, 3), true -> Iterable(2))

As you can see, it looks more or less identical. Some names are different (e.g. foldLeft instead of inject, getOrElse instead of fetch, etc.), and there are some static type annotations. But other than that, it is the same code. The main difference is in the performance: Map.updated does not copy the map, it returns a map which shares all its data except the one updated key-value-pair with the original. The same applies to IndexedSeq.:+ (an alias for IndexedSeq.append).

\$\endgroup\$
1
  • \$\begingroup\$ I know that votes are anonymous, and for a good reason, but if the downvoter would like to explain what is "not useful" about my answer, I would be happy to improve it! (Especially since it seems from the voting history that the person in question upvoted my answer, then a week later reversed their decision, which I assume means that they found a flaw in my answer after analyzing it.) \$\endgroup\$ – Jörg W Mittag Jun 1 '20 at 9:15

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.