From: Intransition Date: 2012-12-11T04:44:32+09:00 Subject: Re: Help me improve Hash#rekey ------=_Part_248_31563838.1355168654480 Content-Type: multipart/alternative; boundary="----=_Part_249_31416065.1355168654480" ------=_Part_249_31416065.1355168654480 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On Monday, December 10, 2012 7:22:17 AM UTC-5, Robert Klemme wrote: > > On Sun, Dec 9, 2012 at 7:19 PM, Intransition > > 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! ------=_Part_249_31416065.1355168654480 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 7bit

On Monday, December 10, 2012 7:22:17 AM UTC-5, Robert Klemme wrote:
On Sun, Dec 9, 2012 at 7:19 PM, Intransition <tran...@gmail.com> 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!

------=_Part_249_31416065.1355168654480-- ------=_Part_248_31563838.1355168654480--