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

Weird method naming for pushing data #2

Open
JanHalozan opened this issue Dec 1, 2015 · 4 comments
Open

Weird method naming for pushing data #2

JanHalozan opened this issue Dec 1, 2015 · 4 comments

Comments

@JanHalozan
Copy link

I find it weird why do there have to be two methods for pushing data. There is push for pushing a single KPI and insertAll for pushing an array. I think it would be better if there was only push. And of this is not possible perhaps unify the naming to something like insert and insertAll.

@colxi
Copy link
Contributor

colxi commented Aug 1, 2017

I find it weird too.
I use this code to unify both methods:

Databox.prototype.push = function (kpi, cb) {
  kpi = (kpi instanceof Array) ? kpi : [kpi];  //<-- my modification (force array when needed)
  return this._pushJSONRequest('/',
    kpi.map(this._processKPI), //<-- don't need to force array anymore here
    cb
  );
};

Simply by adding a line of code in the beggining of the original method ( kpi = (kpi instanceof Array) ? kpi : [kpi];) the .push() can handle single and multiple requests ( and the insertAll() becomes totally useless )

@colxi
Copy link
Contributor

colxi commented Aug 1, 2017

Deprecate .insertAll(), providing backwards legacy compatibillity .insertAll()

Databox.prototype.insertAll = function (kpis, cb) { return this.push(kpis,cb); };

;)

@otobrglez
Copy link
Contributor

We used push and insertAll naming convention so that "all" Databox libraries were all "similar". Behind the scenes push wraps what you give it and invokes _pushJSONRequest, insertAll just passes value directly.

I would also avoid detecting "type" with instanceof. What if you sent something that is not Array, some other (custom) collection type for example ArrayBuffer?

@colxi
Copy link
Contributor

colxi commented Aug 1, 2017

Original code of .push() VS insertAll():

Databox.prototype.push = function (kpi, cb) {
  return this._pushJSONRequest('/',
    [kpi].map(this._processKPI),
    cb
  );
};

Databox.prototype.insertAll = function (kpis, cb) {
  return this._pushJSONRequest('/',
    kpis.map(this._processKPI),
    cb
  );
};

In the firt one, is wrapping kpi with an array, in the second one, assumes is already an array (soo is vulnerable too, to your 'other (custom) collection type' example).
Both are wrapped in a _pushJSONRequest call.

I can understand you want to keep the Databox look and feel, unified accross libraries (even if some implementations are weird), but but considering, i (as many users) will only interact with this library, i prefer to have a single method for processing all my queries.
Actually in the modification code i suggested before, i don't break the look-and-feel of the library, because, insertAll() is never removed (and becomes just a mapping).

Anyway, it was just a personal suggetion, for people, like @JanHalozan who is missing a simplified and more clear way of pushing data.

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

No branches or pull requests

3 participants