Make Quadtree's Box and Point private?

[bttmly]

@davegw

I'm wondering if we can refactor Quadtree to avoid exposing Box and Point to users. For example, maybe the Quadtree constructor would look like this

function Quadtree(minX, minY, maxX, maxY) {
  this.box = new Box(minX, minY, maxX, maxY);
}

This ensures that the box property of any Quadtree is actually a Box instance. Further, by keeping Box private, we don't have to worry about documenting it.

for Quadtree.prototype.insert, something like this:

Quadtree.prototype.insert = function(x, y) {
  var point = new Point(x, y);
  // rest of code....
};

Haven't had a chance to dive deep into the implementation but I'm wondering if you think this would be 1.) desirable and 2.) doable

[davegw]

Agreed it would be nice to not expose Box and Point to the user meaning do not export the Box and Point functions.

At first glance, the best way I'm seeing to implement this is by adding the Box and Point constructor functions and methods as Quadtree prototype methods. Ex:

function Quadtree(minX, minY, maxX, maxY) {
  this.box = Quadtree.prototype.Box(minX, minY, maxX, maxY);
  // rest of code...
}

Quadtree.prototype.Box = function(minX, minY, maxX, maxY) {// Box constructor}
Quadtree.prototype.Box.prototype.contains = function(...) {...}
// rest of Box prototype methods

The downside of this implementation is it exposes Box and Point as Quadtree methods to the user and clutters the Quadtree prototype. Thoughts?

[bttmly]

Quadtree will have Box and Point constructors in closure scope, so I don't think they need to be added to the prototype.

[davegw]

Good point, I'll give it a closer look later today.

[bttmly]

Here just have a look at what I came up with (actually required more refactoring of tests than actual code).
https://github.com/nickb1080/dslib/blob/quadtree-refactor/dataStructures/quadTree.js
https://github.com/nickb1080/dslib/blob/quadtree-refactor/tests/quadTreeSpec.js

[bttmly]

I employed some argument shifting since I wasn't sure internally where Box and Point instances are being used recursively.

[davegw]

Looks good. The argument shifting allows the code to be more extendable and has no functional issues, but I think it is confusing to someone not familiar with the codebase.

It would be a good idea to add some comments here. I can also add the correct recursive calls, which would allow us to ditch the argument shifting.

[bttmly]

I'd prefer commenting over changing all the recursive calls. For one, we have to re-instantiate a Box on each recursive .retrieve() call, and also we'd have something like:

// ... other code ..
nearestPoints = quadtree.retrieve(searchBox.minX, searchBox.minY, searchBox.maxX, searchBox.maxY );

[davegw]

Yeah realize the method calls get messy. Either way works, but fine with commenting.