From 3bbb11353a0bf03c12bc08a6f2832b386d852b15 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 17 Jun 2011 11:07:46 +0200 Subject: [PATCH 01/14] [FIX] non-instance should all be declared with 'var' See doc for rationale bzr revid: xmo@openerp.com-20110617090746-z35n4jsdqvicoa9h --- addons/base_graph/static/src/js/graph.js | 12 ++++---- doc/source/project.rst | 37 ++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 6 deletions(-) diff --git a/addons/base_graph/static/src/js/graph.js b/addons/base_graph/static/src/js/graph.js index 0c0f9deb629..e1805f2d07a 100644 --- a/addons/base_graph/static/src/js/graph.js +++ b/addons/base_graph/static/src/js/graph.js @@ -84,8 +84,8 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ this.$element.html(QWeb.render("GraphView", {"fields_view": this.fields_view, "chart": this.chart,'view_id': this.view_id})); this.opration_fld = {}; if (result.length){ - for(res in result) { - for(fld in result[res]) { + for(var res in result) { + for(var fld in result[res]) { if (typeof result[res][fld] == 'object') { result[res][fld] = result[res][fld][result[res][fld].length - 1]; } @@ -101,7 +101,7 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ } } - for (i in result){ + for (var i in result){ var gen_key = result[i][this.chart_info_fields]+"_"+result[i][this.group_field]; if (this.opration_fld[gen_key] == undefined){ var map_val = {} @@ -182,7 +182,7 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ if (xystr[xystring] == undefined){ xyname = {}; xyname[self.chart_info_fields] = xystring; - for (j in self.group_list){ + for (var j in self.group_list){ xyname[self.group_list[j]] = 0.0001; } xyname[newkey] = result[i][self.operator_field]; @@ -207,7 +207,7 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ //for legend color var grp_color = []; - for (i in self.group_list){ + for (var i in self.group_list){ var legend = {}; if (self.group_list[i] == "val"){ legend['text'] = self.fields[self.operator_field]['string'] @@ -381,4 +381,4 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ }; -// vim:et fdc=0 fdl=0: \ No newline at end of file +// vim:et fdc=0 fdl=0: diff --git a/doc/source/project.rst b/doc/source/project.rst index faa15da2163..80ed0e86143 100644 --- a/doc/source/project.rst +++ b/doc/source/project.rst @@ -16,6 +16,43 @@ Source code repository Merge proposals +++++++++++++++ +Coding issues and coding conventions +++++++++++++++++++++++++++++++++++++ + +Javascript coding +~~~~~~~~~~~~~~~~~ + +Use ``var`` for *all* declarations +********************************** + +In javascript (as opposed to Python), assigning to a variable which does not +already exist and is not explicitly declared (via ``var``) will implicitly +create a global variable. This is bad for a number of reasons: + +* It leaks information outside function scopes +* It keeps memory of previous run, with potentially buggy behaviors +* It may conflict with other functions with the same issue +* It makes code harder to statically check (via e.g. IDE inspectors) + +.. note:: + It is perfectly possible to use ``var`` in ``for`` loops: + + .. code-block:: javascript + + for (var i = 0; i < some_array.length; ++i) { + // code here + } + + this is not an issue + +All local *and global* variables should be declared via ``var``. + +.. note:: generally speaking, you should not need globals in OpenERP Web: you + can just declare a variable local to your top-level function. This + way, if your widget/addon is instantiated several times on the same + page (because it's used in embedded mode) each instance will have its + own internal but global-to-its-objects data. + Writing documentation +++++++++++++++++++++ From ceda49fbad9b86b140012011c4a07087aa31537d Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 17 Jun 2011 11:13:35 +0200 Subject: [PATCH 02/14] [FIX] terminate statements missing semicolons bzr revid: xmo@openerp.com-20110617091335-r9k07mr5m5lp9f0o --- addons/base_graph/static/src/js/graph.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/addons/base_graph/static/src/js/graph.js b/addons/base_graph/static/src/js/graph.js index e1805f2d07a..719ef55df5d 100644 --- a/addons/base_graph/static/src/js/graph.js +++ b/addons/base_graph/static/src/js/graph.js @@ -104,7 +104,7 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ for (var i in result){ var gen_key = result[i][this.chart_info_fields]+"_"+result[i][this.group_field]; if (this.opration_fld[gen_key] == undefined){ - var map_val = {} + var map_val = {}; map_val[this.operator_field] = result[i][this.operator_field]; if (this.operator.length > 1){ map_val[this.operator_field_one] = result[i][this.operator_field_one]; @@ -123,7 +123,7 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ this.opration_fld[gen_key] = map_val; } } - result = [] + result = []; for (i in this.opration_fld){ result.push(this.opration_fld[i]); } From 7ed58deec796efcd4c3cd14bb85f789d081428c6 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 17 Jun 2011 11:13:44 +0200 Subject: [PATCH 03/14] [FIX] remove trailing commas in object literals bzr revid: xmo@openerp.com-20110617091344-mlal79a9poagjwm3 --- addons/base_graph/static/src/js/graph.js | 4 ++-- doc/source/project.rst | 30 ++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/addons/base_graph/static/src/js/graph.js b/addons/base_graph/static/src/js/graph.js index 719ef55df5d..c67a4494594 100644 --- a/addons/base_graph/static/src/js/graph.js +++ b/addons/base_graph/static/src/js/graph.js @@ -283,7 +283,7 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ marker:{ type:"round", width:12 - }, + } } }); for (var i = 1; i Date: Fri, 17 Jun 2011 11:13:54 +0200 Subject: [PATCH 04/14] [IMP] simplify summing: use the += operator to avoid redundancies and excessively long lines bzr revid: xmo@openerp.com-20110617091354-nus2mw3r7g2bvbrr --- addons/base_graph/static/src/js/graph.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/addons/base_graph/static/src/js/graph.js b/addons/base_graph/static/src/js/graph.js index c67a4494594..e0a3c1feeea 100644 --- a/addons/base_graph/static/src/js/graph.js +++ b/addons/base_graph/static/src/js/graph.js @@ -116,9 +116,9 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ this.opration_fld[gen_key] = map_val; }else{ map_val = this.opration_fld[gen_key]; - map_val[this.operator_field] = map_val[this.operator_field] + result[i][this.operator_field]; + map_val[this.operator_field] += result[i][this.operator_field]; if (this.operator.length > 1){ - map_val[this.operator_field_one] = map_val[this.operator_field_one] + result[i][this.operator_field_one]; + map_val[this.operator_field_one] += result[i][this.operator_field_one]; } this.opration_fld[gen_key] = map_val; } From c5e061fec9bc1594f7a99e26970ecf12211ed95f Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 17 Jun 2011 11:24:36 +0200 Subject: [PATCH 05/14] [FIX] don't make explicit boolean comparisons without identity In javascript, '0 == false', '1 == true' and even '"1" == true'. Either use variables directly in a boolean context, or use type-strict equality (===) to ensure the variable is an actual boolean, not some other type coerced to a boolean bzr revid: xmo@openerp.com-20110617092436-s8b5v4v5ytbonvi1 --- addons/base_graph/static/src/js/graph.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/addons/base_graph/static/src/js/graph.js b/addons/base_graph/static/src/js/graph.js index e0a3c1feeea..7a9073996e9 100644 --- a/addons/base_graph/static/src/js/graph.js +++ b/addons/base_graph/static/src/js/graph.js @@ -250,7 +250,7 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ template:function(obj){ if(x_ax['template']){ var val = obj[x_ax['template']]; - val = (typeof val == 'object')?val[1]:(val==false?'Undefined':val); + val = (typeof val == 'object')?val[1]:(!val?'Undefined':val); return val; }else{ return obj; @@ -263,7 +263,7 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ template:function(obj){ if(y_ax['template']){ var vals = obj[y_ax['template']]; - vals = (typeof vals == 'object')?vals[1]:(vals==false?'Undefined':vals); + vals = (typeof vals == 'object')?vals[1]:(!vals?'Undefined':vals); return vals; }else{ return obj; From 485adfdef9db1ecac8cefc4b171fd940eadd5a91 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 17 Jun 2011 11:32:22 +0200 Subject: [PATCH 06/14] [FIX] avoid reusing variables in completely different contexts, especially function parameters. Also avoid declaring the same variables multiple times in the same function, either split the usage into different variables (if it's reuse) or declare only once (if it's alternate initialization) bzr revid: xmo@openerp.com-20110617093222-8mx8tkshkcave3g1 --- addons/base_graph/static/src/js/graph.js | 87 ++++++++++++------------ 1 file changed, 43 insertions(+), 44 deletions(-) diff --git a/addons/base_graph/static/src/js/graph.js b/addons/base_graph/static/src/js/graph.js index 7a9073996e9..73609175798 100644 --- a/addons/base_graph/static/src/js/graph.js +++ b/addons/base_graph/static/src/js/graph.js @@ -39,7 +39,7 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ this.group_field = ''; this.orientation = this.fields_view.arch.attrs.orientation || ''; - for(fld in this.fields_view.arch.children) { + for(var fld in this.fields_view.arch.children) { if (this.fields_view.arch.children[fld].attrs.operator) { this.operator.push(this.fields_view.arch.children[fld].attrs.name); } @@ -91,7 +91,7 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ } if (typeof result[res][fld] == 'string'){ if (this.all_fields[fld]['selection']){ - for (i in this.all_fields[fld]['selection']){ + for (var i in this.all_fields[fld]['selection']){ if(result[res][fld] == this.all_fields[fld]['selection'][i][0]){ result[res][fld] = this.all_fields[fld]['selection'][i]; } @@ -101,37 +101,37 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ } } - for (var i in result){ - var gen_key = result[i][this.chart_info_fields]+"_"+result[i][this.group_field]; + for (var j in result){ + var gen_key = result[j][this.chart_info_fields]+"_"+result[j][this.group_field]; if (this.opration_fld[gen_key] == undefined){ var map_val = {}; - map_val[this.operator_field] = result[i][this.operator_field]; + map_val[this.operator_field] = result[j][this.operator_field]; if (this.operator.length > 1){ - map_val[this.operator_field_one] = result[i][this.operator_field_one]; + map_val[this.operator_field_one] = result[j][this.operator_field_one]; } - map_val[this.chart_info_fields] = result[i][this.chart_info_fields]; + map_val[this.chart_info_fields] = result[j][this.chart_info_fields]; if (this.group_field){ - map_val[this.group_field] = (typeof result[i][this.group_field] == 'object')?result[i][this.group_field][1]:result[i][this.group_field]; + map_val[this.group_field] = (typeof result[j][this.group_field] == 'object')?result[j][this.group_field][1]:result[j][this.group_field]; } this.opration_fld[gen_key] = map_val; }else{ map_val = this.opration_fld[gen_key]; - map_val[this.operator_field] += result[i][this.operator_field]; + map_val[this.operator_field] += result[j][this.operator_field]; if (this.operator.length > 1){ - map_val[this.operator_field_one] += result[i][this.operator_field_one]; + map_val[this.operator_field_one] += result[j][this.operator_field_one]; } this.opration_fld[gen_key] = map_val; } } - result = []; - for (i in this.opration_fld){ - result.push(this.opration_fld[i]); + var graph_data = []; + for (var op in this.opration_fld){ + graph_data.push(this.opration_fld[op]); } if(this.chart == 'bar') { - return this.schedule_bar(result); + return this.schedule_bar(graph_data); } else if(this.chart == "pie") { - return this.schedule_pie(result); + return this.schedule_pie(graph_data); } } }, @@ -156,7 +156,7 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ view_chart = self.orientation == 'horizontal'? 'barH' : 'bar'; } - for (i in result){ + for (var i in result){ if (self.group_field && (this.operator.length <= 1)){ newkey =result[i][self.group_field].split(' ').join('_'); }else{ @@ -172,51 +172,51 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ } } - for (i in result){ - var xystring = result[i][self.chart_info_fields]; + for (var j in result){ + var xystring = result[j][self.chart_info_fields]; if (self.group_field && (self.operator.length <= 1)){ - newkey =result[i][self.group_field].split(' ').join('_'); + newkey =result[j][self.group_field].split(' ').join('_'); }else{ newkey = "val"; } if (xystr[xystring] == undefined){ xyname = {}; xyname[self.chart_info_fields] = xystring; - for (var j in self.group_list){ - xyname[self.group_list[j]] = 0.0001; + for (var k in self.group_list){ + xyname[self.group_list[k]] = 0.0001; } - xyname[newkey] = result[i][self.operator_field]; + xyname[newkey] = result[j][self.operator_field]; if (self.operator.length > 1){ - xyname[newkey_one] = result[i][self.operator_field_one]; + xyname[newkey_one] = result[j][self.operator_field_one]; } xystr[xystring] = xyname; } else{ xyname = {}; xyname = xystr[xystring]; - xyname[newkey] = result[i][self.operator_field]; + xyname[newkey] = result[j][self.operator_field]; if (self.operator.length > 1){ - xyname[newkey_one] = result[i][self.operator_field_one]; + xyname[newkey_one] = result[j][self.operator_field_one]; } xystr[xystring] = xyname; } } - for (i in xystr){ - res.push(xystr[i]); + for (var label in xystr){ + res.push(xystr[label]); } //for legend color var grp_color = []; - for (var i in self.group_list){ + for (var l in self.group_list){ var legend = {}; - if (self.group_list[i] == "val"){ + if (self.group_list[l] == "val"){ legend['text'] = self.fields[self.operator_field]['string'] - }else if(self.group_list[i] == "val1"){ + }else if(self.group_list[l] == "val1"){ legend['text'] = self.fields[self.operator_field_one]['string'] }else{ - legend['text'] = self.group_list[i]; + legend['text'] = self.group_list[l]; } - legend['color'] = COLOR_PALETTE[i]; + legend['color'] = COLOR_PALETTE[l]; grp_color.push(legend); } @@ -228,12 +228,13 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ oth_ax['lines'] = true; oth_ax['title'] = ""+self.y_title+""; + var x_ax, y_ax; if (self.orientation == 'horizontal'){ - var x_ax = oth_ax; - var y_ax = temp_ax; + x_ax = oth_ax; + y_ax = temp_ax; }else{ - var x_ax = temp_ax; - var y_ax = oth_ax; + x_ax = temp_ax; + y_ax = oth_ax; } var bar_chart = new dhtmlxchartChart({ @@ -286,16 +287,15 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ } } }); - for (var i = 1; i Date: Fri, 17 Jun 2011 11:32:59 +0200 Subject: [PATCH 07/14] [FIX] Assigning variables to themselves is a NOOP in Javascript, and just noise bzr revid: xmo@openerp.com-20110617093259-eltod1l1ytsyq82h --- addons/base_graph/static/src/js/graph.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/addons/base_graph/static/src/js/graph.js b/addons/base_graph/static/src/js/graph.js index 73609175798..bd54a68a117 100644 --- a/addons/base_graph/static/src/js/graph.js +++ b/addons/base_graph/static/src/js/graph.js @@ -338,8 +338,6 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ id = id[self.chart_info_fields]; if (typeof id == 'object'){ id = id[0]; - }else{ - id = id; } if(this.view_manager.action.context){ this.view_manager.action.context = {}; From ff50ec64079cf6b8b73db1091d299bf02a1e73b2 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 17 Jun 2011 12:29:31 +0200 Subject: [PATCH 08/14] [FIX] syntax highlighting settings on a pair of code blocks bzr revid: xmo@openerp.com-20110617102931-85kof2qm1m5psnwl --- doc/source/project.rst | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/doc/source/project.rst b/doc/source/project.rst index 38e2fe507dd..37a16159448 100644 --- a/doc/source/project.rst +++ b/doc/source/project.rst @@ -65,7 +65,7 @@ Do not leave trailing commas in object literals While it is legal to leave trailing commas in Python dictionaries, e.g. -:: +.. code-block:: python foo = { 'a': 1, @@ -111,14 +111,18 @@ RST, using Sphinx's `Python domain`_ [#]_: docstring, using Sphinx's `info fields`_ For parameters types, built-in and stdlib types should be using the - combined syntax:: + combined syntax: + + .. code-block:: restructuredtext :param dict foo: what the purpose of foo is unless a more extensive explanation needs to be given (e.g. the specification that the input should be a list of 3-tuple needs to use ``:type:`` even though all types involved are built-ins). Any - other type should be specified in full using the ``:type:`` field:: + other type should be specified in full using the ``:type:`` field + + .. code-block:: restructuredtext :param foo: what the purpose of foo is :type foo: some.addon.Class @@ -154,12 +158,16 @@ when writing javascript API documentation: are not documented, or JsDoc will not understand what they are and will not generate documentation for their content. - As a result, the bare minimum for a namespace is:: + As a result, the bare minimum for a namespace is: + + .. code-block:: javascript /** @namespace */ foo.bar.baz = {}; - while for a class it is:: + while for a class it is: + + .. code-block:: javascript /** @class */ foo.bar.baz.Qux = [...] @@ -256,7 +264,6 @@ with the descriptions and irrelevant atttributes stripped): .. code-block:: javascript - openerp.base.search.Widget = openerp.base.Controller.extend( /** @lends openerp.base.search.Widget# */{ /** From 8ad063965daef98cdd78c6c0efcfbc1fb76fed9d Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 17 Jun 2011 13:52:26 +0200 Subject: [PATCH 09/14] [FIX] do not use for...in to iterate on arrays, avoid using it to iterate on object as well. Use _.each in either cases. See doc for rationale bzr revid: xmo@openerp.com-20110617115226-21meg23d0tg43wgv --- addons/base_graph/static/src/js/graph.js | 126 ++++++++++++----------- doc/source/project.rst | 123 ++++++++++++++++++++++ 2 files changed, 187 insertions(+), 62 deletions(-) diff --git a/addons/base_graph/static/src/js/graph.js b/addons/base_graph/static/src/js/graph.js index bd54a68a117..2aa290e61f2 100644 --- a/addons/base_graph/static/src/js/graph.js +++ b/addons/base_graph/static/src/js/graph.js @@ -39,17 +39,18 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ this.group_field = ''; this.orientation = this.fields_view.arch.attrs.orientation || ''; - for(var fld in this.fields_view.arch.children) { - if (this.fields_view.arch.children[fld].attrs.operator) { - this.operator.push(this.fields_view.arch.children[fld].attrs.name); + _.each(this.fields_view.arch.children, function (field) { + if (field.attrs.operator) { + this.operator.push(field.attrs.name); } - else if (this.fields_view.arch.children[fld].attrs.group) { - this.group_field = this.fields_view.arch.children[fld].attrs.name; + else if (field.attrs.group) { + this.group_field = field.attrs.name; } else { - this.chart_info_fields.push(this.fields_view.arch.children[fld].attrs.name); + this.chart_info_fields.push(field.attrs.name); } - } + }, this); + this.operator_field = this.operator[0]; if(this.operator.length > 1){ this.operator_field_one = this.operator[1]; @@ -80,53 +81,53 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ } }, - schedule_chart: function(result) { + schedule_chart: function(results) { this.$element.html(QWeb.render("GraphView", {"fields_view": this.fields_view, "chart": this.chart,'view_id': this.view_id})); this.opration_fld = {}; - if (result.length){ - for(var res in result) { - for(var fld in result[res]) { - if (typeof result[res][fld] == 'object') { - result[res][fld] = result[res][fld][result[res][fld].length - 1]; + if (results.length){ + _.each(results, function (result) { + _.each(result, function (field_value, field_name) { + if (typeof field_value == 'object') { + result[field_name] = field_value[field_value.length - 1]; } - if (typeof result[res][fld] == 'string'){ - if (this.all_fields[fld]['selection']){ - for (var i in this.all_fields[fld]['selection']){ - if(result[res][fld] == this.all_fields[fld]['selection'][i][0]){ - result[res][fld] = this.all_fields[fld]['selection'][i]; - } + if (typeof field_value == 'string'){ + var choices = this.all_fields[field_name]['selection']; + _.each(choices, function (choice) { + if(field_value == choice[0]){ + result[field_name] = choice; } - } + }); } - } - } + }, this); + }, this); - for (var j in result){ - var gen_key = result[j][this.chart_info_fields]+"_"+result[j][this.group_field]; + _.each(results, function (result) { + var gen_key = result[this.chart_info_fields]+"_"+result[this.group_field]; if (this.opration_fld[gen_key] == undefined){ var map_val = {}; - map_val[this.operator_field] = result[j][this.operator_field]; + map_val[this.operator_field] = result[this.operator_field]; if (this.operator.length > 1){ - map_val[this.operator_field_one] = result[j][this.operator_field_one]; + map_val[this.operator_field_one] = result[this.operator_field_one]; } - map_val[this.chart_info_fields] = result[j][this.chart_info_fields]; + map_val[this.chart_info_fields] = result[this.chart_info_fields]; if (this.group_field){ - map_val[this.group_field] = (typeof result[j][this.group_field] == 'object')?result[j][this.group_field][1]:result[j][this.group_field]; + map_val[this.group_field] = (typeof result[this.group_field] == 'object')?result[this.group_field][1]:result[this.group_field]; } this.opration_fld[gen_key] = map_val; - }else{ + } else { map_val = this.opration_fld[gen_key]; - map_val[this.operator_field] += result[j][this.operator_field]; + map_val[this.operator_field] += result[this.operator_field]; if (this.operator.length > 1){ - map_val[this.operator_field_one] += result[j][this.operator_field_one]; + map_val[this.operator_field_one] += result[this.operator_field_one]; } this.opration_fld[gen_key] = map_val; } - } + }, this); + var graph_data = []; - for (var op in this.opration_fld){ - graph_data.push(this.opration_fld[op]); - } + _.each(this.opration_fld, function (column_data) { + graph_data.push(column_data); + }); if(this.chart == 'bar') { return this.schedule_bar(graph_data); @@ -136,14 +137,14 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ } }, - schedule_bar: function(result) { + schedule_bar: function(results) { var self = this; var view_chart = ''; var xystr = {}; var xyname = {}; var res = []; this.group_list = []; - var newkey = ''; + var newkey = '', newkey_one; var COLOR_PALETTE = ['#cc99ff', '#ccccff', '#48D1CC', '#CFD784', '#8B7B8B', '#75507b', '#b0008c', '#ff0000', '#ff8e00', '#9000ff', '#0078ff', '#00ff00', '#e6ff00', '#ffff00', '#905000', '#9b0000', '#840067', '#9abe00', '#ffc900', '#510090', '#0000c9', '#009b00', @@ -156,69 +157,70 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ view_chart = self.orientation == 'horizontal'? 'barH' : 'bar'; } - for (var i in result){ + _.each(results, function (result) { if (self.group_field && (this.operator.length <= 1)){ - newkey =result[i][self.group_field].split(' ').join('_'); - }else{ + newkey = result[self.group_field].split(' ').join('_'); + } else { newkey = "val"; } if (jQuery.inArray(newkey, self.group_list) == -1){ self.group_list.push(newkey); if(this.operator.length > 1){ - var newkey_one = "val1"; + newkey_one = "val1"; self.group_list.push(newkey_one); } } - } + }, this); - for (var j in result){ - var xystring = result[j][self.chart_info_fields]; + _.each(results, function (result) { + var xystring = result[self.chart_info_fields]; if (self.group_field && (self.operator.length <= 1)){ - newkey =result[j][self.group_field].split(' ').join('_'); + newkey = result[self.group_field].split(' ').join('_'); }else{ newkey = "val"; } if (xystr[xystring] == undefined){ xyname = {}; xyname[self.chart_info_fields] = xystring; - for (var k in self.group_list){ - xyname[self.group_list[k]] = 0.0001; - } - xyname[newkey] = result[j][self.operator_field]; + _.each(self.group_list, function (group) { + xyname[group] = 0.0001; + }); + xyname[newkey] = result[self.operator_field]; if (self.operator.length > 1){ - xyname[newkey_one] = result[j][self.operator_field_one]; + xyname[newkey_one] = result[self.operator_field_one]; } xystr[xystring] = xyname; } else{ xyname = {}; xyname = xystr[xystring]; - xyname[newkey] = result[j][self.operator_field]; + xyname[newkey] = result[self.operator_field]; if (self.operator.length > 1){ - xyname[newkey_one] = result[j][self.operator_field_one]; + xyname[newkey_one] = result[self.operator_field_one]; } xystr[xystring] = xyname; } - } - for (var label in xystr){ - res.push(xystr[label]); - } + }); + + _.each(xystr, function (column_data) { + res.push(column_data); + }); //for legend color var grp_color = []; - for (var l in self.group_list){ + _.each(self.group_list, function (group_legend, index) { var legend = {}; - if (self.group_list[l] == "val"){ + if (group_legend == "val"){ legend['text'] = self.fields[self.operator_field]['string'] - }else if(self.group_list[l] == "val1"){ + }else if(group_legend == "val1"){ legend['text'] = self.fields[self.operator_field_one]['string'] }else{ - legend['text'] = self.group_list[l]; + legend['text'] = group_legend; } - legend['color'] = COLOR_PALETTE[l]; + legend['color'] = COLOR_PALETTE[index]; grp_color.push(legend); - } + }); //for axis's value and title var temp_ax = {}; diff --git a/doc/source/project.rst b/doc/source/project.rst index 37a16159448..aa465aa3f9f 100644 --- a/doc/source/project.rst +++ b/doc/source/project.rst @@ -83,6 +83,91 @@ should *never* use trailing commas in Javascript object literals: in object literals puts you at risks of using them in literal JSON strings as well (though there are few reasons to write JSON by hand) +*Never* use ``for … in`` to iterate on arrays +********************************************* + +:ref:`Iterating over an object with for…in is a bit tricky already +`, it is far more complex than in Python (where it Just +Works™) due to the interaction of various Javascript features, but to iterate +on arrays it becomes downright deadly and errorneous: ``for…in`` really +iterates over an *object*'s *properties*. + +With an array, this has the following consequences: + +* It does not necessarily iterate in numerical order, nor does it iterate in + any kind of set order. The order is implementation-dependent and may vary + from one run to the next depending on a number of reasons and implementation + details. +* If properties are added to an array, to ``Array.prototype`` or to + ``Object.prototype`` (the latter two should not happen in well-behaved + javascript code, but you never know...) those properties *will* be iterated + over by ``for…in``. While ``Object.hasOwnProperty`` will guard against + iterating prototype properties, they will not guard against properties set + on the array instance itself (as memoizers for instance). + + Note that this includes setting negative keys on arrays. + +For this reason, ``for…in`` should **never** be used on array objects. Instead, +you should use either a normal ``for`` or (even better, unless you have +profiled the code and found a hotspot) one of Underscore's array iteration +methods (`_.each`_, `_.map`_, `_.filter`_, etc...). + +Underscore is guaranteed to be bundled and available in OpenERP Web scopes. + +.. _for-in-iteration: + +Use ``hasOwnProperty`` when iterating on an object with ``for … in`` +******************************************************************** + +``for…in`` is Javascript's built-in facility for iterating over and object's +properties. + +`It is also fairly tricky to use`_: it iterates over *all* non-builtin +properties of your objects [#]_, which includes methods of an object's class. + +As a result, when iterating over an object with ``for…in`` the first line of +the body *should* generally be a call to `Object.hasOwnProperty`_. This call +will check whether the property was set directly on the object or comes from +the object's class: + +.. code-block:: javascript + + for(var key in ob) { + if (!ob.hasOwnProperty(key)) { + // comes from ob's class + continue; + } + // do stuff with key + } + +Since properties can be added directly to e.g. ``Object.prototype`` (even +though it's usually considered bad style), you should not assume you ever know +which properties ``for…in`` is going to iterate over. + +An alternative is to use Underscore's iteration methods, which generally work +over objects as well as arrays: + +Instead of + +.. code-block:: javascript + + for (var key in ob) { + if (!ob.hasOwnProperty(key)) { continue; } + var value = ob[key]; + // Do stuff with key and value + } + +you could write: + +.. code-block:: javascript + + _.each(ob, function (value, key) { + // do stuff with key and value + }); + +and not worry about the details of the iteration: underscore should do the +right thing for you on its own [#]_. + Writing documentation +++++++++++++++++++++ @@ -294,6 +379,30 @@ Roadmap Release notes +++++++++++++ +.. [#] More precisely, it iterates over all *enumerable* properties. It just + happens that built-in properties (such as ``String.indexOf`` or + ``Object.toString``) are set to non-enumerable. + + The enumerability of a property can be checked using + `Object.propertyIsEnumeable`_. + + Before ECMAScript 5, it was not possible for user-defined properties + to be non-enumerable in a portable manner. ECMAScript 5 introduced + `Object.defineProperty`_ which lets user code create non-enumerable + properties (and more, read-only properties for instance, or implicit + getters and setters). However, support for these is not fully complete + at this point, and they are not being used in OpenERP Web code anyway. + +.. [#] While using underscore is generally the preferred method (simpler, + more reliable and easier to write than a *correct* ``for…in`` + iteration), it is also probably slower (due to the overhead of + calling a bunch of functions). + + As a result, if you profile some code and find out that an underscore + method adds unacceptable overhead in a tight loop, you may want to + replace it with a ``for…in`` (or a regular ``for`` statement for + arrays). + .. [#] Because Python is the default domain, the ``py:`` markup prefix is optional and should be left out. @@ -326,3 +435,17 @@ Release notes http://code.google.com/p/jsdoc-toolkit/ .. _John Resig's Class implementation: http://ejohn.org/blog/simple-javascript-inheritance/ +.. _\_.each: + http://documentcloud.github.com/underscore/#each +.. _\_.map: + http://documentcloud.github.com/underscore/#map +.. _\_.filter: + http://documentcloud.github.com/underscore/#select +.. _It is also fairly tricky to use: + https://developer.mozilla.org/en/JavaScript/Reference/Statements/for...in#Description +.. _Object.propertyIsEnumeable: + https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Object/propertyIsEnumerable +.. _Object.defineProperty: + https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Object/defineProperty +.. _Object.hasOwnProperty: + https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Object/hasOwnProperty From 413054ce53d20ace6813e26db118399150761829 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 17 Jun 2011 13:55:59 +0200 Subject: [PATCH 10/14] [IMP] use object literals when possible: clearer lower amount of noise, and clearer relations between the object and its components bzr revid: xmo@openerp.com-20110617115559-n89evkkufc3f8k9t --- addons/base_graph/static/src/js/graph.js | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/addons/base_graph/static/src/js/graph.js b/addons/base_graph/static/src/js/graph.js index 2aa290e61f2..81b99cbddbd 100644 --- a/addons/base_graph/static/src/js/graph.js +++ b/addons/base_graph/static/src/js/graph.js @@ -210,7 +210,8 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ //for legend color var grp_color = []; _.each(self.group_list, function (group_legend, index) { - var legend = {}; + var legend = {color: COLOR_PALETTE[index]}; + if (group_legend == "val"){ legend['text'] = self.fields[self.operator_field]['string'] }else if(group_legend == "val1"){ @@ -218,17 +219,18 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ }else{ legend['text'] = group_legend; } - legend['color'] = COLOR_PALETTE[index]; grp_color.push(legend); }); //for axis's value and title - var temp_ax = {}; - var oth_ax = {}; - temp_ax['template'] = self.chart_info_fields; - temp_ax['title'] = ""+self.x_title+"" ; - oth_ax['lines'] = true; - oth_ax['title'] = ""+self.y_title+""; + var temp_ax = { + template: self.chart_info_fields, + title: ""+self.x_title+"" + }; + var oth_ax = { + lines: true, + title: ""+self.y_title+"" + }; var x_ax, y_ax; if (self.orientation == 'horizontal'){ From 57bd9de5023b26ae1e965b2b8f237bb99cd9513d Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 17 Jun 2011 15:22:22 +0200 Subject: [PATCH 11/14] [IMP] reduce variable scopes: move instance variables to function scopes, remove unneeded temps, move variables to inner function scopes. Variable scopes should be as small as possible (but no smaller) in order to make their data flow easy to understand bzr revid: xmo@openerp.com-20110617132222-p0jabvioctjcn9o4 --- addons/base_graph/static/src/js/graph.js | 68 +++++++++--------------- 1 file changed, 26 insertions(+), 42 deletions(-) diff --git a/addons/base_graph/static/src/js/graph.js b/addons/base_graph/static/src/js/graph.js index 81b99cbddbd..7ebeffa78ab 100644 --- a/addons/base_graph/static/src/js/graph.js +++ b/addons/base_graph/static/src/js/graph.js @@ -25,7 +25,6 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ this.rpc("/base_graph/graphview/load", {"model": this.model, "view_id": this.view_id}, this.on_loaded); }, on_loaded: function(data) { - var self = this; this.all_fields = data.all_fields; this.fields_view = data.fields_view; this.name = this.fields_view.name || this.fields_view.arch.attrs.string; @@ -61,7 +60,7 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ this.chart_info = this.chart_info_fields[0]; this.x_title = this.fields[this.chart_info_fields[0]]['string']; this.y_title = this.fields[this.operator_field]['string']; - self.load_chart(); + this.load_chart(); }, load_chart: function(data) { @@ -83,7 +82,6 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ schedule_chart: function(results) { this.$element.html(QWeb.render("GraphView", {"fields_view": this.fields_view, "chart": this.chart,'view_id': this.view_id})); - this.opration_fld = {}; if (results.length){ _.each(results, function (result) { _.each(result, function (field_value, field_name) { @@ -101,10 +99,11 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ }, this); }, this); + var opration_fld = {}; _.each(results, function (result) { var gen_key = result[this.chart_info_fields]+"_"+result[this.group_field]; - if (this.opration_fld[gen_key] == undefined){ - var map_val = {}; + var map_val = {}; + if (opration_fld[gen_key] == undefined){ map_val[this.operator_field] = result[this.operator_field]; if (this.operator.length > 1){ map_val[this.operator_field_one] = result[this.operator_field_one]; @@ -113,19 +112,18 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ if (this.group_field){ map_val[this.group_field] = (typeof result[this.group_field] == 'object')?result[this.group_field][1]:result[this.group_field]; } - this.opration_fld[gen_key] = map_val; } else { - map_val = this.opration_fld[gen_key]; + map_val = opration_fld[gen_key]; map_val[this.operator_field] += result[this.operator_field]; if (this.operator.length > 1){ map_val[this.operator_field_one] += result[this.operator_field_one]; } - this.opration_fld[gen_key] = map_val; } + opration_fld[gen_key] = map_val; }, this); var graph_data = []; - _.each(this.opration_fld, function (column_data) { + _.each(opration_fld, function (column_data) { graph_data.push(column_data); }); @@ -141,9 +139,7 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ var self = this; var view_chart = ''; var xystr = {}; - var xyname = {}; - var res = []; - this.group_list = []; + var group_list = []; var newkey = '', newkey_one; var COLOR_PALETTE = ['#cc99ff', '#ccccff', '#48D1CC', '#CFD784', '#8B7B8B', '#75507b', '#b0008c', '#ff0000', '#ff8e00', '#9000ff', '#0078ff', '#00ff00', '#e6ff00', '#ffff00', @@ -164,52 +160,40 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ newkey = "val"; } - if (jQuery.inArray(newkey, self.group_list) == -1){ - self.group_list.push(newkey); + if (jQuery.inArray(newkey, group_list) == -1){ + group_list.push(newkey); if(this.operator.length > 1){ newkey_one = "val1"; - self.group_list.push(newkey_one); + group_list.push(newkey_one); } } }, this); _.each(results, function (result) { - var xystring = result[self.chart_info_fields]; + var xystring = result[self.chart_info_fields], + xyname = {}; if (self.group_field && (self.operator.length <= 1)){ newkey = result[self.group_field].split(' ').join('_'); }else{ newkey = "val"; } if (xystr[xystring] == undefined){ - xyname = {}; xyname[self.chart_info_fields] = xystring; - _.each(self.group_list, function (group) { + _.each(group_list, function (group) { xyname[group] = 0.0001; }); - xyname[newkey] = result[self.operator_field]; - if (self.operator.length > 1){ - xyname[newkey_one] = result[self.operator_field_one]; - } - xystr[xystring] = xyname; - } - else{ - xyname = {}; + } else { xyname = xystr[xystring]; - xyname[newkey] = result[self.operator_field]; - if (self.operator.length > 1){ - xyname[newkey_one] = result[self.operator_field_one]; - } - xystr[xystring] = xyname; } - }); - - _.each(xystr, function (column_data) { - res.push(column_data); + xyname[newkey] = result[self.operator_field]; + if (self.operator.length > 1){ + xyname[newkey_one] = result[self.operator_field_one]; + } + xystr[xystring] = xyname; }); //for legend color - var grp_color = []; - _.each(self.group_list, function (group_legend, index) { + var grp_color = _.map(group_list, function (group_legend, index) { var legend = {color: COLOR_PALETTE[index]}; if (group_legend == "val"){ @@ -219,7 +203,7 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ }else{ legend['text'] = group_legend; } - grp_color.push(legend); + return legend; }); //for axis's value and title @@ -244,7 +228,7 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ var bar_chart = new dhtmlxchartChart({ view: view_chart, container: self.view_id+"-barchart", - value:"#"+self.group_list[0]+"#", + value:"#"+group_list[0]+"#", gradient: "3d", border: false, width: 1024, @@ -291,13 +275,13 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ } } }); - for (var m = 1; m Date: Fri, 17 Jun 2011 15:25:50 +0200 Subject: [PATCH 12/14] [IMP] move COLOR_PALETTE to toplevel constant, so it does not have to be recreated every time we go through the corresponding function bzr revid: xmo@openerp.com-20110617132550-b5add9fv7xuxjilz --- addons/base_graph/static/src/js/graph.js | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/addons/base_graph/static/src/js/graph.js b/addons/base_graph/static/src/js/graph.js index 7ebeffa78ab..c868807b30d 100644 --- a/addons/base_graph/static/src/js/graph.js +++ b/addons/base_graph/static/src/js/graph.js @@ -3,6 +3,15 @@ *---------------------------------------------------------*/ openerp.base_graph = function (openerp) { +var COLOR_PALETTE = [ + '#cc99ff', '#ccccff', '#48D1CC', '#CFD784', '#8B7B8B', '#75507b', + '#b0008c', '#ff0000', '#ff8e00', '#9000ff', '#0078ff', '#00ff00', + '#e6ff00', '#ffff00', '#905000', '#9b0000', '#840067', '#9abe00', + '#ffc900', '#510090', '#0000c9', '#009b00', '#75507b', '#3465a4', + '#73d216', '#c17d11', '#edd400', '#fcaf3e', '#ef2929', '#ff00c9', + '#ad7fa8', '#729fcf', '#8ae234', '#e9b96e', '#fce94f', '#f57900', + '#cc0000', '#d400a8']; + QWeb.add_template('/base_graph/static/src/xml/base_graph.xml'); openerp.base.views.add('graph', 'openerp.base_graph.GraphView'); openerp.base_graph.GraphView = openerp.base.Controller.extend({ @@ -142,11 +151,6 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ var group_list = []; var newkey = '', newkey_one; - var COLOR_PALETTE = ['#cc99ff', '#ccccff', '#48D1CC', '#CFD784', '#8B7B8B', '#75507b', '#b0008c', '#ff0000', '#ff8e00', '#9000ff', '#0078ff', '#00ff00', '#e6ff00', '#ffff00', - '#905000', '#9b0000', '#840067', '#9abe00', '#ffc900', '#510090', '#0000c9', '#009b00', - '#75507b', '#3465a4', '#73d216', '#c17d11', '#edd400', '#fcaf3e', '#ef2929', '#ff00c9', - '#ad7fa8', '#729fcf', '#8ae234', '#e9b96e', '#fce94f', '#f57900', '#cc0000', '#d400a8']; - if(self.group_field && (this.operator.length <= 1)){ view_chart = self.orientation == 'horizontal'? 'stackedBarH' : 'stackedBar'; }else{ From 872f7e163d0c1034ad2c62c75d071d0ba8215b37 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 17 Jun 2011 15:31:56 +0200 Subject: [PATCH 13/14] [IMP] invert tests and bail out early in order to reduce excessive nesting of functions bzr revid: xmo@openerp.com-20110617133156-g3fk9f386tdrdcje --- addons/base_graph/static/src/js/graph.js | 103 +++++++++++------------ 1 file changed, 50 insertions(+), 53 deletions(-) diff --git a/addons/base_graph/static/src/js/graph.js b/addons/base_graph/static/src/js/graph.js index c868807b30d..7670eca9c83 100644 --- a/addons/base_graph/static/src/js/graph.js +++ b/addons/base_graph/static/src/js/graph.js @@ -91,56 +91,52 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ schedule_chart: function(results) { this.$element.html(QWeb.render("GraphView", {"fields_view": this.fields_view, "chart": this.chart,'view_id': this.view_id})); - if (results.length){ - _.each(results, function (result) { - _.each(result, function (field_value, field_name) { - if (typeof field_value == 'object') { - result[field_name] = field_value[field_value.length - 1]; - } - if (typeof field_value == 'string'){ - var choices = this.all_fields[field_name]['selection']; - _.each(choices, function (choice) { - if(field_value == choice[0]){ - result[field_name] = choice; - } - }); - } - }, this); - }, this); - - var opration_fld = {}; - _.each(results, function (result) { - var gen_key = result[this.chart_info_fields]+"_"+result[this.group_field]; - var map_val = {}; - if (opration_fld[gen_key] == undefined){ - map_val[this.operator_field] = result[this.operator_field]; - if (this.operator.length > 1){ - map_val[this.operator_field_one] = result[this.operator_field_one]; - } - map_val[this.chart_info_fields] = result[this.chart_info_fields]; - if (this.group_field){ - map_val[this.group_field] = (typeof result[this.group_field] == 'object')?result[this.group_field][1]:result[this.group_field]; - } - } else { - map_val = opration_fld[gen_key]; - map_val[this.operator_field] += result[this.operator_field]; - if (this.operator.length > 1){ - map_val[this.operator_field_one] += result[this.operator_field_one]; - } + if (!results.length) { + return; + } + _.each(results, function (result) { + _.each(result, function (field_value, field_name) { + if (typeof field_value == 'object') { + result[field_name] = field_value[field_value.length - 1]; + } + if (typeof field_value == 'string') { + var choices = this.all_fields[field_name]['selection']; + _.each(choices, function (choice) { + if (field_value == choice[0]) { + result[field_name] = choice; + } + }); } - opration_fld[gen_key] = map_val; }, this); + }, this); - var graph_data = []; - _.each(opration_fld, function (column_data) { - graph_data.push(column_data); - }); - - if(this.chart == 'bar') { - return this.schedule_bar(graph_data); - } else if(this.chart == "pie") { - return this.schedule_pie(graph_data); + var opration_fld = {}; + _.each(results, function (result) { + var gen_key = result[this.chart_info_fields] + "_" + result[this.group_field]; + var map_val = {}; + if (opration_fld[gen_key] == undefined) { + map_val[this.operator_field] = result[this.operator_field]; + if (this.operator.length > 1) { + map_val[this.operator_field_one] = result[this.operator_field_one]; + } + map_val[this.chart_info_fields] = result[this.chart_info_fields]; + if (this.group_field) { + map_val[this.group_field] = (typeof result[this.group_field] == 'object') ? result[this.group_field][1] : result[this.group_field]; + } + } else { + map_val = opration_fld[gen_key]; + map_val[this.operator_field] += result[this.operator_field]; + if (this.operator.length > 1) { + map_val[this.operator_field_one] += result[this.operator_field_one]; + } } + opration_fld[gen_key] = map_val; + }, this); + + if (this.chart == 'bar') { + return this.schedule_bar(_.values(opration_fld)); + } else if (this.chart == "pie") { + return this.schedule_pie(_.values(opration_fld)); } }, @@ -158,18 +154,19 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ } _.each(results, function (result) { - if (self.group_field && (this.operator.length <= 1)){ + if (self.group_field && (this.operator.length <= 1)) { newkey = result[self.group_field].split(' ').join('_'); } else { newkey = "val"; } - if (jQuery.inArray(newkey, group_list) == -1){ - group_list.push(newkey); - if(this.operator.length > 1){ - newkey_one = "val1"; - group_list.push(newkey_one); - } + if (_.contains(group_list, newkey)) { + return; + } + group_list.push(newkey); + if (this.operator.length > 1) { + newkey_one = "val1"; + group_list.push(newkey_one); } }, this); From f9fad97b47a24a2f7ba719ab31d0759debe3be21 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Mon, 20 Jun 2011 08:38:48 +0200 Subject: [PATCH 14/14] [IMP] try to improve naming slightly bzr revid: xmo@openerp.com-20110620063848-qltrrdm88v1ww3cy --- addons/base_graph/static/src/js/graph.js | 80 ++++++++++++------------ 1 file changed, 40 insertions(+), 40 deletions(-) diff --git a/addons/base_graph/static/src/js/graph.js b/addons/base_graph/static/src/js/graph.js index 7670eca9c83..edf3809d6f1 100644 --- a/addons/base_graph/static/src/js/graph.js +++ b/addons/base_graph/static/src/js/graph.js @@ -110,40 +110,39 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ }, this); }, this); - var opration_fld = {}; + var graph_data = {}; _.each(results, function (result) { - var gen_key = result[this.chart_info_fields] + "_" + result[this.group_field]; - var map_val = {}; - if (opration_fld[gen_key] == undefined) { - map_val[this.operator_field] = result[this.operator_field]; + var column_key = result[this.chart_info_fields] + "_" + result[this.group_field]; + var column_descriptor = {}; + if (graph_data[column_key] == undefined) { + column_descriptor[this.operator_field] = result[this.operator_field]; if (this.operator.length > 1) { - map_val[this.operator_field_one] = result[this.operator_field_one]; + column_descriptor[this.operator_field_one] = result[this.operator_field_one]; } - map_val[this.chart_info_fields] = result[this.chart_info_fields]; + column_descriptor[this.chart_info_fields] = result[this.chart_info_fields]; if (this.group_field) { - map_val[this.group_field] = (typeof result[this.group_field] == 'object') ? result[this.group_field][1] : result[this.group_field]; + column_descriptor[this.group_field] = (typeof result[this.group_field] == 'object') ? result[this.group_field][1] : result[this.group_field]; } } else { - map_val = opration_fld[gen_key]; - map_val[this.operator_field] += result[this.operator_field]; + column_descriptor = graph_data[column_key]; + column_descriptor[this.operator_field] += result[this.operator_field]; if (this.operator.length > 1) { - map_val[this.operator_field_one] += result[this.operator_field_one]; + column_descriptor[this.operator_field_one] += result[this.operator_field_one]; } } - opration_fld[gen_key] = map_val; + graph_data[column_key] = column_descriptor; }, this); if (this.chart == 'bar') { - return this.schedule_bar(_.values(opration_fld)); + return this.schedule_bar(_.values(graph_data)); } else if (this.chart == "pie") { - return this.schedule_pie(_.values(opration_fld)); + return this.schedule_pie(_.values(graph_data)); } }, schedule_bar: function(results) { var self = this; var view_chart = ''; - var xystr = {}; var group_list = []; var newkey = '', newkey_one; @@ -170,27 +169,28 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ } }, this); + var abscissa_data = {}; _.each(results, function (result) { - var xystring = result[self.chart_info_fields], - xyname = {}; + var label = result[self.chart_info_fields], + section = {}; if (self.group_field && (self.operator.length <= 1)){ newkey = result[self.group_field].split(' ').join('_'); }else{ newkey = "val"; } - if (xystr[xystring] == undefined){ - xyname[self.chart_info_fields] = xystring; + if (abscissa_data[label] == undefined){ + section[self.chart_info_fields] = label; _.each(group_list, function (group) { - xyname[group] = 0.0001; + section[group] = 0.0001; }); } else { - xyname = xystr[xystring]; + section = abscissa_data[label]; } - xyname[newkey] = result[self.operator_field]; + section[newkey] = result[self.operator_field]; if (self.operator.length > 1){ - xyname[newkey_one] = result[self.operator_field_one]; + section[newkey_one] = result[self.operator_field_one]; } - xystr[xystring] = xyname; + abscissa_data[label] = section; }); //for legend color @@ -208,22 +208,22 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ }); //for axis's value and title - var temp_ax = { + var abscissa_description = { template: self.chart_info_fields, title: ""+self.x_title+"" }; - var oth_ax = { + var ordinate_description = { lines: true, title: ""+self.y_title+"" }; - var x_ax, y_ax; + var x_axis, y_axis; if (self.orientation == 'horizontal'){ - x_ax = oth_ax; - y_ax = temp_ax; + x_axis = ordinate_description; + y_axis = abscissa_description; }else{ - x_ax = temp_ax; - y_ax = oth_ax; + x_axis = abscissa_description; + y_axis = ordinate_description; } var bar_chart = new dhtmlxchartChart({ @@ -238,29 +238,29 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ origin:0, xAxis:{ template:function(obj){ - if(x_ax['template']){ - var val = obj[x_ax['template']]; + if(x_axis['template']){ + var val = obj[x_axis['template']]; val = (typeof val == 'object')?val[1]:(!val?'Undefined':val); return val; }else{ return obj; } }, - title:x_ax['title'], - lines:x_ax['lines'] + title:x_axis['title'], + lines:x_axis['lines'] }, yAxis:{ template:function(obj){ - if(y_ax['template']){ - var vals = obj[y_ax['template']]; + if(y_axis['template']){ + var vals = obj[y_axis['template']]; vals = (typeof vals == 'object')?vals[1]:(!vals?'Undefined':vals); return vals; }else{ return obj; } }, - title:y_ax['title'], - lines: y_ax['lines'] + title:y_axis['title'], + lines: y_axis['lines'] }, padding: { left: 75 @@ -282,7 +282,7 @@ openerp.base_graph.GraphView = openerp.base.Controller.extend({ color: grp_color[m]['color'] }); } - bar_chart.parse(_.values(xystr), "json"); + bar_chart.parse(_.values(abscissa_data), "json"); bar_chart.attachEvent("onItemClick", function(id) { self.open_list_view(bar_chart.get(id)); });