Skip to content
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

remove classtool from Address, VersionedData, EncodedData #114

Closed
wants to merge 1 commit into from
Closed

remove classtool from Address, VersionedData, EncodedData #114

wants to merge 1 commit into from

Conversation

ryanxcharles
Copy link
Contributor

Classtool provides two particularly useful features: 1) allowing to overload dependencies in a class, 2) allowing a default instance of a class. However, it is unfriendly to new developers. One solution to this is to make classtool itself a little bit friendlier, as in @gasteve's soop. Another solution, explored here, is to remove classtool and rely on more traditional solutions to the same problems that do not require any dependencies at all.

In this PR I have removed classtool from Address, VersionedData, and EncodedData, which are all related classes. I have also updated the code to fix all tests. I have not removed classtool from other classes. The purpose of this PR is to demonstrate the feasibility of removing classtool and spark a discussion about removing classtool in the interests of making bitcore more friendly to new developers. This PR should not actually be pulled in unless we can all agree that removing classtool is the best option, and in that case the entire codebase should be updated to remove the classtool dependency.

For the record, here is an example of code that overloads dependencies of a class in a way that does not require classtool and is familiar to all javascript developers. The dependency being overloaded here is the database, which is replaced with a mockup database for the purpose of a test:

var sinon = require('sinon')
var assert = require('assert')

var Database = function() {
}

Database.prototype.query = function() {
  return 'herp'
}

var Person = function() {
  this.database = new Database()
}

Person.prototype.getName = function() {
  return this.database.query()
}

it("returns the return value from the original function", function () {
  var person = new Person()
  var database = {};

  database.query = function() {
    return 'derp';
  };
  person.database = database;

  assert.equal('derp', person.getName())
})

it("returns the return value from the original function", function () {
  var person = new Person()
  assert.equal('herp', person.getName())
})

Hat tip to @kyledrake for suggesting this option and writing some proof-of-concept code.

...and update tests and other files so that all tests pass. The idea here is
that classtool is a burden on new developers, and there are other ways to solve
the same problems that classtool solves in ways that are more familiar to most
developers. By removing classtool, we can make bitcore more appealing to more
developers.
@gasteve
Copy link
Member

gasteve commented Mar 6, 2014

So, you would propose that every instance of a Person have a reference to a database? That's very space inefficient for a server that might have many thousands of Persons.

In terms of familiarity, it's more about the users of the library... we want a user to be able to do this:
var Person = require('./Person');

And more specifically to bitcore, this:

var Address = require('bitcore').Address; // loads a namespace with the entire bitcore library
or
var Address = require('bitcore/Address'); // loads just Address

Also, one thing that we need to do in some circumstances is to have two different versions of a class loaded concurrently in memory, but have them bound to the environment in different ways:

var PersonA = load('./Person', {database: db1});
var PersonB = load('./Person', {database: db2});

All instances of PersonA are bound to db1, while all instances of PersonB are bound to db2 (and it doesn't require every instance to waste space with a pointer to a database).

@ryanxcharles ryanxcharles mentioned this pull request Mar 6, 2014
@maraoz
Copy link
Contributor

maraoz commented Mar 10, 2014

solved with soop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants