ObservableArray should have public member .array

[samreid]

To enhance usability, ObservableArray should have a public member .array. Discovered during #265

[pixelzoom]

Please clarify. Are you proposing to add this?

get array() { return this.getArray(); }

[samreid]

The proposal is that:

this._array = []; // @private internal, do not access directly

would change to

// @public (read-only) - the underlying array
this.array = [];

[pixelzoom]

I think that's a very dangerous direction, as is a getter. It's too easy to accidentally modify the "underlying array" and introduce subtle bugs. It also breaks encapsulation by exposing the implementation of how values are stored inside ObservableArray.

[samreid]

How would you characterize the existing implementation, which is ObservableArray.getArray()?

[pixelzoom]

I try to avoid using getArray -- or ObservableArray in general (see #5). I've used ObservableArray in only 2 sims (graphing-lines, equality-explorer), and I was stuck with it in 2 sims that I inherited (unit-rates, vector-addition). When I've had to use getArray, I've made a defense copy using slice. See for example EQUALITY_EXPLORER/TermCreator.

[samreid]

For sims that are using ObservableArray, would you prefer to see:

electrons.array

or

electrons.getArray()

What pattern do you recommend when a sim needs to listen for when items are added or removed?

[pixelzoom]

Ideally, I'd like electrons.getArray() to return an Array of the elements in ObservableArray, so that the implementation of how those elements are stored isn't exposed, and so that the internal storage mechanism (currently Array) is not modified directly. The primary downside to that would be iteration performance.

The current API for listening for items added/removed seems fine. Concerns about that part of the API in #5 seem to have been addressed.

[pixelzoom]

... and i the context of my previous comment, I don't care for electrons.array. Assuming it was an ES5 getter, it looks too much like a field, and every usage of electrons.array. Having to call electrons.getArray() is clearer and (perhaps) more likely to be used judiciously.

[samreid]

How would you feel if we change ObservableArray.map and filter to return Array instead of ObservableArray? That would address many client usage sites.

For cases that need maximum performance, should they be able to access the underlying array? Or will they be discouraged from using ObservableArary in the first place?

[pixelzoom]

How would you feel if we change ObservableArray.map and filter to return Array instead of ObservableArray? That would address many client usage sites.

That would be fine with me. I've never used either, and it seems like returning an Array would be more useful.

For cases that need maximum performance, should they be able to access the underlying array? Or will they be discouraged from using ObservableArary in the first place?

In cases where performance needs to be maximized, I would avoid using ObservableArray altogether if possible (as in Gas Properties.) For situations where you have no other choice, we will probably need to provide access to the underlying array. But that access should be well-documented as "undesirable", "be careful", and "use judiciously", and I'd expect to see it used infrequently, with documentation about why it's necessary.

[samreid]

I improved documentation in the commits. I noticed there are 86 occurrences of .getArray() across the codebase. Many of these could be converted to use ObservableArray.filter and ObservableArray.map which were improved in #265 or other ObservableArray methods. Others may have simple solutions, and the ones that remain should be documented accordingly as prescribed in #266 (comment). For example, density and buoyancy has:

return _.find( this.masses.getArray(), mass => mass.isBoat() ) || null;

But this could likely be changed to something like:

return this.masses.find( mass => mass.isBoat() ) || null;

I'm not sure who should work on this or when it should be done, but we have strayed from the intent of this issue so I'll open a new one.

@pixelzoom can this issue be closed?

[pixelzoom]

Seems appropriate to close.