> On 2012-02-13 23:12:07, Henry Saputra wrote: > > trunk/features/src/main/javascript/features/actions/actions_container.js, > > line 291 > > <https://reviews.apache.org/r/3873/diff/2/?file=74485#file74485line291> > > > > Nothing in particular, just making it easier to read for me while > > fixing the array errors. > > Dan Dumont wrote: > I ask because the code you introduced is often larger (even when closure > complied), with the exception of perhaps the var statements. > > When traversing an array with an index, doing > for(var i=0,b;b=a[i];i++){} > > is smaller than: > for(var i=0;i<a.length;i++){var b=a[i];} > > by about 13 characters > > Yeah sure, just 13 characters :) but that's for each for loop. This > is something that closure doesn't optimize for you because of the trickiness > of the truthy assignment. You as a programmer can know if the value assigned > might possibly be falsy, and should not use this technique in this case (so > closure can't really know that, unless there is an annotation I'm unaware of) > like if a value in an array might legitimately be null, 0, undefined or ''. > > Henry Saputra wrote: > The main goal of this patch is to fix error/ wrong associative array > accesses that caused browsers to throw exception. > You can definitely submit patch improvement to make it smaller yo load. > Probably also add JSUnit test to make sure the intended behavior are done > through the code, sometimes short code could fall with corner cases if not > carefully coded. > > To make sure the condition portion of the loop check for proper boolean > value, I think we could write it like: > > for(var i=0, b; !!(b=a[i]); i++) { > .... > }
I agree the goal of the patch was worthwhile and necessary. I also agree as I mentioned above that coders must be careful when using the technique I mentioned. Though, I don't think coercing the assignment to a boolean buys anything in this situation, as the for loop only expects a truthy value and not just true/false. I'm not going to submit a patch to revert this alone. I only mentioned it because the bulk of the change was refactoring these for loops which did not have anything to do with the stated main goal of the patch. - Dan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3873/#review5062 ----------------------------------------------------------- On 2012-02-12 01:46:58, Henry Saputra wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3873/ > ----------------------------------------------------------- > > (Updated 2012-02-12 01:46:58) > > > Review request for shindig. > > > Summary > ------- > > Some of the iterations in the actions_container.js file are using "in" > operator without calling hasOwnProperty to make sure its not going up to > prototype chain (http://yuiblog.com/blog/2006/09/26/for-in-intrigue/). It > causes trouble when executing in some browsers like FF5. > > Small cleanup for actual array to just simply used for-loop with index. > > > Diffs > ----- > > trunk/features/src/main/javascript/features/actions/actions_container.js > 1243090 > > Diff: https://reviews.apache.org/r/3873/diff > > > Testing > ------- > > Pass JS unit tests and manual test with common container and action container. > > > Thanks, > > Henry > >
