Asp Forum - Hash.merge_add! extension - how does this code look?

Greg Hauptmann

10/26/2008 11:20:00 AM

[Note: parts of this message were removed to make it a legal post.]

Hi,
I wanted a way to be able to "add" values to a Hash such that it keeps the
old and the new values. For examples example adding an item to a hash for
which the keys are dates. Here's a first cut. Any feedback on coding
style etc?

================================
class Hash
# Merges a hash with only one item into your hash. If there is already a
# hash entry for that key the both existing value(s) & new value are kept
via
# use of an Array
def merge_add!(h)
raise "Parameter passed in not a hash" if !h.instance_of?(Hash)
raise "Can only pass hash with one item to be added" if h.length > 1
h_key = h.to_a[0][0]
h_value = h.to_a[0][1]
if self.has_key?(h_key)
existing_value = self[h_key]
existing_value_as_array = existing_value.instance_of?(Array) ?
existing_value : [existing_value]
new_value = existing_value_as_array << h_value
self[h_key] = new_value
else
h_value.instance_of?(Array) ? self.merge!(h) : self.merge!( {h_key =>
[h_value]} )
end
end
end
================================
require File.expand_path(File.dirname(__FILE__) + '/../spec_helper')

describe "merge_add!" do
before(:each) do
@empty_hash = {}
@hash_with_number = {100 => 1}
@hash_with_array = {100 => [1]}
end

it "should raise an error if there is no object passed to the method" do
lambda{ @empty_hash.merge_add!() }.should raise_error(ArgumentError)
end
it "should raise an error if the object passed is not a Hash" do
lambda{@empty_hash.merge_add!(123)}.should raise_error(RuntimeError,
"Parameter passed in not a hash")
end
it "should raise an error if the number of items in the hash in not 1" do
lambda{@empty_hash.merge_add!({101 => 2, 102 => 3})}.should
raise_error(RuntimeError, "Can only pass hash with one item to be added")
end
it "should create a new Array & wrap new value as an array" do
@empty_hash.merge_add!( {101 => 2} )
@empty_hash[101].should eql([2])
end
it "should create a new Array & use the array passed as the value" do
@empty_hash.merge_add!( {101 => [2]} )
@empty_hash[101].should eql([2])
end
it "should migrate existing value to an Array, then add the new value, if
existing value is NOT an array" do
@hash_with_number.merge_add!( {100 => 2} )
@hash_with_number[100].should eql([1, 2])
end
it "should migrate add the new value to existing, if existing value IS an
array" do
@hash_with_array.merge_add!( {100 => 2} )
@hash_with_array[100].should eql([1, 2])
end

end

================================

tks

17 Answers

David A. Black

10/26/2008 12:13:00 PM

Hi --

On Sun, 26 Oct 2008, Greg Hauptmann wrote:

> Hi,
> I wanted a way to be able to "add" values to a Hash such that it keeps the
> old and the new values. For examples example adding an item to a hash for
> which the keys are dates. Here's a first cut. Any feedback on coding
> style etc?

Just a quick (1/2-cup of coffee) point:

> ================================
> class Hash
> # Merges a hash with only one item into your hash. If there is already a
> # hash entry for that key the both existing value(s) & new value are kept
> via
> # use of an Array
> def merge_add!(h)

That's not a good name for it, because there's no non-bang merge_add
method. The ! in method names only makes sense (with very few,
very specialized exceptions, like the way Jim W. uses them in Builder)
if the methods come in pairs: one regular method and one "dangerous"
method ("dangerous" being Matz's term to describe the meaning of the
!).

The reasoning is as follows.

Bang method names never have, and never will, coincide completely with
receiver-altering methods in Ruby. That's just not a possibility, and
has never been envisioned. (Consider Array#pop, String#replace, and
many, many more.) So adding a bang just because the receiver is being
changed doesn't make sense, and dilutes the convention of the
regular/dangerous pairing.

