Add data group class to Graph3d #2804
Conversation
lib/graph3d/DataGroup.js
Outdated
| var Range = require('./Range'); | ||
|
|
||
|
|
||
| function DataGroup() { |
There was a problem hiding this comment.
What about adding a comment with @constructor annotation and a small explanation similar to the one in your pull request?
There was a problem hiding this comment.
For all matters related to jsdoc annotations, I am assuming that http://usejsdoc.org is leading.
There was a problem hiding this comment.
I totally agree that header commenting should be reviewed as well. However, I would really like to see this explicitly stated in the programming guidelines. It's something that developers should be aware of and should adhere to, throughout the entire codebase of vis.js.
lib/graph3d/DataGroup.js
Outdated
| * the Graph. | ||
| * @param {Number} style Style Number | ||
| */ | ||
| DataGroup.prototype.dataInitialize = function(graph3d, rawData, style) { |
There was a problem hiding this comment.
Better use active voice with verb first for your function calls i.e. initializeData ?
There was a problem hiding this comment.
Isn't here the graph3d @param missing or is this optional?
lib/graph3d/DataGroup.js
Outdated
|
|
||
|
|
||
| function DataGroup() { | ||
| this.dataTable = null; // The original data table |
There was a problem hiding this comment.
Mention the consequences for this assignment in the constructor comment. For example, do we need to call initializeData? What happens when we don't call initializeData?
lib/graph3d/DataGroup.js
Outdated
| // calculate minimums and maximums | ||
| var NUMSTEPS = 5; | ||
|
|
||
| var xRange = this.getColumnRange(data, this.colX); |
There was a problem hiding this comment.
Is there any way we can shorten the following lines? Looks quite similar to me. Only the axis is different.
There was a problem hiding this comment.
Tough one, but I'll do my best. #2853 will take away a lot of the hurt here eventually.
There was a problem hiding this comment.
See new methods DataGroup.prototype._collectRangeSettings() and DataGroup.prototype._initializeRange()
lib/graph3d/DataGroup.js
Outdated
| }; | ||
|
|
||
|
|
||
| DataGroup.prototype.getDistinctValues = function(column) { |
There was a problem hiding this comment.
Sure no problem. But then also make it a requirement in the guidelines.
lib/graph3d/DataGroup.js
Outdated
|
|
||
|
|
||
| /** | ||
| * NOTE: This method is used nowhere. |
lib/graph3d/DataGroup.js
Outdated
| DataGroup.prototype.reload = function() { | ||
| if (this.dataTable) { | ||
| this.setData(this.dataTable); | ||
| } |
lib/graph3d/Graph3d.js
Outdated
| @@ -392,8 +327,10 @@ Graph3d.prototype._checkValueField = function (data) { | |||
| return; // No need to check further | |||
| } | |||
|
|
|||
| var colValue = this.colValue; | |||
There was a problem hiding this comment.
I would inline the value when the overhead of retrieving the value i.e. this.colValue is cheap. For expensive function calls etc. it would be indeed better to assign it once. Personal taste here.
lib/graph3d/Graph3d.js
Outdated
| if (defaultMin !== undefined) { | ||
| range.min = defaultMin; | ||
| } | ||
| Graph3d.prototype._dataInitialize = function(rawData, style) { |
lib/graph3d/Graph3d.js
Outdated
| } | ||
| } | ||
| var dataX = this.getPresentXValues(data); | ||
| var dataY = this.getPresentYValues(data); | ||
|
|
||
| var sortNumber = function (a, b) { |
There was a problem hiding this comment.
This is used in so many places. Maybe we should add it to a util class.
There was a problem hiding this comment.
Not sure what you mean here. This is the only place in graph3d where getPresent[XY]Values() are called.
Or perhaps you mean the sort function? That does occur in several places.
There was a problem hiding this comment.
Ah, I see what you mean. getPresentXValues() and getPresentYValues() are almost the same.
On top of that, my shiny new method DataGroup.getDistinctValues() does almost the same thing. I'll DRY the code.
There was a problem hiding this comment.
I was referring to the sort function. I'm fine with the getPresentXValues calls.
There was a problem hiding this comment.
I can now not unsee the similarity between getPresent[X|Y]Values() and DataGroup.getDistinctValues(). I will need to DRY them!
There was a problem hiding this comment.
The previous DRY takes care of some sort functions. All the regular sorts have now been reduced to a single instance in DataGroup.prototype.getDistinctValues().
For the remaining sorts:
Graph3d.prototype._redrawBar()- Distinctly different sorting order, can't combine.Graph3d.prototype.calcTranslations()- Distinctly different sorting order, can't combine.Filter()(constructor) - sort is now redundant, done ingetDistinctValues(). And the data values are never strings, so it was silly anyway.
In addition:
Graph3d.prototype._calcTranslations()- optional parametersortnever used, removed.
|
I pushed the changes till now. All the review points are addressed except the last - I want to think a bit about how to handle that. |
|
@yotamberk Thanks! I'm totally going to bring |
|
Republishing this PR to circumvent unapproved review. |
This is the next step in the implementation of multiple graphs in a view for
Graph3D.Added class
DataGroup, which is a container for all data of one specific graph.In this implementation, only one instance is used internally. Eventually, multiple instances will be possible, one per graph to display. The interface of
Graph3Dis unchanged.