Conversation
myattj
left a comment
myattj
left a comment
There was a problem hiding this comment.
This looks generally good to me, no syntax stuff or big logical issues. But this feels a little above my pay grade to approve, since more teams use appteamtoolbox, so maybe request @maxnabokow or @samrshi
samrshi
left a comment
samrshi
left a comment
There was a problem hiding this comment.
Left a few comments. Let me know what you think
| if index >= count { | ||
| items.append(element) | ||
| } else { | ||
| items.insert(element, at: index) | ||
| } |
There was a problem hiding this comment.
what happens if index is < 0? we should probably handle that case, since we currently allow/support passing -1 as a param from other places
There was a problem hiding this comment.
When we use the insertAt function within Centible, it's only passed positive values since it's using existing indexes, so that's a good point I didn't think about. I suppose if it's less than zero we can just instantly return it just as a precaution.
There was a problem hiding this comment.
you could also maybe prepend it to the list
| // If the index is equal to 0, it indicates that the elements passed through as an argument is a larger array, and items is the subset of elements. | ||
| if index == -1 { |
There was a problem hiding this comment.
Is this comment accurate? Seeing "if this index is equal to 0" immediately followed by if index == -1 is kind of funky. What's going on here?
There was a problem hiding this comment.
That's a typo, I'll fix that comment
| // Given a certain sorted array, this function determines whether to sort it based on a superset of a larger array, or an array simply in a different order. | ||
| public mutating func reorderItems(to elements: [Element], index: Int) { | ||
| // If the index is equal to 0, it indicates that the elements passed through as an argument is a larger array, and items is the subset of elements. | ||
| if index == -1 { |
There was a problem hiding this comment.
I'm not a fan of passing an index of -1 to change the behavior of this method. Other than changing how the sorting is done, index is only used to update currIndex, right?
I also don't quite understand the intended behavior of this method in the two cases. Is this right?
- If
index == -1, then we resort the internalitemsarray, preserving the relative order of items in theelementsparameter? - If
index != -1, then we filter the internalitemsarray based on whether each element is inelements?
If that is correct, then these two behaviors seem pretty unrelated to each other. And passing -1 to index definitely doesn't make sense to switch between the two to me.
Maybe they should be two separate methods? One to rearrange elements based on another array, and one to filter? Or at least we should come up with a better mechanism to switch between the behavior and pick a better name. You could also add a method to change currIndex if needed.
Regardless, let's add some good doc (///) comments to whatever methods we add to this repo.
There was a problem hiding this comment.
You bring up good points, yeah I'll separate the two conditions within the function to be their own independent function. Passing -1 to index was just a way to distinguish two different cases that occurred when the function occurred, but in retrospect it probably would've been better to use enums and make separate functions. I'll fix this and redo the documentation.
There was a problem hiding this comment.
Cool. I might prefer to provide separate methods and let the caller call the associated method
| // Maps each element in 'elements' to its 'id', creating an array of IDs. | ||
| let unsortedArrayID = elements.map { $0.id } | ||
|
|
||
| // Filters 'items' to include only those whose 'id' is found in 'unsortedArrayID'. | ||
| let sortedArray = items.filter { unsortedArrayID.contains($0.id) } | ||
|
|
||
| // Updates 'items' to be the filtered list, effectively removing any items not present in 'elements'. | ||
| items = sortedArray |
There was a problem hiding this comment.
this doesn't seem to reorder the elements in the list, which is pretty confusing to me given the name of the method
There was a problem hiding this comment.
I'll fix this in the new separate function.
|
maxnabokow
removed request for
erayala15,
maxnabokow,
samrshi and
sarayuyen3
4 participants
Added Extra Functions to the Toolbox Pacagek for more flexible data structure features