Closed misfo closed 11 years ago
What about calling it deep_freeze!
? That'd also sound more dangerous than fast_
.
@lgierth that could work. The !
suffix is a bit overloaded but I guess that's fine if it just ends up meaning "check the documentation for why this method has a bang on it"
I always liked the explanation for that convention from: http://dablog.rubypal.com/2007/8/15/bang-methods-or-danger-will-rubyist
This is a bit different from how it's used in popular ruby gems, but I tend to follow this in all my code. The bang method is typically the more dangerous one, it's the "I know what I'm doing" method.
@dkubb so you're saying you like the rename fast_deep_freeze
-> deep_freeze!
, right? I think I do, too.
Sent from my phone
On Sep 20, 2013, at 1:58 AM, Dan Kubb notifications@github.com wrote:
I always liked the explanation for that convention from: http://dablog.rubypal.com/2007/8/15/bang-methods-or-danger-will-rubyist
This is a bit different from how it's used in popular ruby gems, but I tend to follow this in all my code. The bang method is typically the more dangerous one, it's the "I know what I'm doing" method.
— Reply to this email directly or view it on GitHub.
+1 for deep_freeze!
@misfo yeah, I like the rename of #fast_deep_freeze
to #deep_freeze!
One thing to note when comparing results of benchmarks/speed.rb
is that the hash creation before the benchmarks somehow affects the speed of the benchmarks themselves. So to compare this PR against master you should add the hash2 = nested(3, 5, 500)
line before running on master.
With the above in mind, running benchmarks/speed.rb
indicates that:
deep_freeze
is about 60% slower than on master, but fixes #10 deep_freeze!
is about 4x faster than deep_freeze
Is the documentation in IceNine.deep_freeze!
clear enough about when to use the method?
@misfo imo it's fine, yes!
I updated the specs with your suggestions from today in 4468c8d
Alright, this should be good to go now. Thanks for the suggestions!
@misfo This is merged now, thanks so much!
It looks like there's a few failures on travis when I merged this into master. I can't look at it now, but I'll try looking at it after work. In the meantime, if anyone is interested PDI.
@dkubb @misfo fwiw, on my machine, there are a few surviving mutants ...
@snusnu As was the case for #8, the mutations Mutant is showing actually do make the tests fail. So I'm not sure what they're indicating...
deep_freeze
is 50% slower than before the commit, but will deep freeze shallowly frozen objects (fixing #10).fast_deep_freeze
is 2.5x faster than previous deep_freeze, with the exact same behavior.This PR incorporates the faster algorithm from #9, but with the method name
IceNine.fast_deep_freeze
.If someone has a better idea for the method names, let me know. My reasoning was that
deep_freeze
will never have any surprising edge cases (like #10) andfast_deep_freeze
is an acceptable name for the other method because it screams "I know what I'm doing here!" The downside of this name is that literals are good candidates forfast_deep_freeze
, and I don't look forward to seeing:The
fast_
just adds that much more noise to the code...I still need to update the tests and documentation a bit, I just thought I'd see what people think...