Make IDs unique based on existing items; fixes #102

[andrejewski]

No description provided.

[justinbmeyer]

Is items list-like? If yes, better to use .forEach or can-util/js/each/

[andrejewski]

I'll use can-util's each, I do not think we can make the assumption it is list-like based on the current code and each will handle the object case.

[justinbmeyer]

I think we might be able to calculate the right number only once and cache it. I think this is n^2.

[andrejewski]

It is O(n^2). We can make it O(n) if we make a point-of-call cache. Since we don't copy the individual items if they are passed directly any item's ID can be changed throughout a test so non-point-of-call caching is ineffective. If you want to change nextUniqueId()'s i=0 to say i=100 to not bump into someone's items, that's fine as well. In test scenarios, the uniqueness guarantee (correctness) is more important than the minor amortized cost (performance).

[justinbmeyer]

It's expected that passed data is not changed. I consider changing stored data by reference an invalid use. Is this amortized? I thought it was called every POST?

…

Sent from my iPhone

On May 14, 2017, at 8:02 PM, Chris Andrejewski ***@***.***> wrote: @andrejewski commented on this pull request. In helpers/legacyStore.js: > var i = 0; return function () { return i++; } })(); + // Check to see if an existing item has the ID we want to assign + var isIdUnique = function (items, id) { + for (var key in items) { + if (items[key].id === id) { + return false; + } + } + return true; + } + + var getUniqueId = function (items) { + var id = nextUniqueId(); It is O(n^2). We can make it O(n) if we make a point-of-call cache. Since we don't copy the individual items if they are passed directly any item's ID can be changed throughout a test so non-point-of-call caching is ineffective. If you want to change nextUniqueId()'s i=0 to say i=100 to not bump into someone's items, that's fine as well. In test scenarios, the uniqueness guarantee (correctness) is more important than the minor amortized cost (performance). — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or mute the thread.

[justinbmeyer]

In practice, I've created fixtures with 1000s of items. It's common to use fixtures to simulate large amounts of data.

…

Sent from my iPhone

On May 14, 2017, at 8:02 PM, Chris Andrejewski ***@***.***> wrote: @andrejewski commented on this pull request. In helpers/legacyStore.js: > var i = 0; return function () { return i++; } })(); + // Check to see if an existing item has the ID we want to assign + var isIdUnique = function (items, id) { + for (var key in items) { + if (items[key].id === id) { + return false; + } + } + return true; + } + + var getUniqueId = function (items) { + var id = nextUniqueId(); It is O(n^2). We can make it O(n) if we make a point-of-call cache. Since we don't copy the individual items if they are passed directly any item's ID can be changed throughout a test so non-point-of-call caching is ineffective. If you want to change nextUniqueId()'s i=0 to say i=100 to not bump into someone's items, that's fine as well. In test scenarios, the uniqueness guarantee (correctness) is more important than the minor amortized cost (performance). — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or mute the thread.

[justinbmeyer]

There's a bunch of places like this in our projects where we assume that data should not be mutated such as an event object's type when it's dispatched or initial props passed to new DefineMap. We could seal or clone these objects, but it's generally not worth it because users have a similar expectation. Perhaps we need a way to make this explicit ... that certain objects passed to functions should not be mutated.

…

Sent from my iPhone

On May 14, 2017, at 9:06 PM, Justin Meyer ***@***.***> wrote: It's expected that passed data is not changed. I consider changing stored data by reference an invalid use. Is this amortized? I thought it was called every POST? Sent from my iPhone > On May 14, 2017, at 8:02 PM, Chris Andrejewski ***@***.***> wrote: > > @andrejewski commented on this pull request. > > In helpers/legacyStore.js: > > > var i = 0; > return function () { > return i++; > } > })(); > > + // Check to see if an existing item has the ID we want to assign > + var isIdUnique = function (items, id) { > + for (var key in items) { > + if (items[key].id === id) { > + return false; > + } > + } > + return true; > + } > + > + var getUniqueId = function (items) { > + var id = nextUniqueId(); > It is O(n^2). We can make it O(n) if we make a point-of-call cache. Since we don't copy the individual items if they are passed directly any item's ID can be changed throughout a test so non-point-of-call caching is ineffective. If you want to change nextUniqueId()'s i=0 to say i=100 to not bump into someone's items, that's fine as well. In test scenarios, the uniqueness guarantee (correctness) is more important than the minor amortized cost (performance). > > — > You are receiving this because your review was requested. > Reply to this email directly, view it on GitHub, or mute the thread. >