Re: Help me improve Hash#rekey

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]<javascript:>> 
> 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.
 

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


> > 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!!! Put the default 
:to_sym back in and we could have a deal :-)

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

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