Putting a bang on an unpaired method name just because the method
seems "dangerous", in a unary way, doesn't make sense either. Rails
does this, and it's about my least favorite thing about the Rails
source code. If ! just means "This method does something dramatic!",
we're back to a very fuzzy, uninformative, impressionistic use of !.

Matz had it right: the regular/dangerous pairing of otherwise
same-named methods is an incredibly creative and useful application of
the bang. It actually tells you something. Every time people complain
because gsub! returns nil if the receiver doesn't change, all I can
think is: "You were warned! There's a bang in the name; that means it's
similar to gsub but it's dangerous. Heads up! Go read the docs! Don't
complain!" :-)

In Ruby, unpaired methods always have names that already incorporate
what they do and whether or not they change their receivers. Yes,
there are some glitches (like Array#delete and String#delete being
different as to receiver-changing). But Array#pop isn't Array#pop!
because there's no possible non-destructive "pop"; that would just be
array[-1]. So if you've got an unpaired method name and you feel like
it's not communicating enough, it's best to change the name rather
than add a !, for the reasons above.

Anyway, that's the gist. Sorry to pounce on one thing. I'll have more
coffee and look at the code itself :-)

David

--
Rails training from David A. Black and Ruby Power and Light:
Intro to Ruby on Rails January 12-15 Fort Lauderdale, FL
Advancing with Rails January 19-22 Fort Lauderdale, FL *
* Co-taught with Patrick Ewing!
See http://www.r... for details and updates!

Trans

10/26/2008 12:54:00 PM

On Oct 26, 7:20=A0am, "Greg Hauptmann" <greg.hauptmann.r...@gmail.com>
wrote:
> Hi,
> I wanted a way to be able to "add" values to a Hash such that it keeps th=
e
> old and the new values. =A0For examples example adding an item to a hash =
for
> which the keys are dates. =A0 Here's a first cut. =A0Any feedback on codi=
ng
> style etc?
>
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D
> class Hash
> =A0 # Merges a hash with only one item into your hash. =A0If there is alr=
eady a
> =A0 # hash entry for that key the both existing value(s) & new value are =
kept
> via
> =A0 # use of an Array
> =A0 def merge_add!(h)
> =A0 =A0 raise "Parameter passed in not a hash" if !h.instance_of?(Hash)

You don't need to do this. It is "anti-ducktyping". h could be a hash-
*like* object and that would be okay.

> =A0 =A0 raise "Can only pass hash with one item to be added" if h.length =
> 1

Why artificially limit it to one entry hashes? Supporting any hash
would make it more flexible.

> =A0 =A0 h_key =3D h.to_a[0][0]
> =A0 =A0 h_value =3D h.to_a[0][1]
> =A0 =A0 if self.has_key?(h_key)
> =A0 =A0 =A0 existing_value =3D self[h_key]
> =A0 =A0 =A0 existing_value_as_array =3D existing_value.instance_of?(Array=
) ?
> existing_value : [existing_value]
> =A0 =A0 =A0 new_value =3D existing_value_as_array << h_value
> =A0 =A0 =A0 self[h_key] =3D new_value
> =A0 =A0 else
> =A0 =A0 =A0 h_value.instance_of?(Array) ? self.merge!(h) : self.merge!( {=
h_key =3D>
> [h_value]} )
> =A0 =A0 end
> =A0 end
> end

The idea is a good one. I think implementation could use some
improvement. For comparison have a look at Facets' Hash#collate.

http://facets.rubyforge.org/doc/api/core/classes/Hash.ht...

t

Stefan Rusterholz

10/26/2008 9:45:00 PM

Greg Hauptmann wrote:
> Hi, I wanted a way to be able to "add" values to a Hash such that it
> keeps the old and the new values. For examples example adding
> an item to a hash for which the keys are dates. Here's a first cut.
> Any feedback on coding style etc?

Reading your problem and code I think you missed that Hash#merge
optionally takes a block which seems to solve your issue just fine but
in a more generic manner:

