Review Board 1.7.22


Array iteration cleanup in the actions_container.js file

Review Request #3873 - Created Feb. 12, 2012 and submitted

Henry Saputra
Reviewers
shindig
shindig
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.
Pass JS unit tests and manual test with common container and action container.
Ship it!
Posted (Feb. 12, 2012, 10:29 a.m.)
lgtm
  1. Thanks for the review Paul. Committed as revision 1243281 to trunk
Posted (Feb. 13, 2012, 11:01 p.m.)

   

  
Was there a reason you needed to refactor this code?
And this?
And this?
Posted (Feb. 13, 2012, 11:12 p.m.)

   

  
Nothing in particular, just making it easier to read for me while fixing the array errors.
  1. 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 ''.
  2. 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++) {
       ....
    } 
  3. 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.
Posted (Feb. 15, 2012, 2:50 p.m.)
Henry, sorry for the abrupt review.  No hard feelings here, and none were intended :)