Code review comment for lp:~openerp-dev/openobject-addons/6.0-bug_725908-ach

Revision history for this message
Jay Vora (Serpent Consulting Services) (jayvora) wrote :

Anup,

Thanks to you and Kirti for the fix. It was really a good one.

Kindly go through these important points to improve here:

1. Variable naming convention.(Do not opt for i,j,k,v).
2. Wrong indentation on your proposal Line 21.
3. Just a note : Whenever we use child_of operator, go for the right operant with [] of ids. This doesn't create any problem as this has been covered well recently in expression.py, but its a good practice.(Refer to line 31)
4. Does the order='id' give you a different result when order='parent_left'? (Refer to Line 31)
5. Line 61 should be location_dest_id = location. Think over it.
6. Use a shorthand operator. (Refer to line 63)
7. Improve the notification, it could be notification or warning(Line 70).
8. You don't need line 73.
9. Why do we need len(line_ids)? (Line 90).
10. Regarding last 2 for loops, I am not 100% sure whether its an optimized solution or not.
11. Regarding Line 75, are you sure we always have one inventory to check? I doubt because, we can use this wizard for multiple inventories. And its always wise to check before writing list[0].

Correct me If I missed anything.

I am investigating more in order to get the best suited fix.

Thanks.

review: Needs Fixing

« Back to merge proposal