# Ruby Exercise: Implement your own “#group_by” method

"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?

# 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.

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).

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.

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

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] }


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).

• 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.) – Jörg W Mittag Jun 1 '20 at 9:15