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
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 functional-programming, 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).
