Internally-changing object state as code smell?

[Ashley Moran]

Hi

A client recently refactored a bit of code we initially hacked
together as several nested iterators (it generates an HTML table).
His refactoring uses object state to store intermediate steps. A
highly reduced example in this style could be:

class Generator
def initialize(length)
@length = length
@string = ""
end

def generate
(1..@length).each { |number| add_number(number) }
@string
end

def add_number(number)
@string << "."
end
end

Generator.new(5).generate # => "....."
Generator.new(12).generate # => "............"

Now, this is not something I'd write myself, but I can't articulate
why in terms of code smells. (I primarily don't write like this
because of the coupling between methods after one external message is
received... whether or not that should actually be a concern, I don't
know.)

Is there anything above that could be described as a code smell,
currently categorised or otherwise? (The example may be too
simplified to convey what I dislike about a larger example.)

Reek reports no warnings.

Thanks
Ashley

--
http://www.patchspace.co.uk/
http://www.linkedin.com/in/ashleymoran
http://aviewfromafar.net/

[Kevin Rutherford]

Hello again,

>   class Generator
>     def initialize(length)
>       @length = length
>       @string = ""
>     end
>
>     def generate
>       (1..@length).each { |number| add_number(number) }
>       @string
>     end
>
>     def add_number(number)
>       @string << "."
>     end
>   end
>
>   Generator.new(5).generate # => "....."
>   Generator.new(12).generate # => "............"
>
> Now, this is not something I'd write myself,

Ok, so the first step is to write your own solution and get it as
"clean" as you can. (Kata are good for you :)

> but I can't articulate
> why in terms of code smells.  (I primarily don't write like this
> because of the coupling between methods after one external message is
> received... whether or not that should actually be a concern, I don't
> know.)

That's called TemporaryField (it's in my book :).

> Reek reports no warnings.

http://github.com/kevinrutherford/reek/issues#issue/22

Cheers,
Kevin
--
http://www.kevinrutherford.co.uk -- agile, TDD, XP, lean, TOC

[Ashley Moran]

On 28 Sep 2009, at 11:11, Kevin Rutherford wrote:

> Ok, so the first step is to write your own solution and get it as
> "clean" as you can. (Kata are good for you :)

I've already done that, I posted after refactoring it :)

> That's called TemporaryField (it's in my book :).
>
>> Reek reports no warnings.
>
> http://github.com/kevinrutherford/reek/issues#issue/22

Hmmm, but that ticket says "A class has a field that is not set in the
constructor, but is used in some method that doesn't also set it",
when in the above snippet it is set in the constructor. So is the
above a TemporaryField, or is it still a TemporaryField if the value
in the constructor is not from a parameter?

And what would happen if the above object was actually a Queue, and
was initialising array in the constructor? Then Queue#enqueue would
send :<< to the array, but the code would be syntactically identical...

[Kevin Rutherford]

Hi Ash,

On Mon, Sep 28, 2009 at 6:27 PM, Ashley Moran
<ashley...@patchspace.co.uk> wrote:
>
>> http://github.com/kevinrutherford/reek/issues#issue/22
>
> Hmmm, but that ticket says "A class has a field that is not set in the
> constructor, but is used in some method that doesn't also set it",
> when in the above snippet it is set in the constructor.  So is the
> above a TemporaryField, or is it still a TemporaryField if the value
> in the constructor is not from a parameter?

I wrote that because it's a relatively easy edge case that I can peel
off first. The general case requires either a lot of static analysis
or a test run.

[Ashley Moran]

On 28 Sep 2009, at 20:15, Kevin Rutherford wrote:

> I wrote that because it's a relatively easy edge case that I can peel
> off first. The general case requires either a lot of static analysis
> or a test run.

I understand now.

Ta
Ash