Everything

php

Iterator Class/foreach Logic Flaw?

php.internals

Philip Fletcher

22 years ago

At the risk of making a complete fool of myself, I'd like to ask the smarter
brains than me to checkout the current logic in the implementation of
Iterator classes and foreach().

The current logic of foreach( $obj as $key => $val ) { ... } where $obj
implements Iterator, is:

for($obj->rewind(); $obj->hasMore(); $obj->next() ) { ... }

but it should be:

for($obj->rewind(); $obj->isValid(); $obj->next() ) { ... }

As the iterator exits the penultimate interation, next() is called, placing
the 'pointer' on the last 'record'. But before the last record is
processed, hasMore() is called, which *should* return FALSE, because we
have already moved to the last record.

The implementations I have seen so far all return TRUE from hasMore() when
positioned (i.e. current()) on the last record - this is not logical
(Captain). It also prevents proper use of a class implementing an iterator
outside of the foreach construct.

Philip

Marcus Börger

22 years ago

Hello Philip,

have a look at it's dokumentation: ext/spl/spl.php
there you'll find that hasMore() needs to be checked before an access to
key() or current() can be done. Hence it's meaning is somewhat like
isValid().

regards
marcus

Thursday, February 26, 2004, 4:53:50 PM, you wrote:

> At the risk of making a complete fool of myself, I'd like to ask the smarter
> brains than me to checkout the current logic in the implementation of
> Iterator classes and foreach().

> The current logic of foreach( $obj as $key => $val ) { ... } where $obj
> implements Iterator, is:

> for($obj->rewind(); $obj->hasMore(); $obj->next() ) { ... }

> but it should be:

> for($obj->rewind(); $obj->isValid(); $obj->next() ) { ... }

> As the iterator exits the penultimate interation, next() is called, placing
> the 'pointer' on the last 'record'. But before the last record is
> processed, hasMore() is called, which *should* return FALSE, because we
> have already moved to the last record.

> The implementations I have seen so far all return TRUE from hasMore() when
> positioned (i.e. current()) on the last record - this is not logical
> (Captain). It also prevents proper use of a class implementing an iterator
> outside of the foreach construct.

> Philip

--
Best regards,
Marcus                            mailto:helly@php.net

Philip Fletcher

22 years ago

Thanks Marcus

Looking at the documentation, the comments and method name (for hasMore())
do not have the same meaning - the comment is saying 'does the current
element exist?' and the method name says 'are there any more elements?'.

This makes the api very ambiguous - hasMore() and isValid() have very
specific meaning in many languages wrt iterators.

Whilst I appreciate adding an isValid() method to the Iterator interface is
not trivial (and I am loath to suggest a change this close to PHP RC
phase), but it is surely better to have an un-ambiguous api on release -
especially after all the clamouring for an interface implementation :)

Regards

Philip

Marcus Boerger wrote:

Andrei Zmievski

22 years ago

On Thu, 26 Feb 2004, Philip Fletcher wrote:

> Thanks Marcus
>
> Looking at the documentation, the comments and method name (for hasMore())
> do not have the same meaning - the comment is saying 'does the current
> element exist?' and the method name says 'are there any more elements?'.
>
> This makes the api very ambiguous - hasMore() and isValid() have very
> specific meaning in many languages wrt iterators.

I agree. The semantics are fairly different. Let's not confuse them.

-Andrei

"This isn't right. This isn't even wrong."
-- Wolfgang Pauli

Hans Lellelid

22 years ago

Hi Marcus,

Andrei Zmievski wrote:

> On Thu, 26 Feb 2004, Philip Fletcher wrote:
>
>>Looking at the documentation, the comments and method name (for hasMore())
>>do not have the same meaning - the comment is saying 'does the current
>>element exist?' and the method name says 'are there any more elements?'.
>>
>>This makes the api very ambiguous - hasMore() and isValid() have very
>>specific meaning in many languages wrt iterators.
>
>
> I agree. The semantics are fairly different. Let's not confuse them.
>

I also agree strongly with the need for this to get fixed before PHP
goes to release.  IMO the current hasMore() is very misleading and has
the result that people can't intuitively use it outside of the foreach()
context.

Whether the method is called isValid() or something else that makes
sense is less important to me, but providing an intuitive interface that
won't leave code users scratching their heads or submitting bug reports
for what appears to be faulty implementing iterator logic is really
important.

BTW, Iterators are awesome(!), and it's only because they are so awesome
& everyone will use them that I think this is important to address
before release.

Thanks,
Hans

Andi Gutmans

22 years ago

Well we don't have very much time anymore because I want to roll RC1 ASAP
(assuming that we get a couple of bugs fixed which need fixing).
Personally I'm fine with either of these (I don't mind hasMore()) but a
decision has to be reached quickly. From discussing with Marcus I don't
think he feels very strongly about it but like me doesn't think hasMore()
is so bad. People are used to this semantics IMO.

At 09:03 AM 3/1/2004 -0500, Hans Lellelid wrote:

Hans Lellelid

22 years ago

Hi -

Andi Gutmans wrote:

> Well we don't have very much time anymore because I want to roll RC1
> ASAP (assuming that we get a couple of bugs fixed which need fixing).
> Personally I'm fine with either of these (I don't mind hasMore()) but
> a decision has to be reached quickly. From discussing with Marcus I
> don't think he feels very strongly about it but like me doesn't think
> hasMore() is so bad. People are used to this semantics IMO.

Well, I think that it's confusing.  I think that current users are used
to the semantics, but right now it's a pretty small group using PHP5.  I
think that it'll be a lot harder to change something like this after
release -- and I do think the current method naming is semantically
wrong.  Having implemented several Iterators, until now it's  been a bit
of a second-guess (have to go read the example in zend-engine2.php doc)
every time.  I think that this would be a lot clearer if it could be
renamed.