hash = {'foo' => 'bar1'}
hash.merge! 'foo' => 'bar2' do |key, existing, new|
Array(existing) + [new]
end
p hash # -> {"foo"=>["bar1", "bar2"]}

Additionally I would normalize all values to arrays upfront here, makes
dealing with the hash a lot easier.

Regards
Stefan
--
Posted via http://www.ruby-....

Trans

10/26/2008 11:09:00 PM

On Oct 26, 5:45=A0pm, Stefan Rusterholz <apei...@gmx.net> wrote:

> Reading your problem and code I think you missed that Hash#merge
> optionally takes a block which seems to solve your issue just fine but
> in a more generic manner:
>
> hash =3D {'foo' =3D> 'bar1'}
> hash.merge! 'foo' =3D> 'bar2' do |key, existing, new|
> =A0 Array(existing) + [new]
> end
> p hash # -> {"foo"=3D>["bar1", "bar2"]}

Nice. I didn't know it could take a block either. Has that been there
all along?

T.

Greg Hauptmann

10/27/2008 3:48:00 AM

[Note: parts of this message were removed to make it a legal post.]

thanks for highlighting this! Is the quickest way to normalise to Array via
the following?
hashitem.instance_of?(Array) ? hashitem : [hashitem]

On Mon, Oct 27, 2008 at 7:45 AM, Stefan Rusterholz <apeiros@gmx.net> wrote:

> Greg Hauptmann wrote:
> > Hi, I wanted a way to be able to "add" values to a Hash such that it
> > keeps the old and the new values. For examples example adding
> > an item to a hash for which the keys are dates. Here's a first cut.
> > Any feedback on coding style etc?
>
> Reading your problem and code I think you missed that Hash#merge
> optionally takes a block which seems to solve your issue just fine but
> in a more generic manner:
>
> hash = {'foo' => 'bar1'}
> hash.merge! 'foo' => 'bar2' do |key, existing, new|
> Array(existing) + [new]
> end
> p hash # -> {"foo"=>["bar1", "bar2"]}
>
> Additionally I would normalize all values to arrays upfront here, makes
> dealing with the hash a lot easier.
>
> Regards
> Stefan
> --
> Posted via http://www.ruby-....
>
>

Stefan Rusterholz

10/27/2008 7:21:00 AM

Greg Hauptmann wrote:
> thanks for highlighting this! Is the quickest way to normalise to Array
> via
> the following?
> hashitem.instance_of?(Array) ? hashitem : [hashitem]

If you're building up the hash yourself then I'd use the following
idiom:
collector = Hash.new { |h,k| h[k] = [] }
collector['key1'] << 'value1'
collector['key2'] << 'value1'
collector['key1'] << 'value2'
p collector

Simplifies adding to and reading from the hash. But be aware that with
the hash having a different default value than nil, you can't use 'if
collector[key] then' to test for existence of a key anymore, you have to
use has_key?.

If you're not building the hash up yourself, then yes, I'd use what you
wroten in an each loop and override the existing value.

Regards
Stefan
--
Posted via http://www.ruby-....

Greg Hauptmann

10/27/2008 7:41:00 AM

I just noticed the code suggested might have a problem when the
original hash is empty. That is using the ability to pass a block to
'merge!" is good, but when the source hash is empty it does not seem
to trigger use of the code in the block to merge.

-----code----
def merge_add!(h)
self.merge!(h) do |key, existing, new|
new = new.instance_of?(Array) ? new : [new]
existing = existing.instance_of?(Array) ? existing : [existing]
existing + new
end
end
-------------
---in console---
?> {}.merge_add!({1 => 100})
=> {1=>100} <<== DID NOT PUT THE '100' IN AN ARRAY!!

>> {1 => 300}.merge_add!({1 => 100})
=> {1=>[300, 100]}
--------------

regards
Greg

