Code review comment for lp:~therp-nl/account-financial-tools/6.1-account-purchase-default

Revision history for this message
Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c) wrote :

A few suggested refactorings to improve code maintainability.

line 110: you check on line 112 for the truth of partner_id, so you can safely use the default default value in the call to dict.get()

line 112-113: you can get rid of the 'and invoice_type', since you have an in valid_value check just after this which will return False if invoice_type is None.
context['partner_id']

line 116: no need for another lookup of context['partner_id']: this value is partner_id from line 110

line 117: I personally prefer the attribute access on records, as in
    if partner_obj and partner_obj.property_account_expense:

 * Is there really a case where the partner_id you got from the context can be non existing? Apart from concurrent read / deletion, this sounds far fetched, so I'd be tempted to remove the first part of the test. That remark also applies to line 134.

line 136-137:
  * you are mixing attribute style access and dict style access.
  * the "or 0" bit is not necessary : the and expression will evaluate to False and replacing False by 0 does not change the outcome of the test line 138

line 138: please use "if a != b" rather than "if not a == b"

review: Needs Fixing (code review, no test)

« Back to merge proposal