Refactor GraferController and add remove layer functions #31
Conversation
|
we should start prefixing branches with |
src/grafer/GraferController.ts
Outdated
| return generateId.previous++; | ||
| }; | ||
|
|
||
| generateId.previous = 0; |
There was a problem hiding this comment.
In the case that we have several GraferController instances in an app, they will continue to add to this number. Imagine the case where a scientist is exploring several model at the same time, going back and forth and creating views, filters, etc. All of that creates new IDs, even though some of the points generated will be the same and the IDs will not be shared between GraferController instances.
It is an edge case, but eventually the user could overflow this number, can we implement this as a class method instead?
|
|
||
| this.loadColors(data); | ||
| this.loadPoints(data, pointsRadiusMapping); | ||
| this.loadLayers(data, pointsRadiusMapping); |
There was a problem hiding this comment.
Definitely like the code organization here, thanks!
src/grafer/GraferController.ts
Outdated
| } | ||
| } | ||
|
|
||
| private addLabel(layersData: GraferLabelsData, hasColors: boolean): any { |
There was a problem hiding this comment.
couldn't the argument name here just be labelsData?
| labelsData.mappings | ||
| ); | ||
|
|
||
| if (!hasColors) { |
There was a problem hiding this comment.
I feel like this could be modified slightly to also address #20
No need to do it in this PR, just leaving a comment for future reference
(would need to be modified in addNode and addEdge as well
src/grafer/GraferController.ts
Outdated
| this._viewport.graph = Graph.fromNodesArray(context, nodes, pointsRadiusMapping); | ||
| this._viewport.graph.picking = new PickingManager(this._viewport.context, this._viewport.mouseHandler); | ||
| } | ||
| this.mapPointsToNodes(data, pointsRadiusMapping); |
There was a problem hiding this comment.
Remind me, we agreed to not allow for points to be added through the addLayer methods, right?
There was a problem hiding this comment.
I don't believe we ever had that discussion...
From an implementation standpoint, it looks like it would require some more work to allow points to be added through the addLayer method because mapPointsToNodes is designed to take in another data format
From a user/dev standpoint, at first glance allowing points to be added through addLayer seems like a good thing to have
There was a problem hiding this comment.
In terms of priorities adding new points won't be needed in the short term (at least two months).
src/grafer/GraferController.ts
Outdated
| const mappings = Object.assign({}, pointsRadiusMapping, data.points.mappings); | ||
| this._viewport.graph = new Graph(this._viewport.context, data.points.data, mappings); | ||
| private mapPointsToNodes(data: GraferControllerData, pointsRadiusMapping: { radius: (entry: any) => number; }): void { | ||
| if (!Boolean(this._viewport.graph)) { |
There was a problem hiding this comment.
It feels weird that this if statement is here, it somewhat obfuscates the code:
- if I manually call
mapPointsToNodesI'd expect it to do as I say, it may not. - if I am reading code that calls
mapPointsToNodeswithout checking if it can be called first I would be confused
Take this if statement outside of the function so the function performs its intended operation no matter what.
There was a problem hiding this comment.
Ok I will make this change. Just for context though, the original code had this if statement where const hasPoints = Boolean(this._viewport.graph); so leaving this if statement in the fn made sense to me
There was a problem hiding this comment.
yup, makes sense, it just sticks out to me now.
src/grafer/GraferController.ts
Outdated
| } | ||
| nodes.push(layers[i].nodes.data); | ||
| } | ||
| this._viewport.graph = Graph.fromNodesArray(this.context, nodes, pointsRadiusMapping); |
There was a problem hiding this comment.
This is an interesting architecture problem, I would expect this function to perform its operations and return the result rather than modifying an internal variable. Two solutions are possible:
- change the name of the function to something along the lines of
createGraphFromNodesand return the newly created graph, the calling code is responsible for assigning the graph to the viewport and creating a picking manager - refactor the code so there is a way to map points to nodes without creating a graph
for now I think that renaming the function is good enough but I'll let that decision to you.
There was a problem hiding this comment.
Renaming the function works for me as well, going with that change :)
src/grafer/GraferController.ts
Outdated
| } | ||
| } | ||
|
|
||
| private addLabel(layersData: GraferLabelsData, hasColors: boolean): any { |
There was a problem hiding this comment.
shouldn't this function be called addLabels since it adds multiple labels at once?
src/grafer/GraferController.ts
Outdated
| if (!nodes && !hasPoints) { | ||
| throw 'Cannot load an edge-only layer in a graph without points!'; | ||
| } | ||
| private addEdge(edgesData: GraferEdgesData, nodes: any, hasColors: boolean): any { |
There was a problem hiding this comment.
addEdges since it adds multiple edges at once
src/grafer/GraferController.ts
Outdated
| } | ||
| return value; | ||
| }; | ||
| private addNode(nodesData: GraferNodesData, hasColors: boolean): any { |
There was a problem hiding this comment.
addNodes since it adds multiple nodes at once
src/grafer/GraferController.ts
Outdated
| private mapPointsToNodes(data: GraferControllerData, pointsRadiusMapping: { radius: (entry: any) => number; }): void { | ||
| const nodes = []; | ||
| const layers = data.layers; | ||
| for (let i = 0, n = layers.length; i < n; ++i) { | ||
| const data = layers[i].nodes?.data ?? layers[i].labels?.data; | ||
| for (let ii = 0, nn = data.length; ii < nn; ++ii) { | ||
| (data[ii] as any).point = this.generateId(); | ||
| } | ||
| nodes.push(layers[i].nodes.data); | ||
| } | ||
| this._viewport.graph = Graph.createGraphFromNodes(this.context, nodes, pointsRadiusMapping); | ||
| this._viewport.graph.picking = new PickingManager(this._viewport.context, this._viewport.mouseHandler); | ||
| } |
There was a problem hiding this comment.
I'm still not too happy about the semantics of this function, it is named mapPointsToNodes, doesn't return anything and has the following non-explicit side effects:
- it creates a graph
- assigns the graph to the viewport
- creates a picking manager
- assigns the picking manager to the graph
Side effects should always be explicit in a function's signature, by returning void it is obvious that the function has side effects, but not which effects based on its name. You could:
- call this function
concatenateNodesFromLayersand return thenodesarray - call this function
createGraphFromLayerNodesand return theGraphinstance - call this function
initializeGraphFromLayerNodesand leave it the way it is
sorry to be pedantic, being explicit about what a function does makes it so when code is read by someone unfamiliar with it, they don't have to look at function implementations to figure out what is going on.
There was a problem hiding this comment.
no totally makes sense to me, thank you for making this suggestion. I think it greatly improves not only the readability of the code, but if at some point we want to multithread stuff in this class then we need to remove side effects from the expensive methods anyway. I have made the change to use concatenateNodesFromLayers and return the nodes array
src/grafer/GraferController.ts
Outdated
| if (!Boolean(this._viewport.graph)) { | ||
| this.mapPointsToNodes(data, pointsRadiusMapping); |
There was a problem hiding this comment.
this illustrates the semantics I mentioned above perfectly, someone new to the code would read the above as:
if there is not a graph, map points to nodes it seems unrelated, to know what that means they would have to look at the implementation of the function.
If instead, we were to pick from one of the options above, here's how the code would look and read:
// call this function concatenateNodesFromLayers and return the nodes array
if (!Boolean(this._viewport.graph)) {
const nodes = concatenateNodesFromLayers(data);
this._viewport.graph = Graph.createGraphFromNodes(this.context, nodes, pointsRadiusMapping);
this._viewport.graph.picking = new PickingManager(this._viewport.context, this._viewport.mouseHandler);
}
^ it would read if there is no graph, concatenate nodes from layers, create graph from nodes and assign a new picking manager to it
// call this function createGraphFromLayerNodes and return the Graph instance
if (!Boolean(this._viewport.graph)) {
this._viewport.graph = createGraphFromLayerNodes(data, pointsRadiusMapping);
this._viewport.graph.picking = new PickingManager(this._viewport.context, this._viewport.mouseHandler);
}
^ it would read if there is no graph, create graph from layer nodes and assign a new picking manager to it
// call this function initializeGraphFromLayerNodes and leave it the way it is
if (!Boolean(this._viewport.graph)) {
initializeGraphFromLayerNodes(data, pointsRadiusMapping);
}
^ it would read if there is no graph, initialize graph from layer nodes
Semantics are not something I'm good at, but we should all strive to write easy to read code.
src/grafer/GraferController.ts
Outdated
| @@ -57,9 +57,12 @@ export class GraferController extends EventEmitter { | |||
| return this.viewport.context; | |||
| } | |||
|
|
|||
| generateIdPrev: number; | |||
There was a problem hiding this comment.
May want to be explicit whether this is intended to be public/private
src/grafer/GraferController.ts
Outdated
| } | ||
| } | ||
|
|
||
| public addLayer(layer: GraferLayerData, name: string, hasColors: boolean): void { |
There was a problem hiding this comment.
Is hasColors necessary in this public function? From a code organization perspective this makes sense, but as a user I'd expect the object to keep track of whether or not I've passed in colours on construction.
|
A few small comments, but otherwise looks good. Thanks Manfred! |
…roller-refactor-2
Replaces PR #25