On Mon, Oct 27, 2008 at 5:21 PM, Stefan Rusterholz <apeiros@gmx.net> wrote:
>
> Greg Hauptmann wrote:
> > thanks for highlighting this! Is the quickest way to normalise to Array
> > via
> > the following?
> > hashitem.instance_of?(Array) ? hashitem : [hashitem]
>
> If you're building up the hash yourself then I'd use the following
> idiom:
> collector = Hash.new { |h,k| h[k] = [] }
> collector['key1'] << 'value1'
> collector['key2'] << 'value1'
> collector['key1'] << 'value2'
> p collector
>
> Simplifies adding to and reading from the hash. But be aware that with
> the hash having a different default value than nil, you can't use 'if
> collector[key] then' to test for existence of a key anymore, you have to
> use has_key?.
>
> If you're not building the hash up yourself, then yes, I'd use what you
> wroten in an each loop and override the existing value.
>
> Regards
> Stefan
> --
> Posted via http://www.ruby-....
>

Greg Hauptmann

10/28/2008 12:43:00 AM

thanks all for feedback to date - here's my latest take

------------------------------------
def merge_add!(h)
raise "Parameter passed in not a hash" if !h.instance_of?(Hash)

# normalise input hash to contain arrays
h.each { |key, value| if !value.instance_of?(Array) then h[key] =
[value] end }

self.merge!(h) do |key, existing, new|
existing = existing.instance_of?(Array) ? existing : [existing]
existing + new
end
end
------------------------------------
Macintosh-2:myequity greg$ cat spec/lib/hash_extensions_spec.rb
require File.expand_path(File.dirname(__FILE__) + '/../spec_helper')

describe "merge_add!" do
before(:each) do
@empty_hash = {}
@hash_with_number = {100 => 1}
@hash_with_array = {100 => [1]}
end

it "should raise an error if there is no object passed to the method" do
lambda{ @empty_hash.merge_add!() }.should raise_error(ArgumentError)
end
it "should raise an error if the object passed is not a Hash" do
lambda{@empty_hash.merge_add!(123)}.should
raise_error(RuntimeError, "Parameter passed in not a hash")
end
it "should raise an error if the number of items in the hash in not 1" do
lambda{@empty_hash.merge_add!({101 => 2, 102 => 3})}.should_not raise_error
end
it "should create a new Array & wrap new value as an array" do
@empty_hash.merge_add!( {101 => 2} )
@empty_hash[101].should eql([2])
end
it "should create a new Array & use the array passed as the value" do
@empty_hash.merge_add!( {101 => [2]} )
@empty_hash[101].should eql([2])
end
it "should migrate existing value to an Array, then add the new
value, if existing value is NOT an array" do
@hash_with_number.merge_add!( {100 => 2} )
@hash_with_number[100].should eql([1, 2])
end
it "should migrate add the new value to existing, if existing value
IS an array" do
@hash_with_array.merge_add!( {100 => 2} )
@hash_with_array[100].should eql([1, 2])
end

end

On Mon, Oct 27, 2008 at 5:41 PM, Greg Hauptmann
<greg.hauptmann.ruby@gmail.com> wrote:
> I just noticed the code suggested might have a problem when the
> original hash is empty. That is using the ability to pass a block to
> 'merge!" is good, but when the source hash is empty it does not seem
> to trigger use of the code in the block to merge.
>
> -----code----
> def merge_add!(h)
> self.merge!(h) do |key, existing, new|
> new = new.instance_of?(Array) ? new : [new]
> existing = existing.instance_of?(Array) ? existing : [existing]
> existing + new
> end
> end
> -------------
> ---in console---
> ?> {}.merge_add!({1 => 100})
> => {1=>100} <<== DID NOT PUT THE '100' IN AN ARRAY!!
>
>>> {1 => 300}.merge_add!({1 => 100})
> => {1=>[300, 100]}
> --------------
>
> regards
> Greg
>
> On Mon, Oct 27, 2008 at 5:21 PM, Stefan Rusterholz <apeiros@gmx.net> wrote:
>>
>> Greg Hauptmann wrote:
>> > thanks for highlighting this! Is the quickest way to normalise to Array
>> > via
>> > the following?
>> > hashitem.instance_of?(Array) ? hashitem : [hashitem]
>>
>> If you're building up the hash yourself then I'd use the following
>> idiom:
>> collector = Hash.new { |h,k| h[k] = [] }
>> collector['key1'] << 'value1'
>> collector['key2'] << 'value1'
>> collector['key1'] << 'value2'
>> p collector
>>
>> Simplifies adding to and reading from the hash. But be aware that with
>> the hash having a different default value than nil, you can't use 'if
>> collector[key] then' to test for existence of a key anymore, you have to
>> use has_key?.
>>
>> If you're not building the hash up yourself, then yes, I'd use what you
>> wroten in an each loop and override the existing value.
>>
>> Regards
>> Stefan
>> --
>> Posted via http://www.ruby-....
>>
>
>

