Response to _Opportunity

id.js

// 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;
}

@joshuaebowling, thanks for the submission. Some questions:

1. How would we easily be able to see your changes from the original?
2. What is id? What is it supposed to do?
3. Does it work? If so, is there a way we could easily tell?

@posita, I appreciate the questions and will take them one at a time.
Answer to Question 1
If I forked it from the original, then you (and perhaps anyone else) would be able to see the changes in relation to the original.

Answers to Question 2

• The id function.
  • What is it? It is a function that accepts an argument of the same name.
  • What does it do?
    1. It returns an object called persona (which I describe in more detail in my response to Question 3).
    2. It sets the value of _id to the value of the argument id
    3. It also pushes the value of _id to _all_ids. It does this because I added it. I added it because close() attempts to remove said value from said array although it was never added. This asymmetry lead me to believe that my addition didn't cross the threshold of refactoring.
• The id argument.
  • What is it? It is an argument delared on the above-mentioned id function.
  • What does it do? It is used to set the value of internal variable _id.

Answers to Question 3

• Does it work? In order to satisfactorily answer this question I need to specify how I visualize its intent in words which follow:
  • NAMESPACE should be an object with at least one property, namely id. This id property should be a function that takes an argument also named id. When NAMESPACE.id is invoked, it returns an object that has the following properties.
    • getId as a function
      • this function would return the value of _id variable declared inside the id function.
    • close as a function
      • this function removes the value returned from getId() from the internal array _all_ids which only ever contains 1 or 0 items. _all_ids contains 1 item after id is invoked and before close is invoked. all_ids contains 0 items all other times.
  • I pass no judgement on why the code would seem to work in this way. That, to me, would open the door to refactoring which is not what I understand my task to be. Of course, that's not meant to be a prescriptive definition, just descriptive so you can better understand why I made the choices I made.
• If so, is there a way we could easily tell? This answer is first contingient upon whether I accurately derived a reasonable intention from the code. If I did, then I have to it admit it only proves the following:
  1. getId returns the id argument's value via the _id variable, both before and after close is invoked.
  2. this._closed was an improper reference as its value is undefined; instead the proper reference is _closed which is set to false when id is invoked and true when close is invoked.
     The proof might look something like what follows (this would be an appendage):

var myId = 'joshuaebowling';
var myPersona = NAMESPACE.id(myId);
var getIdEqualsMyId = myPersona.getId() === myId;
console.log('getIdEqualsMyId=%s',getIdEqualsMyId);
/* invoking the following function merely proves this._closed was an improper reference. 
It will not prove that _all_ids is empty, nor that _closed is set to true.
*/
myPersona.close();
var getIdEqualsMyIdAfterClose =  myPersona.getId() === myId;
console.log('getIdEqualsMyIdAfterClose=%s',getIdEqualsMyIdAfterClose);

As I mentioned above, I attempted to correct without refactoring.

@joshuaebowling, thanks for your responses. They are appreciated and understood. My questions were a little open-ended (admittedly intentionally), but let me be more specific:

1. Are you able to point me to a revision history showing the differences between our version and yours?
2. At a high level, based on what you can determine from the original source code, what functionality is id supposed to provide? In other words, what do you think it is used for and can it actually be used that way as written? For example, why does the class exist at all? Why have a close method? I realize this is a bit of a puzzle (hint, hint), since one lacks a context, but take your best guess.
3. Given your best guess at #2, using commonly available development tools and practices, how could you repeatably demonstrate to others that your modifications are valid?

You've already answered #3 with your code snippet (which looks an awful lot like a unit test; hint, hint). But let me know if your answer changes at all after reconsidering #2.

One more hint:

/* ... I have no clue to what end numbers are added or deleted from this array. ... */

Are you sure you have no clue? 😉 Who else would modify _all_ids? Could they modify it?

@posita
First, thanks for the hints, I'll make good use of them below.

1. With shamelessness and a grain of grit, I can show you a revision history.
2. The id function is supposed to provide a means (via close) to remove ids from an Array manifesting itself as an instance named _all_ids, and separately return the id value (via getId) given when NAMESPACE.id was invoked with said value. It could function properly if the Array prototype is/was populated with or has access to a set of ids from which the class instance need only remove said id from _all_ids's prototype or Array's prototype.
3. Taking your hint, I'd need to write unit tests. Specifically, I would need a suite that did the following:

• push some values into Array's prototype
• create an instance of Array via constructor then check if the value's pushed to Array's prototype are present in the instance as well.
• create an instance of NAMESPACE.id passing a value pushed to Array's prototype as my id argument. Then, verify getId returns proper value. Then, invoke close. Then, check the indexOf said value to if it's less than 0. But, the test will prove my solution be invalid if I'm correct about the use Array's prototype as the populating mechanism. For my solution to be valid, I'd have to delete _all_ids.__proto__[getId()]; OR delete Array.prototype[getId()].

Then, another question occurs: what does it mean to close where close removes the id from the set? That could also mean that _id should also be set to undefined and perhaps even delete persona.
I will edit id.js accordingly if you would like.

Here are my test mock-ups

They assume NAMESPACE to be available for testing

It could function properly if the Array prototype is/was populated with or has access to a set of _id_s from which the class instance need only remove said id from _all_ids's prototype or Array's prototype.

Ah, now we're getting warmer. Are you sure we want to go about modifying Array's prototype? After all, the original author went through the trouble to give us a NAMESPACE object, I'm assuming so id wouldn't create create clashes in the global name space. That seems to be a respectful practice. With that in mind, is modifying the prototype of a builtin type in order to get id "working" indicative of a deficiency with the builtin type or a problem with id?

You're right to point out that in the original code, nothing gets added to _all_ids, so delete'ing them doesn't really do anything. Is that indicative that something is missing? Is _all_ids even in the right place? Should it even be an Array instance, or should it be something else?

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? What if I also told you that ids were for modeling (keeping track of) some kind of server side resource? Does that help you guess what _all_ids was intended for? Does it help you guess what close should do?

One more hint: we can benefit a lot more from using the tools available to us. For example, consider making a full-fledged GitHub repository where you can track your code changes, check in unit tests, put how to run them in the README, etc. 😉

@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 by delete _all_ids[getId] without creating undefined entries which happens when _all_ids is an instance of Array. Additionally, it would be non-standard (so far as I know) to remove items from an Array 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 ids that are yet to be closed. So an instance generated by id can be found and then deleted after a message/result is delivered/returned.

I rewrote the test mock-ups also

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 ids that are yet to be closed. So an instance generated by id 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

1. 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

1. There's no protection from _all_ids getting overwritten or deleted accidentally.
2. There would be no event hooks, etc.
3. 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.