Issue 6856073: Gadash Visualization
|
Created:
11 years, 5 months ago by oahmad Modified:
11 years, 4 months ago CC:
team-5-guys-project_googlegroups.com, pfrisella Base URL:
http://analytics-api-samples.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionGadash Visualization
Patch Set 1 #
Total comments: 22
Patch Set 2 : Gadash Visualization Corrected #
Total comments: 8
Patch Set 3 : Gadash-1.0. Added 5 chart wrappers with 6 default objects for customized visualization and 4 functi… #Patch Set 4 : Gadash-1.0. Added 5 chart wrappers with 6 default objects for customized visualization and 4 functi… #MessagesTotal messages: 11
the main comment is that all the stuff in defaultOnSuccess makes this library hard to extend. Please add it all to it's own methods and call it from the draw method. https://codereview.appspot.com/6856073/diff/1/src/reporting/javascript/ez-ga-... File src/reporting/javascript/ez-ga-dash/gadash-1.0.js (right): https://codereview.appspot.com/6856073/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:83: * @type {DataTable} please remove. see comments below. https://codereview.appspot.com/6856073/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:346: gadash.Chart.prototype.defaultOnSuccess = function(resp) { We want to keep this part of the library as simple as possible. Also all of this new logic looks like it's specific to "drawing" can you put it in it's own method and call it from the draw method Happy to discuss offline on why this is important https://codereview.appspot.com/6856073/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:348: gadash.dTable = dataTable; please don't use globals like this. see comment below https://codereview.appspot.com/6856073/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:350: var datePattern = /^(20)\d{2}(0[1-9]|1[012])(0[1-9]|[12][0-9]|3[01])$/; could you add a comment to the code as to why you are using such a complicated reg ex? https://codereview.appspot.com/6856073/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:355: var dateFormatter = new google.visualization.DateFormat({ pattern: 'MMM d' }); remove spaces around {} https://codereview.appspot.com/6856073/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:366: gadash.util.convertToMMMd = function() { this should accept a dataTable and return a modified dataTable. It should not be accessing a global value https://codereview.appspot.com/6856073/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:386: var datePattern = /^(20)\d{2}(0[1-9]|1[012])(0[1-9]|[12][0-9]|3[01])$/; This is crazy complex. can you add a comment on what this does? https://codereview.appspot.com/6856073/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:389: switch (month) { This runs in O(n) time. Pleas use an object so it runs in O(1) i.e. var monthMap = { '01': 'Jan', '02': 'Feb', ... }; var monthStr = monthMap[month]; https://codereview.appspot.com/6856073/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:719: 'chartOptions': { 2 space indent for every indent. See style guide: http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone... Here and in all objects below https://codereview.appspot.com/6856073/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:881: remove this extra line. 2 space indent 4 space indent for lines that wrap, like where you start the line with a . Here and in all the wrappers below
Sign in to reply to this message.
The Gadash Visualization issue (#6856073) has been corrected as indicated. Laurent https://codereview.appspot.com/6856073/diff/1/src/reporting/javascript/ez-ga-... File src/reporting/javascript/ez-ga-dash/gadash-1.0.js (right): https://codereview.appspot.com/6856073/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:83: * @type {DataTable} On 2012/11/21 00:36:19, nm wrote: > please remove. see comments below. Done. https://codereview.appspot.com/6856073/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:346: gadash.Chart.prototype.defaultOnSuccess = function(resp) { On 2012/11/21 00:36:19, nm wrote: > We want to keep this part of the library as simple as possible. > > Also all of this new logic looks like it's specific to "drawing" > > can you put it in it's own method > and call it from the draw method > > Happy to discuss offline on why this is important Done. https://codereview.appspot.com/6856073/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:348: gadash.dTable = dataTable; On 2012/11/21 00:36:19, nm wrote: > please don't use globals like this. see comment below Done. https://codereview.appspot.com/6856073/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:350: var datePattern = /^(20)\d{2}(0[1-9]|1[012])(0[1-9]|[12][0-9]|3[01])$/; On 2012/11/21 00:36:19, nm wrote: > could you add a comment to the code as to why you are using such a complicated > reg ex? It is to make sure we are dealing with string represnting a date https://codereview.appspot.com/6856073/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:350: var datePattern = /^(20)\d{2}(0[1-9]|1[012])(0[1-9]|[12][0-9]|3[01])$/; On 2012/11/21 00:36:19, nm wrote: > could you add a comment to the code as to why you are using such a complicated > reg ex? Done. https://codereview.appspot.com/6856073/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:355: var dateFormatter = new google.visualization.DateFormat({ pattern: 'MMM d' }); On 2012/11/21 00:36:19, nm wrote: > remove spaces around {} Done. https://codereview.appspot.com/6856073/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:366: gadash.util.convertToMMMd = function() { On 2012/11/21 00:36:19, nm wrote: > this should accept a dataTable and return a modified dataTable. > > It should not be accessing a global value Done. https://codereview.appspot.com/6856073/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:386: var datePattern = /^(20)\d{2}(0[1-9]|1[012])(0[1-9]|[12][0-9]|3[01])$/; On 2012/11/21 00:36:19, nm wrote: > This is crazy complex. > > can you add a comment on what this does? Done. https://codereview.appspot.com/6856073/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:389: switch (month) { On 2012/11/21 00:36:19, nm wrote: > This runs in O(n) time. Pleas use an object so it runs in O(1) i.e. > > var monthMap = { > '01': 'Jan', > '02': 'Feb', > ... > }; > > var monthStr = monthMap[month]; Done. https://codereview.appspot.com/6856073/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:389: switch (month) { On 2012/11/21 00:36:19, nm wrote: > This runs in O(n) time. Pleas use an object so it runs in O(1) i.e. > > var monthMap = { > '01': 'Jan', > '02': 'Feb', > ... > }; > > var monthStr = monthMap[month]; Done. https://codereview.appspot.com/6856073/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:719: 'chartOptions': { On 2012/11/21 00:36:19, nm wrote: > 2 space indent for every indent. See style guide: > http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone... > > Here and in all objects below Done. https://codereview.appspot.com/6856073/diff/1/src/reporting/javascript/ez-ga-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:881: On 2012/11/21 00:36:19, nm wrote: > remove this extra line. > > 2 space indent > 4 space indent for lines that wrap, like where you start the line with a . > > Here and in all the wrappers below Done.
Sign in to reply to this message.
LGTM... Jeetendra, have a look at this. once complete, send me the email address of the person who can upload these specific changes to the repository
Sign in to reply to this message.
email: oahmad@uci.edu On Mon, Nov 26, 2012 at 1:21 PM, <nm@google.com> wrote: > LGTM... > > Jeetendra, have a look at this. > > once complete, send me the email address of the person who can upload > these specific changes to the repository > > > https://codereview.appspot.**com/6856073/<https://codereview.appspot.com/6856... > > -- > You received this message because you are subscribed to the Google Groups > "Team-5-Guys-Project" group. > To post to this group, send email to team-5-guys-project@** > googlegroups.com <team-5-guys-project@googlegroups.com>. > To unsubscribe from this group, send email to team-5-guys-project+** > unsubscribe@googlegroups.com<team-5-guys-project%2Bunsubscribe@googlegroups.com> > . > For more options, visit this group at http://groups.google.com/** > group/team-5-guys-project?hl=**en<http://groups.google.com/group/team-5-guys-project?hl=en> > . > >
Sign in to reply to this message.
LGTM. Made a couple of minor comments. Once you've addressed those, you can submit it. As a side note, it looks like the file is getting too long. So we might want to consider splitting the wrappers in a separate file. But we can discuss that later. https://codereview.appspot.com/6856073/diff/2001/src/reporting/javascript/ez-... File src/reporting/javascript/ez-ga-dash/gadash-1.0.js (right): https://codereview.appspot.com/6856073/diff/2001/src/reporting/javascript/ez-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:490: * Creates a date format 'MMM d', which can be call by chart wrappers call -> called https://codereview.appspot.com/6856073/diff/2001/src/reporting/javascript/ez-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:502: * in a String format composed of 3 letters representing the month followed instead of describing it in words, you could just say "... changes its values to a date string of the form 'MMM dd' (e.g., Oct 23)." https://codereview.appspot.com/6856073/diff/2001/src/reporting/javascript/ez-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:524: * composed of 3 letters representing the month followed by a space and Same here, please provide an example. https://codereview.appspot.com/6856073/diff/2001/src/reporting/javascript/ez-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:732: * This object is used by all five chart wrappers. the number 'five' may change in the future. So consider making the comment more general. e.g., just say "used by the chart wrappers".
Sign in to reply to this message.
updated https://codereview.appspot.com/6856073/diff/2001/src/reporting/javascript/ez-... File src/reporting/javascript/ez-ga-dash/gadash-1.0.js (right): https://codereview.appspot.com/6856073/diff/2001/src/reporting/javascript/ez-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:490: * Creates a date format 'MMM d', which can be call by chart wrappers On 2012/11/26 22:38:06, jsoneja wrote: > call -> called Done. https://codereview.appspot.com/6856073/diff/2001/src/reporting/javascript/ez-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:502: * in a String format composed of 3 letters representing the month followed On 2012/11/26 22:38:06, jsoneja wrote: > instead of describing it in words, you could just say "... changes its values to > a date string of the form 'MMM dd' (e.g., Oct 23)." Done. https://codereview.appspot.com/6856073/diff/2001/src/reporting/javascript/ez-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:524: * composed of 3 letters representing the month followed by a space and On 2012/11/26 22:38:06, jsoneja wrote: > Same here, please provide an example. Done. https://codereview.appspot.com/6856073/diff/2001/src/reporting/javascript/ez-... src/reporting/javascript/ez-ga-dash/gadash-1.0.js:732: * This object is used by all five chart wrappers. On 2012/11/26 22:38:06, jsoneja wrote: > the number 'five' may change in the future. So consider making the comment more > general. e.g., just say "used by the chart wrappers". Done.
Sign in to reply to this message.
LGTM. I think Nick is still out. Pete, can you give oahmad@uci.edu submit access for the repo? Thanks, Jeetendra.
Sign in to reply to this message.
|
