Limit sport filters for MVP #50
Conversation
zachseidner1
left a comment
zachseidner1
left a comment
There was a problem hiding this comment.
Thank you for the fix! Instead of rewriting this if statement so many times, can we factor this out into a function that checks if a sport display name is valid (something that we want to show)? Typing out so many strings again and again like this is extremely error prone.
zachseidner1
left a comment
zachseidner1
left a comment
There was a problem hiding this comment.
Could you resolve the merge conflicts before requesting my review? On Android Studio, pull the latest changes from main, then go back to your branch and do Git > Merge > origin/main. Then this should bring up the merge conflict resolution editor. Let me know if you need help from there!
# Conflicts: # app/src/main/java/com/cornellappdev/score/components/FeaturedGameCard.kt # app/src/main/java/com/cornellappdev/score/components/GameCard.kt # app/src/main/java/com/cornellappdev/score/model/Game.kt # app/src/main/java/com/cornellappdev/score/viewmodel/HomeViewModel.kt # app/src/main/java/com/cornellappdev/score/viewmodel/PastGamesViewModel.kt
zachseidner1
left a comment
zachseidner1
left a comment
There was a problem hiding this comment.
This is a huge improvement. I should've seen this in my first review, but I just realized that this could still be better. The issue with this is that now, every time we create a view model or read sports data, we will have to remember to call isValidSport. It would be very easy for someone later on to forget to call this function and then have issues as a result. To my understanding, throughout the entire app we only want to show valid sports that we know the data is correct for. So I think instead we should filter for valid sports at the root of the data source, the repository. This way we would only have to do it once in the entire code base, and it's less error prone in the future. Sorry for the thrash on this, but I'm hoping it's an easy change to make. Let me know if you agree!
|
For some reason when I run the app with these changes, on the first run it will fail to fetch games (or it times out), but when I rerequest on the same run, it will successfully load. Not sure if this is an issue with my emulator or the code. |
zachseidner1
left a comment
zachseidner1
left a comment
There was a problem hiding this comment.
Thanks for making these changes! I think this is the usual behavior of Score because of the backend, so don't worry about the loading issue. It could also be an emulator thing.
| GenderDivision.ALL, | ||
| null -> Sport.entries | ||
| } | ||
| }.filter { isValidSport(it.displayName) } |
There was a problem hiding this comment.
I don't think we need to filter here since we already should've done this in the repository.
There was a problem hiding this comment.
This does not work if I remove it
There was a problem hiding this comment.
oh oops that was definitely wrong of me, I thought that was data coming from the BE, sorry for not looking at that closer.
| games?.games?.filter { game -> isValidSport(game?.sport ?: "") } | ||
| ?.mapNotNull { game -> |
There was a problem hiding this comment.
Nit: filterNotNull and then map is a bit cleaner here so you don't have to handle null in the predicate.
Uh oh!
There was an error while loading. Please reload this page.