Review: Needs Fixing code
* What's the point of DiagramView.get_activity? Why not just do two read_slice
in javascript?
* For diagram_info, why is it taking a **kwargs when you know the name of
every single argument (and half of them are mandatory), just to spend the first
10 lines of the methods unpacking **kw into local variables?
* Handling of search (which should not be there in the first place) calls a
method "schedule_event" which does not exist… (diagram.js:338)
* There's an unused variable (diagram.js:270)
* For things which don't have to do with manipulating the DOM and events, I'd
rather you use underscore than jquery, e.g. $.each (diagram.js:230,
diagram.js:244) or jQuery.inArray (diagram.js:274) could be replaced by _.each
and _.contains (warning: order of parameters might be different).
* Talking about jquery, please stick to its usual name: `$`. We needed to use
`jQuery` in the previous client due to conflicts with other libraries, there is
no such concern in the new client.
* I'm not exactly fond of all the magic numbers in the code (e.g.
diagram.js:171, why `n.node.x - 30`? And why `n.node.y - 13`?)
* Why is the graph variable called `dia`? There's no limitation on the size of
variable names, don't use abbreviations if it does not make the code clearer.
* There are quite a bit of redundancies between the node-creation clauses: the
attributes are the same in all except for `r` which varies in the circle case,
and the `set` variable is created the same way, with just a difference in
offset. It would be nice to deduplicate those.
* I find the crapload of instance variables created from line 49 onwards (in
diagram.js) debatable: why not just create a structure which could be provided
almost directly to the `get_diagram_info` RPC call
* The diagram view has *only* two children (one node and one arrow, in this
order, I fixed the schema to ensure that). There is no reason to perform an
iteration with nodename check (diagram.js:62), just get the first child (it's
the node descriptor) and the second child (it's the second child). Also, both
node and arrow can only have `field` children, no need to check for child's tag.
* Avoid iterating on arrays with "for…in", it's slower and risky (if somebody
added new methods on array, which should not happen but you never know). Either
use for(;;;) or Underscore's each method (diagram.js:67, diagram.js:87)
* I'm not sure I understand why the conditional expression in
diagram.js:{71,72} are so complex. could you explain?
--
https://code.launchpad.net/~openerp-dev/openerp-web/diagram-vda/+merge/73350
Your team OpenERP R&D Team is subscribed to branch
lp:~openerp-dev/openerp-web/diagram-vda.
_______________________________________________
Mailing list: https://launchpad.net/~openerp-dev-gtk
Post to : [email protected]
Unsubscribe : https://launchpad.net/~openerp-dev-gtk
More help : https://help.launchpad.net/ListHelp
