Private accessors and duplication

5 views
Skip to first unread message

Ashley Moran

unread,
Sep 26, 2009, 4:21:26 PM9/26/09
to ruby...@googlegroups.com
Hi all (Kevin?)

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

unread,
Sep 28, 2009, 6:30:33 AM9/28/09
to ruby...@googlegroups.com
Hi Ash,

> 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

unread,
Sep 28, 2009, 6:45:30 AM9/28/09
to ruby...@googlegroups.com
On Mon, Sep 28, 2009 at 11:30 AM, 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

unread,
Sep 28, 2009, 1:38:34 PM9/28/09
to ruby...@googlegroups.com

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

unread,
Sep 28, 2009, 2:19:53 PM9/28/09
to ruby...@googlegroups.com

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

unread,
Sep 28, 2009, 3:24:45 PM9/28/09
to ruby...@googlegroups.com
> I give up, you must know something I don't!

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

unread,
Sep 28, 2009, 3:53:21 PM9/28/09
to ruby...@googlegroups.com

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 :)

Reply all
Reply to author
Forward
0 new messages