-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor GraferController and add remove layer functions #31
Conversation
we should start prefixing branches with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice structure and easy to read code, just a few comments, there are a few things that I omitted to not be too pedantic :)
src/grafer/GraferController.ts
Outdated
return generateId.previous++; | ||
}; | ||
|
||
generateId.previous = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couldn't the argument name here just be labelsData
?
labelsData.mappings | ||
); | ||
|
||
if (!hasColors) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remind me, we agreed to not allow for points to be added through the addLayer
methods, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels weird that this if statement is here, it somewhat obfuscates the code:
- if I manually call
mapPointsToNodes
I'd expect it to do as I say, it may not. - if I am reading code that calls
mapPointsToNodes
without 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
createGraphFromNodes
and 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
concatenateNodesFromLayers
and return thenodes
array - call this function
createGraphFromLayerNodes
and return theGraph
instance - call this function
initializeGraphFromLayerNodes
and 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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