Re: Help me improve Hash#rekey

On Mon, Dec 10, 2012 at 8:44 PM, Intransition <[e-mail]> wrote:
>
>
> On Monday, December 10, 2012 7:22:17 AM UTC-5, Robert Klemme wrote:
>>
>> On Sun, Dec 9, 2012 at 7:19 PM, Intransition <[e-mail]> wrote:
>> > The note `TODO: Improve Hash#rekey code!!!` has been in my docs for too
>> > long. I could use other's insights and thought others might enjoy the
>> > challenge. So here's the code:
>>
>> I think the problem is in the API.  That's what makes it too complex.
>> There are too many cases of specific handling and options (see below):
>>
>> 1. Unnecessary option: if the key is supposed to stay intact the block
>> should just return the original key.
>
> I agree. Just wanted someone else to confirm. Out it goes!

:-)

>> >     #
>> >     #   foo = { :name=>'Gavin', :wife=>:Lisa }
>> >     #   foo.rekey{ |k| k.to_s }  #=>  { "name"=>"Gavin", "wife"=>:Lisa }
>> >     #   foo                      #=>  { :name =>"Gavin", :wife=>:Lisa }
>> >     #
>> >     # If no key map or block is given, then all keys are converted
>> >     # to Symbols.
>>
>> 2. Why that default?  In my mind this is too much implicit logic.
>> Also this can be easily achieved with
>>
>> hash.rekey(&:to_sym)
>
>
> I can understand that. I used the default b/c the vast majority of the time
> I was using it to convert to symbols. Rather then have yet another method
> like ActiveSupport's `symbolize_keys` it made more sense to me to just give
> #rekey this as the default.
>
> I can't really take that back now. It's been too long part of the API.

Well, then you mustn't change *anything* which affects observable behavior.

>> >     #
>> >     # Note that if both a +key_map+ and a block are given, the +key_map+
>> > is
>> >     # applied first then the block.
>>
>> 3. I would change that to EITHER block OR map argument, but not both.
>
>
> Yea, I thought about that when I added support for the mapping. That was
> actually something that came later that the original block form. At first I
> had thought about making an entirely different method, but then thought that
> was a waste and that it made more sense as part of #rekey. So when I first
> added it I did an "either or", just as you suggest. But then I thought
> "why?" it certainly can handle both even if people will almost never use
> both.
>
> What do you think? Does it really matters enough to change it now? Like I
> said, I doubt anyone has used both, so this is something that could be
> change if it really is worth it.

I would get rid of that behavior.  After all, if someone wants do do
Hash lookups as part of conversion they can still do that in the block
without much effort.

I also noticed a slight asymmetry between block and Hash: a block will
return something for every key passed in.  But since a Hash is fixed
at the time of method call (modulo default_proc of course) there is
not really a way to handle absent keys ad hoc.

>> > Not the use of `facets/na`. That is defined as:
>> >
>> >     class << NA = ArgumentError.new
>> >       def inspect ; 'N/A' ; end
>> >       def method_missing(*); self; end
>> >     end
>> >
>> > But it is really nothing more than a dummy object used to mean Not
>> > Applicable. So in the case of #rekey, if the block returns NA then the
>> > key
>> > goes unchanged. Thinking about it again now, it's probably unnecessary,
>> > but
>> > I had wanted a way to say "leave it alone" while also making sure that
>> > `nil`
>> > could still be used as a key (even if that's rare). Feel free to remove
>> > the
>> > NA business, but if you do please explain why you think its not needed.
>>
>> If one needs a special key one can use a Symbol for that as
>> efficiently as nil.  nil is the value return if something is absent
>> and I believe it does not make for a good key in a Hash.
>
>
> Wouldn't use symbols b/c then you have a special exception. NA was made just
> for such cases. But you probably right that `nil` doesn't make a good hash
> key no matter what. Nonetheless, it doesn't really matter b/c as you said
> above, they can just return the original key.
>
>>
>> > Best solution will get their name put in front of CREDITs for the next
>> > release of Facets.
>>
>> :-)
>>
>> class Hash
>>   def rekey(mapping = nil, &convert)
>>     c = convert || mapping
>>     dup.tap do |h| # preserve type and defaults
>>       h.clear
>>       each_pair {|k, v| h[c[k] || k] = v}
>>     end
>>   end
>> end
>>
>
> Sweet. Much smaller than mine, that's for damn sure!!!

Yeah, but that's easy when one removes the complexity in behavior.

> Put the default
> :to_sym back in and we could have a deal :-)

Well, that's easy, isn't it?

def rekey(mapping = nil, &convert)
  c = convert || mapping || :to_sym.to_proc

> I'm need to test and benchmark it first though.

Have fun!

> Oh, and nice use of polymorphism using #[] for both proc and hash retrieval!

Thank you! :-)

Kind regards

robert



--
remember.guy do |as, often| as.you_can - without end
http://blog.rubybestpractices.com/