Clearly I don't know what is involved in renaming this method.
Obviously it will break some existing code ... but it's an easy fix
(since generally I assume the only code that would need to change is the
implementing class) & better now then never IHMO.

I know you want to roll RC1 asap -- I want that too (!) -- but I think
it would be great if this change could be incorporated.  Just my .02.

Thanks,
Hans

Andi Gutmans

22 years ago

As I said I'm indifferent. It would be nice to have Marcus in this
discussion but he's on the PHP Cruise and I'm not sure how often he's
checking is email.

At 09:21 AM 3/1/2004 -0500, Hans Lellelid wrote:

Marcus Börger

22 years ago

Hello Andi, hello all

Monday, March 1, 2004, 3:46:56 PM, you wrote:

> As I said I'm indifferent. It would be nice to have Marcus in this
> discussion but he's on the PHP Cruise and I'm not sure how often he's
> checking is email.

never. Today i am back and checked 1K8 mails :-(
I'll change it on monday or tuesday...and people i need you to verify i
don't overlook an example or something else....

> At 09:21 AM 3/1/2004 -0500, Hans Lellelid wrote:
>>Hi -
>>
>>Andi Gutmans wrote:
>>
>>>Well we don't have very much time anymore because I want to roll RC1 ASAP
>>>(assuming that we get a couple of bugs fixed which need fixing).
>>>Personally I'm fine with either of these (I don't mind hasMore()) but a
>>>decision has to be reached quickly. From discussing with Marcus I don't
>>>think he feels very strongly about it but like me doesn't think hasMore()
>>>is so bad. People are used to this semantics IMO.
>>
>>Well, I think that it's confusing.  I think that current users are used to
>>the semantics, but right now it's a pretty small group using PHP5.  I
>>think that it'll be a lot harder to change something like this after
>>release -- and I do think the current method naming is semantically
>>wrong.  Having implemented several Iterators, until now it's  been a bit
>>of a second-guess (have to go read the example in zend-engine2.php doc)
>>every time.  I think that this would be a lot clearer if it could be renamed.
>>
>>Clearly I don't know what is involved in renaming this method.
>>Obviously it will break some existing code ... but it's an easy fix (since
>>generally I assume the only code that would need to change is the
>>implementing class) & better now then never IHMO.
>>
>>I know you want to roll RC1 asap -- I want that too (!) -- but I think it
>>would be great if this change could be incorporated.  Just my .02.
>>
>>Thanks,
>>Hans
>>

--
Best regards,
Marcus                            mailto:helly@php.net

Philip Fletcher

22 years ago

Sorry for follow-up to myself, but it would be somewhat easier to just
rename hasMore() to isValid() and then implement hasMore() after PHP5 goes
gold.

Philip Fletcher wrote:

Marcus Börger

22 years ago

Hello Philip,

just to make it clear. hasMore() would be the exact same then isValid()
only the name would change. The semantic is already what you refer to as
isValid.

Thursday, February 26, 2004, 9:43:40 PM, you wrote:

> Sorry for follow-up to myself, but it would be somewhat easier to just
> rename hasMore() to isValid() and then implement hasMore() after PHP5 goes
> gold.

> Philip Fletcher wrote:

>> Whilst I appreciate adding an isValid() method to the Iterator interface
>> is not trivial (and I am loath to suggest a change this close to PHP RC
>> phase) ...

--
Best regards,
Marcus                            mailto:helly@php.net

Philip Fletcher

22 years ago

Hi Marcus

Yes, the current implementation of hasMore() has an identical semantic to
isValid().

The generally understood semantic of isValid() and hasMore() are quite
different - hence the need for the rename.

Regards

Philip

Marcus Boerger wrote:

Marcus Börger

22 years ago

Hello Philip,

Thursday, February 26, 2004, 10:05:09 PM, you wrote:

> Hi Marcus

> Yes, the current implementation of hasMore() has an identical semantic to
> isValid().

> The generally understood semantic of isValid() and hasMore() are quite
> different - hence the need for the rename.

As a side note: There is no generally understood hasMore(). Though there is
isDone() and hasMore() is sometimes used/implemented as !isDone(). The main
problem is that the original Iterator description everyone refers to doesn't
have isValid() but instead only has isDone() which doesn't fit into for(;;).
Reference: Gamma et Al, Design Patterns, 1994, Addison Wesley, p 257ff

> Regards

> Philip

> Marcus Boerger wrote:

>> Hello Philip,
>>
>> just to make it clear. hasMore() would be the exact same then isValid()
>> only the name would change. The semantic is already what you refer to as
>> isValid.
>>

--
Best regards,
Marcus                            mailto:helly@php.net

Philip Fletcher

22 years ago

Marcus Boerger wrote:

> As a side note: There is no generally understood hasMore().

I strongly disagree - the Java Iterator has a hasNext() method which is the
exact sementic of hasMore().

> Though there
> is isDone() and hasMore() is sometimes used/implemented as !isDone(). The
> main problem is that the original Iterator description everyone refers to
> doesn't have isValid() but instead only has isDone() which doesn't fit
> into for(;;). Reference: Gamma et Al, Design Patterns, 1994, Addison
> Wesley, p 257ff

Hehe - I happen to have the book open on that page:)

Fortunately, PHP does not implement GOF patterns wholesale, therefore
procedural PHP constructs such as foreach need to deviate from the
isDone(). It logically requires an isValid() expression - not a hasMore()
or hasNext().

The isValid() iterator method is implemented in the Eclipse project - it
gets a mention in dispatches on phppatterns: http://www.phppatterns.com
index.php/article/articleview/50/1/1/  (just over half way down).

Regards

Philip