On 26 September 2011 20:22, Brian Candler <b.candler / pobox.com> wrote:

> > def collect_urls(base_url, page_number)
> >   valid_urls = []
> >   1000.times do   # prevent infinite looping
> >     text = get_text(base_url, page_number)
> >     if text =~ /\A\s*Previous/
> >       valid_urls << "END!"
> >       break
> >     else
> >       valid_urls << @target_url
> >       page_number += 1
> >     end
> >   end
> >   return valid_urls
> > end
>
> Incidentally, you seem to be relying on the instance variable
> @target_url being set after get_text being called. It would be cleaner
> to return the url and the text as two values.
>
>    return target_url, text
>
>    ...
>
>    target_url, text = get_text(base_url, page_number)



Brian,

Many thanks for your helpful suggestions.  This is working for me now.
 Originally, I was trying to avoid 'hard-coding' the loop such as
'1000.times do' but I see now that this is probably the most elegant
solution.  Two really good learning points there for me two on the  use of
instance variables and efficiently returning and referring to expressions.
 Thanks again!

Matt