Nested iterators
Adrian Mowat
I just ran reek for the first time. The code base is in its infancy
and I didn't get too many reports (phew!). However, I got quite a lot
about nested iterators which I didn't even realise were a potential
issue.
Does anyone have any links about why these are bad and what to do
about it?
Many Thanks
Adrian
Kevin Rutherford
Not sure much has been written about it. I added the check because
it's often a cause of LongMethod, and because it often indicates
there's a missing abstraction somewhere.
Try extracting all/most of the outermost block to a new method; this
will silence the Nested Iterators complaint and may bring other smells
to light. (Alternatively, disable the smell in a config file if you're
happy with the code.)
As Reek grows to encompass more smells, my vision is that fixing
little things like NestedIterators will push the code into shapes that
can easily reveal other smells. So using Reek will become an iterative
smell-chase. Maybe. In some distant future, when my kids are grown up.
Cheers,
Kevin
--
m: +44 (0) 797 356 3521
w: http://www.kevinrutherford.co.uk
e: ke...@rutherford-software.com
t: @kevinrutherford
l: http://www.linkedin.com/in/kevinrutherford
Adrian Mowat
Thanks for getting back to me.
This is interesting. When I looked at the code again I realised that
the nested iterator was actually a whacking great dependency that
needs to be inverted. The code is converting the results of an active
record find into a core domain object and I found the converter code
needs to live outside the domain object rather than being a method on
it (which, of course, is obvious once you see it).
As an abstract thought though, your reply suggested I factor out the
outer block. I'm not sure I can see how this would be possible.
For example, in my case I have a situation sort of like this...
irb(main):001:0> def nested1(array)
irb(main):002:1> array.each do |inner|
irb(main):003:2* inner.each {|value| puts value}
irb(main):004:2> end
irb(main):005:1> end
The client code wants to print every value in the inner arrays to the
screen
irb(main):006:0> nested1([[1,2,3],[4,5,6]])
1
2
3
4
5
6
I can see how to factor out the inner block....
irb(main):008:0> def nested2_1(array)
irb(main):009:1> array.each do |inner|
irb(main):010:2* print_values(inner)
irb(main):011:2> end
irb(main):012:1> end
=> nil
irb(main):013:0> def print_values(values)
irb(main):014:1> values.each {|v| puts v}
irb(main):015:1> end
=> nil
irb(main):017:0> nested2_1([[1, 2, 3], [4, 5, 6]])
1
2
3
4
5
6
... but I can't see how to factor the outer block without the nested
iterator continuing to happen somewhere.
Am I missing something?
Many Thanks
Adrian
On Jul 29, 10:29 am, Kevin Rutherford <ke...@rutherford-software.com>
wrote:
Kevin Rutherford
> This is interesting. When I looked at the code again I realised that
> the nested iterator was actually a whacking great dependency that
> needs to be inverted. The code is converting the results of an active
> record find into a core domain object and I found the converter code
> needs to live outside the domain object rather than being a method on
> it (which, of course, is obvious once you see it).
That's the thing with Reek (and with code smells in general). They
point at areas of suspect code, but the smelly symptoms are frequently
just a red flag -- the /real/ problem is usually buried underneath
somewhere.
> I can see how to factor out the inner block....
>
> irb(main):008:0> def nested2_1(array)
> irb(main):009:1> array.each do |inner|
> irb(main):010:2* print_values(inner)
> irb(main):011:2> end
> irb(main):012:1> end
> => nil
> irb(main):013:0> def print_values(values)
> irb(main):014:1> values.each {|v| puts v}
> irb(main):015:1> end
> => nil
> irb(main):017:0> nested2_1([[1, 2, 3], [4, 5, 6]])
> 1
> 2
> 3
> 4
> 5
> 6
>
> ... but I can't see how to factor the outer block without the nested
> iterator continuing to happen somewhere.
>
> Am I missing something?
You've done exactly what I meant, although I didn't express it very clearly :)