NPE fix when iterating through ids in the SubscriptionSuccessfulMessage #184
Conversation
|
Hi, Sorry for the delay on this, and thanks for the PR! I'll look at this now. |
| final String userData = hash.get(id) != null ? GSON.toJson(hash.get(id)) : null; | ||
| final User user = new User(id, userData); | ||
| idToUserMap.put(id, user); | ||
| if (ids != null && !ids.isEmpty()) { |
There was a problem hiding this comment.
Is the ids.isEmpty() check necessary? If an interable data structure like this is empty, then the for loop will just iterate zero times, since there's nothing to iterate over. Unless I'm missing something, the only check necessary here (and thank you for adding this!) is the check that ids is not null.
There was a problem hiding this comment.
It's my understanding the for loop will create a Iterator object even if it's empty so this isEmpty() check offers a performance improvement (all be it a tiny one). If you feel strongly this makes the code less readable then I'm happy to change it.
There was a problem hiding this comment.
Not at all, I guess it can offer a small performance benefit! I'll merge this PR in and release by EOD.
Description of the pull request
Adds a null check to ids list before iterating in for loop. Fixes #183
Why is the change necessary?
On Android 7 and 8 we've recently seen a spike in crashes from inside the 1.8 pusher SDK. We have seen NullPointerException when iterating through this list of ids. Related issue #183
CC @pusher/mobile