Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DiffUtil and possible performance update #12

Closed
brh opened this issue Apr 11, 2019 · 3 comments
Closed

DiffUtil and possible performance update #12

brh opened this issue Apr 11, 2019 · 3 comments

Comments

@brh
Copy link

brh commented Apr 11, 2019

In the DiffUtil code

As suggested by a teammate the areItemsTheSame and the areContentsTheSame should probably be reversed, since the compare closure seems more in line with comparing the contents. So it will look like this instead
`

        override fun areItemsTheSame(oldItemPosition: Int, newItemPosition: Int): Boolean {
            return items[oldItemPosition] == newList[newItemPosition]
        }

        override fun areContentsTheSame(oldItemPosition: Int, newItemPosition: Int): Boolean {
            return compare(items[oldItemPosition], newList[newItemPosition])
        }`

Or alternatively for more flexibility, you can change the lambda to have an additional param
compare: (T, T, Boolean)
where the Boolean lets the method know if this is the contentsTheSame check or not

Final comment, could you be making unnecessary copies of the new list
items = newList.toMutableList()
I am not sure if this is needed at all where you can just do
items = newList
Or if if has to be mutable
if (newList is MutableList) items = newList else items = newList.toMutableList()

I can send all these changes as a pull request if you like

@radoyankov
Copy link
Collaborator

radoyankov commented Apr 11, 2019

Your teammate is right, that does make more sense. Could you make a merge request with that change? You can also include the MutableList check.

brh pushed a commit to brh/fast-list that referenced this issue Apr 12, 2019
@brh
Copy link
Author

brh commented Apr 12, 2019

If all ok, please post lib update to maven, jitpack, or whatever.

radoyankov added a commit that referenced this issue May 14, 2019
DiffUtil and possible performance update #12
@radoyankov
Copy link
Collaborator

yea I'll do that.

REBOOTERS pushed a commit to REBOOTERS/fast-list that referenced this issue Jun 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants