Skip to content

Instantly share code, notes, and snippets.

@DavidBruant

DavidBruant/review.md

Last active August 29, 2015 13:55
Show Gist options
  • Save DavidBruant/8767087 to your computer and use it in GitHub Desktop.
Save DavidBruant/8767087 to your computer and use it in GitHub Desktop.
Download ZIP
Sigma.js Review
Raw
review.md

Reviewing this verion

Overall comments

ids

Why are ids strings (instead of numbers)? More importantly, why are ids needed at all? In JavaScript, objects can be unambiguously identified with their reference.

var o = {};
var o2 = {};
var o3 = o;

o !== o2
o === o3
o2 !== o3

By spec, the object reference is guaranteed to be unique (so you wouldn't need to check for uniqueness or generate unique ids as you often do) and unforgeable (so people aren't tempted to be smart about an id generation scheme)

Searching things by id always incurs a cost because of the lookup and this lookup is rather unnecessary. ids are useful only when interacting with contexts where the memory isn't shared (different machine, different process, database, force in a WebWorker ;-) ). And even then, they could be an implementation detail that can be hidden.

WeakMaps (or a Map or a Set if you need to enumerate the values) can be useful to associate data to an object without putting it as a property. They'll help with memory leak without having to do too much about it. Polyfills exist. They have all sorts of downsides, but they're polyfills, so the native version will be used if available which is going to be more and more common.

I admit that removing the use of ids will break APIs... Maybe for the best?

API

In the graph API, nodes and edges ought to be getters and not methods.

Important

if one middleware throws, everything is cancelled. Is it intended? Add a comment if so.

(cosmetic : use forEach to traverse an array; second argument is index)

sigma.classes.graph.js

  • graph.addMethod

Why not create graphs from a constructor and just let people access to Graph.prototype? oh... that's related to having access to per-instance private? Damned JavaScript!

  • graph.attach

Looks a lot like an event system

Set configurable: false, otherwise, if the node isn't cloned and already provided an id, it will remain to true... or get rid of ids altogether ;-) Same bug for edges.

Cosmetic

sigma.core.js

Use Array.isArray (probably repeated in a lot of places)

If conf.renderer === null, 'container' in o throws an error. Beware of typeof x === 'object' in general.

Maybe schedule it via rAF

No need to convert to string, the JS engine already does it. See step 6 of http://es5.github.io/#x11.2.1

Purely philosophical question : why a quadtree and not a binary tree?

for-in will be probably faster and more elegant

sigma.utils.js

Maybe you'll be interested in the to-be-standard Object.assign and add it as polyfill https://github.com/paulmillr/es6-shim/blob/1942309cb40da9e87ab8565ec60b19b9c5591c64/es6-shim.js#L651-L657

If useful, take a look at performance.now() for sub-millisecond precision.

sigma.misc.bindEvents.js

Maybe add a polyfill for Math.hypot

=> this.captors.forEach(bindCaptor)

sigma.classes.camera.js

Number.isFinite

use forEach

sigma.classes.dispatcher.js

=> forEach

Maybe belongs in util.

sigma.classes.graph.js

Will throw if one property is non-configurable. Maybe belongs in util.

Initialize only if this.settings('clone')

@jacomyal
Copy link

jacomyal commented Feb 3, 2014

First, I've been rushing to release in time, and since it's a complex code, having someone making the effort to read and find some trouble spots is really helpful. Thanks for that :)

Overall comments

ids

I kept ids for two main reasons:

  • First, sigma is a rendering engine, but I hope to see it used for graph manipulation applications. And if your app can read and update graph data, you will probably need a clean way to identify your nodes and edges. It does not make them mandatory for every uses, but this is what I had in mind when designing the graph model.
  • Also, I wanted to keep clearly delimited and flat indexes, to keep them easy to maintain and to understand, and even eventually to export. It has indeed a cost, but I am pretty sure it is quite strong and well tested like this.

I admit the edge ids especially are kind of annoying, but it is easy to spawn them from parsers, like it happens in the GEXF parser.

But I keep this in mind for a later version. I prefer not to break the API so soon, but if there is later a nice graph model with references-based indexes that is well tested and faster, I will integrate it.

API

I wanted a simple and intuitive way to retrieve data from the graph, so the nodes and edges methods return a copy of the related arrays if called without arguments, the related object if an ID is specified, and the array of the related objects if an array of ids is given. But since they are almost always called without arguments, this is indeed open to criticism...

Important

Middlewares

Oops indeed... Middlewares do require some more work, especially on errors management.

sigma.classes.graph

  • graph.addMethod
  • graph.attach

It looks indeed like an event system. But since the target is also the dispatcher, I prefered to design it in a more ad-hoc way. Also, since methods can be added, I did not see a good way to use the prototype while keeping these methods catchable... There are a bit more explanation on the Graph wiki page.

Indeed, my bad.

Cosmetic

Your notes that are not commented here are those I just have to implement in the sources.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment