Code review comment for lp:~pkunal-parmar/ubuntu-calendar-app/TimeLineView

Revision history for this message
Olivier Tilloy (osomon) wrote :

> > Also, this loop is incorrect, as children are destroyed their length
> > decreases, but the iterator keeps increasing, and will eventually become
> > invalid. You need to do something like this:
> >
> > while (children.length > 0) {
> > children[0].destroy()
> > }
>
> while (children.length > 0) {
> children[0].destroy()
> }
>
> This will create a infinite loop, as we are not removing object from children
> array/list. we are just destroying object so length of array will not change.
> so existing loop works fine, but then array length will keep increasing so to
> reset length i am now resetting array with following statement.
>
> children = [];

Good point, I should have tested my suggestion first.
However, I don’t think you should reset the value of children to [], that’s likely to break automatic memory management, since we don’t have a guarantee of when the children are actually removed from the list. I think the following would work (destroying the children in reverse order, thus ensuring the iterator never becomes invalid):

    function destroyAllChildren() {
        for(var i = children.length; i >= 0; --i) {
            children[i].destroy();
        }
    }

review: Needs Fixing

« Back to merge proposal