Trans

10/28/2008 1:39:00 AM

On Oct 27, 8:42=A0pm, "Greg Hauptmann" <greg.hauptmann.r...@gmail.com>
wrote:
> thanks all for feedback to date - here's my latest take
>
> ------------------------------------
> =A0 def merge_add!(h)
> =A0 =A0 raise "Parameter passed in not a hash" if !h.instance_of?(Hash)

Again, this is considered poor form. The reason is, if it isn't a Hash
it will blow up in the next couple of statements anyway, but more
importantly something other a Hash might emulate one. And there's no
reason not to allow it to work.

> =A0 =A0 # normalise input hash to contain arrays
> =A0 =A0 h.each { |key, value| if !value.instance_of?(Array) then h[key] =
=3D
> [value] end }
>
> =A0 =A0 self.merge!(h) do |key, existing, new|
> =A0 =A0 =A0 existing =3D existing.instance_of?(Array) ? existing : [exist=
ing]
> =A0 =A0 =A0 existing + new
> =A0 =A0 end
> =A0 end

def merge_add!(h)
q =3D {}
(keys | h.keys).each do |k|
q[k] =3D Array(self[k]) + Array(h[k])
end
replace(q)
end

trans.

Greg Hauptmann

10/28/2008 6:09:00 AM

interesting - is it really Ruby approach to let things 'break' during
a method so to speak as opposed to defensive coding and doing some
form of validation at the beginning of the method? I noted when I run
my spec's with your code i get the error

"<NoMethodError: undefined method `keys' for 123:Fixnum>"

which is quite a bit more cryptic than my

"RuntimeError - "Parameter passed in not a hash"

Wouldn't it be harder to debug code if one were getting the former
error message rather than the later?

On Tue, Oct 28, 2008 at 11:38 AM, Trans <transfire@gmail.com> wrote:
>
>
> On Oct 27, 8:42 pm, "Greg Hauptmann" <greg.hauptmann.r...@gmail.com>
> wrote:
>> thanks all for feedback to date - here's my latest take
>>
>> ------------------------------------
>> def merge_add!(h)
>> raise "Parameter passed in not a hash" if !h.instance_of?(Hash)
>
> Again, this is considered poor form. The reason is, if it isn't a Hash
> it will blow up in the next couple of statements anyway, but more
> importantly something other a Hash might emulate one. And there's no
> reason not to allow it to work.
>
>> # normalise input hash to contain arrays
>> h.each { |key, value| if !value.instance_of?(Array) then h[key] =
>> [value] end }
>>
>> self.merge!(h) do |key, existing, new|
>> existing = existing.instance_of?(Array) ? existing : [existing]
>> existing + new
>> end
>> end
>
>
> def merge_add!(h)
> q = {}
> (keys | h.keys).each do |k|
> q[k] = Array(self[k]) + Array(h[k])
> end
> replace(q)
> end
>
>
> trans.
>
>

comp.lang.ruby

Hash.merge_add! extension - how does this code look?

Greg Hauptmann

David A. Black

Trans

Stefan Rusterholz

Trans

Greg Hauptmann

Stefan Rusterholz

Greg Hauptmann

Greg Hauptmann

Trans

Greg Hauptmann

x Login to ForumsZone