Fix tons of bugs relating to null snapshot.values and improve perf for good citizens
#871
Conversation
Signed-off-by: Alex Saveau <[email protected]>
Signed-off-by: Alex Saveau <[email protected]>
| protected void onKeyAdded(DataSnapshot data) {} | ||
|
|
||
| protected void onKeyAdded(DataSnapshot data, int index) { | ||
| onKeyAdded(data); |
There was a problem hiding this comment.
Unfortunately, this isn't backwards compatible if people call super on the old method. 😢 I totally understand if you don't want to proceed with this change though we would be missing out on some significant perf gains: O(n) with the old method, O(1) with the new method on simple additions.
| int dataCount = size(); | ||
| int dataIndex = 0; | ||
| for (int keyIndex = 0; dataIndex < dataCount; keyIndex++) { | ||
| if (keyIndex == mKeySnapshots.size()) { break; } |
There was a problem hiding this comment.
I added this line to the old method to handle being called by a deletion at the last index.
| mChatIndicesRef = FirebaseDatabase.getInstance() | ||
| .getReference() | ||
| .child("chatIndices") | ||
| .child(FirebaseAuth.getInstance().getCurrentUser().getUid()); |
There was a problem hiding this comment.
Added a uid to the indices to make them more realistic.
| @@ -58,6 +71,10 @@ private void init(Query query) { | |||
| mQuery = query; | |||
| } | |||
|
|
|||
| void setPreChangeEventListener(PreChangeEventListener listener) { | |||
There was a problem hiding this comment.
Not sure what to do about this api... seems like other devs might want to use it, but it's kinda gross.
SUPERCILEX
changed the title
snapshot.valuessnapshot.values and improve perf for good citizens
|
@SUPERCILEX sorry I never responded here. I think we're both on the same page about this one. It's definitely an improvement in some ways, but If there are any small improvements from this you'd like to keep we can move forward with those, otherwise I'd just say we document the limitations of the indexed adapters. |
|
Oops, didn't mean to close this PR. @samtstern yep, I'll keep as many changes as I can as long as they're backwards compatible. |
Signed-off-by: Alex Saveau <[email protected]>
|
@samtstern Could you please restart the build? My Travis is passing. Thanks! |
Signed-off-by: Alex Saveau <[email protected]>
| } else { | ||
| int dataCount = size(); | ||
| int dataIndex = 0; | ||
| for (int keyIndex = 0; dataIndex < dataCount; keyIndex++) { |
There was a problem hiding this comment.
This is a very strange for loop. It initializes keyIndex but then waits for a condition on dataIndex, all while incrementing keyIndex on each pass.
Took me a very long time to decipher. I'd suggest converting it to something like this:
int dataIndex = 0;
int keyIndex = 0;
while (dataIndex < dataCount) {
// ... stuff
keyIndex++;
}Since the first line is if (keyIndex == mKeySnapshots.size()) { break; } you may even want to make the main condition while(dataIndex < dataCount && keyIndex < mKeySnapshots.size())
Also add comments throughout the method please!
There was a problem hiding this comment.
@samtstern Same! Another contributor who worked on my original PR wrote that for loop and it always takes me 5+ mins to get my head around it again. I thought that was just me! 😆 Anyway, your suggestions sound like great improvements!
Signed-off-by: Alex Saveau <[email protected]>
Signed-off-by: Alex Saveau <[email protected]>
| } else if (mDataSnapshots.get(index).getKey().equals(superKey)) { | ||
| index++; | ||
| private int returnOrFindIndexForKey(int index, String key) { | ||
| int realIndex; |
There was a problem hiding this comment.
Should we initialize realIndex to 0 for clarity? If key.equals(superKey) is true on the first loop then we will break and return realIndex without ever having initialized it.
There was a problem hiding this comment.
Hmmm, we could but intellij complains because we do initialize it to dataIndex while out of the loop. Do you still want to proceed?
There was a problem hiding this comment.
Oh oops I missed that, thanks!
#870 (comment)