Refactoring backend code to fit standards required for testing #77
Conversation
Andy130604
requested review from
CassioManuguerra,
Thimphou,
JeremyHugentobler,
JoniDutoit,
ecornamu and
npfrei
There was a problem hiding this comment.
Holy, that's a big PR. The work is huge thanks a lot ! It was a necessary change and you did a great job. The refactoring looks great and it seems that no feature is lost in the process.
I know it is a bit bold but dare I say a little more code comments would improve readability ? Apart from that, the code lgtm functionality-wise
There was a problem hiding this comment.
Thank you for your tremendous amount of work, it is much appreciated !
Refactoring
The refactoring was extremely effective, you reduced the size of a lot of functions, but comments are still something that are missing in a lot of places. I started putting some github comments were some should be added, but in a general manner, our code should have more, (Since you did a lot of refactoring, it was not your responsibility to comment other's code, so it's more a general comment addressed to the whole dev team)
Hilt
Nice initiative to convert all the viewmodels to Hilt, but in the process, the MapViewModel's functionnalities were affected. The markers are only fetched on the onLoad event of the map, and not during the camera movement anymore.
This should not be to complicated to fix, but we can do it together later today if you want.
Other
Thank you for the Gradle script ! Will be really useful
|
|
||
| // Class used to get OutdoorActivities from overpass API | ||
| @Singleton | ||
| class OutdoorActivityRepositoryImpl @Inject constructor(private val apiService: ApiService) : |
There was a problem hiding this comment.
I think this class was originally written by @npfrei (correct me if I'm wrong), and it lacks a bit of comments
| import kotlinx.coroutines.tasks.await | ||
|
|
||
| @Singleton | ||
| class AuthRepositoryImpl |
There was a problem hiding this comment.
Same here, would be nice to have comments on this part of the code
|
|
||
| typealias SignOutResponse = Response<Boolean> | ||
|
|
||
| interface AuthRepository { |
There was a problem hiding this comment.
Your refactoring of the login structure is really elegant, but please add some comments for the context / when / how to / what to expect from your methods
|
|
||
| // TODO: Make different composables to reduce this file size | ||
| LastaTheme { |
There was a problem hiding this comment.
Thank you !! really good segmentation of the main activity, much cleaner now
| navigateToHomeScreen() | ||
| } | ||
| } | ||
| is Failure -> LaunchedEffect(Unit) { print(signInWithGoogleResponse.e) } |
There was a problem hiding this comment.
To do in the future: create a composable for sending "error feedback" to the user (like a red box on the top of the screen, with a parameterized text).
| Button(onClick = onSignOut) { Text(text = "Sign out") } | ||
| Button( | ||
| onClick = { | ||
| authViewModel.signOut() |
There was a problem hiding this comment.
Could put this in another function to gain some space and if it might be reused somewhere else, otherwise good job
There was a problem hiding this comment.
Really amazing implication in order to establish organization/homogenization through out the project. One downside is that since the refactoring is done before merging our pull requests it may cause more work in order to resolver conflicts later as some files that we have been working on got replaced/changed. Of course it was necessary in order to test the functions so thank you for the work done! Besides Jeremy remarks I don't have anything else to add, the whole structure looks good to me and great job with the repository and interfaces, as well as implementing dagger-hilt for the viewmodel, looks really clean to me!
There was a problem hiding this comment.
Hello,
great restructuring of the app, will greatly benefit the testing. Code looks really solid, maybe a few suggestions here and there :
app/build.gradle.kts: The gradle task you added will be great for submitting (especially for me who struggles to think about KTFMT ;_;), maybe we could also add the connected tests?app/src/main/java/com/lastaoutdoor/lasta/data/api/OutdoorActivityRepositoryImpl.kt: When you catch an exception, you only return an empty list, we should also ensure to print a message to the user notifying him/her that the call failed.app/src/main/java/com/lastaoutdoor/lasta/di/AppModule.kt: We should comment a bit more the AppModule, even if it is pretty straightforward, just to explain its use a bit moreapp/src/main/java/com/lastaoutdoor/lasta/ui/navigation/MainAppNavGraph.kt: Maybe for later make it so that there is no need to pass the preference and auth view models as explicit parameters- The map seems to be not working, the points do not update automatically
Overall this is really good thank you for your humongous work, great code clarity.
There was a problem hiding this comment.
Hi Andrew,
Great work on the project! Your contributions have significantly enhanced testing efficiency and improved code modularity through refactoring.
Thank you for implementing the UserPreferences, I'll implement this in link with the database soon, we take a step in the right direction I guess!
Suggestions for improvement:
- Ensure error messages are provided in
OutdoorActivityRepositoryImpl.kt. - Consider commenting more on tricky parts!
Thank you for your dedication to organization and consistency. Let's address these suggestions and continue moving forward!
|
There was a problem hiding this comment.
Hi,
In my previous review, I reported some changes to make, but after a discussion with the team, we decided to fix the issues (that are non-blocking) in another branch.
This will allow people to start testing earlier and I will fix the viewmodel on another branch (juste changing some conditions / structure to adapt with Hilt)
I did mostly the auth, preferences and api, still have to wait for @Thimphou PR to look at DB implementation.
The map view model got remodeled a bit and still have to make the discovery screen get the activities from API. I came very close to it working, I think it is a matter of minutes.