Private accessors and duplication
Ashley Moran
Is it desirable behaviour, or a limitation in Reek, than means
duplication is only reported on #palindrome_name? in the code snippet
below?
class Duplication
attr_reader :name
private :name
def initialize
@name = "Fred"
@secret = "Password"
end
def palindrome_name?
name == name.reverse
end
def palindrome_password?
@secret == @secret.reverse
end
end
What is your opinion on private accessors, and code like the above?
When I find myself in this situation currently, I silence reek like
this:
class Duplication
def palindrome_name?
name == reversed_name
end
private
def reversed_name
name.reverse
end
end
In general, I like this - the extra methods are generally more
descriptive than the extra class length is distracting. Also,
anything that puts class length up with locally relevant concepts
leaves less room for loosely-related concepts (that should be held
elsewhere) to creep in. *But* the instance variable approach seems
perfectly readable to me.
I ask mainly because in some objects (for example, those that include
DataMapper::Resource), you don't have the option of accessing local
state by instance variables, which means a whole class of objects is
forced to extract methods as above.
Any thoughts?
Cheers
Ash
--
http://www.patchspace.co.uk/
http://www.linkedin.com/in/ashleymoran
http://aviewfromafar.net/
Kevin Rutherford
> Is it desirable behaviour, or a limitation in Reek, than means
> duplication is only reported on #palindrome_name? in the code snippet
> below?
>
> class Duplication
> attr_reader :name
> private :name
>
> def initialize
> @name = "Fred"
> @secret = "Password"
> end
>
> def palindrome_name?
> name == name.reverse
> end
>
> def palindrome_password?
> @secret == @secret.reverse
> end
> end
>
> What is your opinion on private accessors, and code like the above?
Reek doesn't (currently) know that #name is private. But I suppose it
shouldn't report calls to local methods, because presumably the class
knows what it is doing.
> When I find myself in this situation currently, I silence reek like
> this:
>
> class Duplication
> def palindrome_name?
> name == reversed_name
> end
>
> private
>
> def reversed_name
> name.reverse
> end
> end
Hmmm. There's some other Duplication in this class though; so my fix is this:
class String
def palindrome?
self == self.reverse
end
end
class Duplication
# ...
def palindrome_name?
name.palindrome?
end
def palindrome_password?
@secret.palindrome?
end
end
> I ask mainly because in some objects (for example, those that include
> DataMapper::Resource), you don't have the option of accessing local
> state by instance variables, which means a whole class of objects is
> forced to extract methods as above.
Really -- why not?
Cheers,
Kevin
--
http://www.kevinrutherford.co.uk -- agile, TDD, XP, lean, TOC
Kevin Rutherford
<ke...@rutherford-software.com> wrote:
>
> class String
> def palindrome?
> self == self.reverse
> end
> end
Today's kata: test-drive a palindrome? method for String, ensuring
that it is much faster than the above.
Ashley Moran
On 28 Sep 2009, at 11:30, Kevin Rutherford wrote:
> Reek doesn't (currently) know that #name is private. But I suppose it
> shouldn't report calls to local methods, because presumably the class
> knows what it is doing.
Should I file a ticket?
>> When I find myself in this situation currently, I silence reek like
>> this:
>>
>> class Duplication
>> def palindrome_name?
>> name == reversed_name
>> end
>>
>> private
>>
>> def reversed_name
>> name.reverse
>> end
>> end
>
> Hmmm. There's some other Duplication in this class though; so my fix
> is this:
Actually, this is a red herring. We don't have any code like that in
our app, I just wanted to demonstrate the point with two versions of
the same method.
> class String
> def palindrome?
> self == self.reverse
> end
> end
Extending core Ruby classes has always seemed dangerous to me. I'm
surprised you don't consider it a code smell, or is it the antidote to
primitive obsession?
Cheers
Ashley
Ashley Moran
On 28 Sep 2009, at 11:45, Kevin Rutherford wrote:
> Today's kata: test-drive a palindrome? method for String, ensuring
> that it is much faster than the above.
I give up, you must know something I don't!
###
require "rubygems"
require "spec"
require "spec/autorun"
class String
def palindrome?
self == self.reverse
end
def fast_palindrome?
!(0..length/2).any? { |i| self[-1 * i -1] != self[i] }
end
end
def time
start_time = Time.now
yield
Time.now - start_time
end
describe String do
before(:each) do
GC.disable
end
after(:each) do
GC.enable
end
describe "#palindrome?" do
it "is true when the string is a palindrome" do
"aa".should be_palindrome
"aibohphobia".should be_palindrome
"racecar".should be_palindrome
end
it "is false when the string is not a palindrome" do
"arachnophobia".should_not be_palindrome
"sidecar".should_not be_palindrome
end
end
describe "#fast_palindrome?" do
it "is true when the string is a palindrome" do
"aa".should be_palindrome
"aibohphobia".should be_fast_palindrome
"racecar".should be_fast_palindrome
end
it "is false when the string is not a palindrome" do
"arachnophobia".should_not be_fast_palindrome
"sidecar".should_not be_fast_palindrome
end
it "is faster than #palindrome? for palindromes" do
10.times do
time_for_palindrome = time do
250_000.times { "aibohphobia".palindrome? }
end
time_for_fast_palindrome = time do
250_000.times { "aibohphobia".fast_palindrome? }
end
time_for_fast_palindrome.should be < (0.25 *
time_for_palindrome)
end
end
it "is faster than #palindrome? for non-palindromes" do
10.times do
time_for_palindrome = time do
250_000.times { "1234567890X".palindrome? }
end
time_for_fast_palindrome = time do
250_000.times { "1234567890X".fast_palindrome? }
end
time_for_fast_palindrome.should be < (0.25 *
time_for_palindrome)
end
end
end
end
###
Am I missing something obvious? I will be using my post-work pint as
an excuse though =)
Ashley
Kevin Rutherford
I doubt it.
Does your solution work for empty strings? What about strings of
10,000 chars? Or Kanji strings?
Did you test long strings in which the non-palindrome char is close to
the centre?
Are there cases where the #reverse approach is faster? If so, how
could you speed up those cases?
Ashley Moran
On 28 Sep 2009, at 20:24, Kevin Rutherford wrote:
> I doubt it.
> Does your solution work for empty strings?
I don't even know if an empty string can be a palindrome, but you're
right, I did miss this by thinking only about the performance specs.
> What about strings of
> 10,000 chars? Or Kanji strings?
Unicode was one thing I knew would break it, but on the basis my
current version was too slow, seemed like a pointless addition making
it slower.
> Did you test long strings in which the non-palindrome char is close to
> the centre?
Hmmm, seeing how both examples I thought of (success and immediate
failure) were much slower, it'd have to be a huge string where the
#reverse takes longer than the byte lookups. So the palindrome
character would have to be near the edges for it to be faster.
I thought about generating long strings, then I realised most English
words are < 15 letters long.
> Are there cases where the #reverse approach is faster?
All of them so far :)
> If so, how
> could you speed up those cases?
That's the question...
I don't actually believe it can be done faster in pure Ruby except for
the one edge case of a long string that fails near the start of the
search. Well, unless there's a much cleverer way than anything I've
though of.
Consider today's kata failed on the basis I unfortunately have to go
home now. But feel free to post more :)