Code review comment for lp:~jgrandguillaume-c2c/openobject-addons/multi-company-cost-price

Revision history for this message
Xavier (Open ERP) (xmo-deactivatedaccount) wrote :

Fabien considers the business logic good so I have no issue with that, but on the style/tech front I have a few from a quick overview of the branch:

Inconsistency
=============

stock/wizard/wizard_partial_move.py and stock/wizard/wizard_partial_picking.py seem to have been modified in similar but not identical ways: in both cases `amount_unit` is computed through `product.price_get(pricetype.field, context)[product.id]` but while the latter uses `amount_unit` on the next line, the former uses `product.amount_unit` and never calls `amount_unit`. This might be a bug in either of them.

(I also note a high level of redundancy between these wizards (at least on this specific method), they might benefit from from de-duplication in the future, to avoid that kind of potential issues)

Commented code
==============

In a few places, you commented existing code or committed commented code. Please remove these instances, removed code can be found via the VCS's history, and I don't quite see the justification for added already commented code (if it's work in progress its place is in a non-merged branch, if it's something to add/complete, it should be in a bug on the tracker):

* hr_timesheet_invoice/hr_timesheet_invoice.py at revid:<email address hidden> (commented existing code)
* account_analytic_analysis/account_analytic_analysis.py at revid:<email address hidden> (added a commented method)

Style
=====

At revid:<email address hidden>, in account_analytic_analysis/account_analytic_analysis.py you perform a test using dict.has_key. In this case, please either use `'key' in dict` (`has_key` is deprecated using Python2.6 -3, and doesn't exist anymore in Python 3). Furthermore this specific instance would probably be simpler by using `dict.setdefault` or a `collections.defaultdict` instance.

As a side note, in a few places you edited code using old-style difference operators (<>). If you can (not in this branch, but in the future) don't hesitate swapping them for "modern" difference operators (!=).

review: Needs Fixing

« Back to merge proposal