-
-
Save joshuaebowling/98457ba11d743e146def to your computer and use it in GitHub Desktop.
// I'm going to leave my debugging code in so you can see how I prove to myself that I've resolved issues | |
/* I added the NAMESPACE declaration to troubleshoot. | |
* I understand this was only part of a larger | |
* codeblock that may or may not have declared NAMESPACE | |
*/ | |
var NAMESPACE; | |
if (!NAMESPACE || typeof NAMESPACE == 'undefined') { | |
NAMESPACE = { | |
_all_ids: new Object() | |
}; | |
var id = function (id) { | |
var persona = {}; | |
// shorthand reference | |
var _all_ids = NAMESPACE._all_ids; | |
// local var declaration | |
var _id = id; | |
/* because the id gets deleted from _all_ids on close() invocation, I imagine it should be added also. | |
* but I have no clue to what end numbers are added or deleted from this array. | |
*/ | |
// added terminating semi-colon to end of function | |
var getId = function () { | |
// use of local var | |
return _id; | |
}; | |
// assignment of function to returned object | |
persona.getId = getId; | |
// add the current _id to _all_ids | |
_all_ids[_id] = persona; | |
// local var declaration | |
var _closed = false; | |
// added terminating semi-colon to end of function | |
var close = function () { | |
delete _all_ids[getId()]; | |
// the line below references a prototypal property that doesn't seem to be in existence | |
// this._closed = true; | |
// the output below should prove that this._closed is an improper reference | |
console.log('according to the original code, this should be false, not undefined.', this._closed); | |
// instead, I'll reference the local property created above | |
_closed = true; | |
}; | |
persona.close = close; | |
return persona; | |
} | |
NAMESPACE['id'] = id; | |
} |
I would have to say no since I've ruled out the possiblity of prototypal shenanigans. It would lead me to believe that it should be part of the NAMESPACE object. This possibility may be further confirmed by the name
_all_ids
since its formatted in an internal-ish style.
Excellent, so how can we fix it? Is it as simple as NAMESPACE._all_ids = {}
, or should we do something else? What are the benefits/drawbacks to such an approach (vs. another)?
Like a list of users for messaging or somesuch activity, which would explain
close
perhaps.
Great examples.
For keeping track of
id
s that are yet to be closed. So an instance generated byid
can be found and then deleted after a message/result is delivered/returned.
Let's write some tests that demonstrates how we expect close
to work, then get those tests to pass (which probably implies fixing where _all_ids
lives and how it's used).
We're getting there! 😁
I would recommend migrating this to a GitHub repository, since it provides better tools for this kind of thing. (At the very least, we'll know that @-style mentions notify the mentioned party).
@posita,
I'll begin immediately.
@posita,
see the repository
@posita,
Is it as simple as NAMESPACE._all_ids = {}
, or should we do something else? What are the benefits/drawbacks to such an approach (vs. another)?
Pros
- Simple is awesome, unless it's overly simplistic. What's more, it seems to work judging by the tests I've included in the repository.
Cons
- There's no protection from
_all_ids
getting overwritten or deleted accidentally. - There would be no event hooks, etc.
- There would be no facility for features such as
undo
.
My recommendation: There is precedent in the code to declare a local and return the local from a function, a la var getId = function() {}; persona.getId = getId;
This will take care of Cons 1 which is critical while Cons 2,3 are pre-optimizey and I've seen no indication such features are necessary so far.
@posita,
Personally, I don't know of any cases where it's prudent to alter the prototype in this way, especially a built-in one. That doesn't mean I've never seen it done or put it past a puzzler. But since this is not the scenario, I can happily move on.
I'm going to attempt this answer by straining my thoughts through your questions as follows:
Is that indicative that something is missing?
Yes, the addition of any data to
_all_ids
which acts as container of some sort.Is _all_ids even in the right place?
I would have to say no since I've ruled out the possiblity of prototypal shenanigans. It would lead me to believe that it should be part of the NAMESPACE object. This possibility may be further confirmed by the name
_all_ids
since its formatted in an internal-ish style.Should it even be an Array instance, or should it be something else?
It would make sense for it to be an
Object
, espeically since whatever is stored in_all_ids
can be referenced like_all_ids[getId()]
and whatever is referenced can be removed bydelete _all_ids[getId]
without creatingundefined
entries which happens when_all_ids
is an instance ofArray
. Additionally, it would be non-standard (so far as I know) to remove items from anArray
in this manner.I'll demonstrate an
Object
-based solution in my edits to id.js.What if I told you that NAMESPACE.id was a constructor-ish thing for creating objects to be used in a server context, rather than a browser context?
It would tell me that I don't need to be concerned with a clever user gaining access to other users' information or other malfeasances.
What if I also told you that ids were for modeling (keeping track of) some kind of server side resource?
Like a list of users for messaging or somesuch activity, which would explain
close
perhaps.Does that help you guess what _all_ids was intended for?
For keeping track of
id
s that are yet to be closed. So an instance generated byid
can be found and then deleted after a message/result is delivered/returned.I rewrote the test mock-ups also