openerp-community-reviewer team mailing list archive

[Pedro Manuel Baeza <pedro.baeza@xxxxxxxxx>]

Review: Needs Fixing code review

Hi, Jordi,

Thanks for starting to contribute to community modules! Let me make some suggestions on the proposed code. Some of them are requirements of the coding conventions of the community. Others are little improvements that you can apply to your day to day code. I know that current modules don't comply with these recommendations, but we have inherited a lot of code that we are adapting slowly, so we have to be more open-mind with it, but for new contributions, we encourage to make them good from the start.

- Remove line 17: print "on_change_user", because it only has a debug purpose.

- You don't need these lines if you are not going to use in the method context variable (you don't need them either if you pass context to another methods as argument):

if context is None:
    context = {}

- Please respect PEP8 convention of 79 cols maximum width. It makes code more readable and favor things like diff comparison. At first, it seems ugly, but when you get used to, it's better, I assure you. Here you have full PEP8 specification:

http://legacy.python.org/dev/peps/pep-0008/

- When you change only one dictionary value, it's faster and more readable to assign it directly:

res.update({'section_id': section_id})

replaced by

res['section_id'] = section_id

- From 6.1, when you need the object reference for a model, you can call self.pool['object.name'] instead of self.pool.get('object.name'). It's shorter and if you misspell one model name, you get the error traceback in the correct line, not a few lines (or a lot) after, when you use the variable.

- It's also convenient to reserve variables for object references to get shorter lines (to respect PEP8) when calling search, browse and so on, and you only make the reference getting once:

section_ids = self.pool.get('crm.case.section').search(...

become

section_obj = self.pool['crm.case.section']
section_ids = section_obj.search(...

- It's faster to include in the 'for' sentence directly the browse method, than call each time with only one ID:

employee_ids = self.pool.get('hr.employee').search(cr, uid, [('user_id','=',user_id)], context=context)
for employee_id in employee_ids:
    employee = self.pool.get('hr.employee').browse(cr, uid, employee_id, context=context)
    ...

become

employee_obj = self.pool['hr.employee']
employee_ids = employee_obj.search(cr, uid, [('user_id','=',user_id)],
                                   context=context) # PEP8
for employee in employee_obj.browse(cr, uid, employee_ids, context=context):
    ...

- I'm not sure about the flow you propose, because there is already an onchange method for section_id that assigns section department to the lead. Maybe a better flow is to only assign section_id from the user, and let onchange_section_id to do the rest, or directly get department from hr.employee only (because we are working at employee level, not section level). First approach requires to propose the patch on core, not here, so I bet for the second one.

Sorry if I am too picky, but I hope this doesn't discourage you from contributing.

Regards.
-- 
https://code.launchpad.net/~jb.eficent/department-mgmt/department-mgmt-bugfix-1296055/+merge/212286
Your team Department Core Editors is subscribed to branch lp:department-mgmt.