feat(core): add first and last methods to Collection class
#6056
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6056 +/- ##
==========================================
- Coverage 99.79% 99.79% -0.01%
==========================================
Files 264 264
Lines 18753 18765 +12
Branches 4655 4413 -242
==========================================
+ Hits 18715 18726 +11
- Misses 38 39 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -337,6 +337,10 @@ export class ArrayCollection<T extends object, O extends object> { | |||
| return true; | |||
| } | |||
|
|
|||
| first(): T | undefined { | |||
| return this.getItems()[0]; | |||
There was a problem hiding this comment.
this could be done more effectively, now you convert the internal items set to an array just to select its first value, you can do this[0] to read the first value instead (plus check the loaded state first)
There was a problem hiding this comment.
first(): T | undefined {
return this.items.values().next().value;
}How about this one. This doesnt convert it into array
There was a problem hiding this comment.
yes, but why? your code feels more complex than my proposal (and you still need to check the loaded state).
There was a problem hiding this comment.
first(): T | undefined {
return this[0];
}I guess you meant this
There was a problem hiding this comment.
yes, plus this.checkInitialized(); before that
There was a problem hiding this comment.
sure thing. Will push the updates
| @@ -345,6 +349,10 @@ export class ArrayCollection<T extends object, O extends object> { | |||
| return this.count() === 0; | |||
| } | |||
|
|
|||
| last(): T | undefined { | |||
| return this.getItems()[this.items.size - 1]; | |||
There was a problem hiding this comment.
same here, you can just check the loaded state and
8000
return this[this.length - 1]. in fact this.length is a getter that will check the loaded state, so here it should be enough to just do this
| return this.getItems()[this.items.size - 1]; | |
| return this[this.length - 1]; |
Co-authored-by: Martin Adámek <banan23@gmail.com>
| if (this.isInitialized()) { | ||
| return this[0]; | ||
| } | ||
| return undefined; |
There was a problem hiding this comment.
the method should throw if the collection is not initialized, same for the last one. i would move the checkInitialized to ArrayCollection (and mark it as protected) and do this instead
| if (this.isInitialized()) { | |
| return this[0]; | |
| } | |
| return undefined; | |
| this.checkInitialized(); | |
| return this[0]; |
then the Collection implementations can be removed completely
There was a problem hiding this comment.
checkInitialized doesnt exist on ArrayCollection class. I have the checkInitialized on the collection one though
override first(): T | undefined {
this.checkInitialized();
return super.first();
}There was a problem hiding this comment.
yes, and I just told you to move that method down
|
fyi i will most likely remove the whole |
Yeah its kind of confusing but then again laravels elequoent orm as well as doctrine seems to follow this pattern so mixed thoughts |
PHP's plain arrays get passed around by value ("copy on write" technically, but from userland's pov, that's like passing by value), and the wrappers are there to ensure they get passed by reference instead. In JS, arrays are passed by reference, so these sorts of wrappers are useless. |
Co-authored-by: Martin Adámek <banan23@gmail.com>
first and last methods to Collection class
|
@rubiin do you plan to finish this? |
Currently occupied with some work and couldnt push changes . Will update that sometime soon |
Adds methods on collection which is just a syntatic sugars