-
Notifications
You must be signed in to change notification settings - Fork 0
Room integration and locale change #3
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
base: develop
Are you sure you want to change the base?
Conversation
…radle(project level)
…) and finish its setup. Created classes for favorite users screen (fragment, module, view model, layout)
# Conflicts: # app/src/main/java/com/base_android_template/App.kt
…issue where FavoriteGithubUsersViewModel cannot be injected
…tabase in order to avoid null fields and created a method to map the GithubUserResponse to GithubUserEntity. Implemented insertGithubUsers(), getGithubUsers() and deleteGithubUsers() methods into GithubUsersListDao. Implemented the "Either", "NetworkHandler" and "ExceptionHandler" classes. Added an interface and its implementation for remote Github users list call. Implemented the DatabaseModule to inject room database related classes. Changed the implementation of GithubUsersListUseCase to fetch the list from server and to save it locally in database if the server returns success. Changed the logic in GithubUsersListViewModel to fetch firstly the local users list and if list is empty to call the server endpoint. Updated the adapter, view holder and diffUtil callback clasess to use GithubUserEntity instead of GIthubUserResponse
…ponseCall and NetworkResponseAdapterFactory classes.
…language selection in settings screen.
…nguage in Hawk when changing it. Removed language menu in MainActivity's toolbar. Implemented a BindingAdapters method responsible to set from layout the checked changed listener on RadioGroup
…e message. Animate using MotionLayout the title and the message when scrolling the users list.
…from the ViewModel
… exceptions inside GithubUsersViewModel
|
I changed the into branch to develop to avoid changes that were already reviewed |
NegreaVlad
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed some code, but not all of it. This is a partial review
app/src/main/AndroidManifest.xml
Outdated
| <application | ||
| android:allowBackup="true" | ||
| android:name=".App" | ||
| android:allowBackup="false" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we want this?
|
|
||
| factory<UILoading> { UILoadingImplementation() } | ||
|
|
||
| single { ExceptionHandler() } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this class doesn't implement an interface, there is no benefit of injecting it
| } | ||
|
|
||
| private suspend fun makeRequest(block: suspend () -> Response<List<GithubUserResponse>>): Either<Exception, List<GithubUserResponse>> { | ||
| if (!networkHandler.hasNetworkConnection()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it makes sense to check if there is an active network connection here, before making the call.
If we want to know if the error occurred because of no network, we can check that when getting 404 response
| object NetworkException : Exception() | ||
| object ServerException : Exception() | ||
| object SaveGithubUsersException : Exception() | ||
| object EmptyLocalGithubUsersLisException : Exception() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"EmptyLocalGithubUsersLisException" -> EmptyLocalGithubUsersListException
| @@ -0,0 +1,9 @@ | |||
| package com.base_android_template.shared.network | |||
|
|
|||
| sealed class Exception { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a general network exception is not good IMO because this class will get very large and you cannot understand anything.
Not all projects require the identification of network is down, or other general errors that can happen for all API calls.
I would like to have an error specific for each use case i.e a sealed class like this with errors specific only for github users and a GeneralError. We will not propagate more info downwards because I don't think it's needed for this project.
If certain error need to be treated in a certain way across all the app, then we can create a base class for errors, but it's not needed now.
The API can return a retrofit response which is analysed in the use case
|
|
||
| class ExceptionHandler { | ||
|
|
||
| fun handleGeneralException(exception: Throwable): Either.Failure<Exception> = when (exception) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The naming is wrong. "Handle" means something final. The exception is handled here and that's it. This is just a mapper
| return try { | ||
| val response = block.invoke() | ||
| handleResponse(response) | ||
| } catch (exception: Throwable) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid a too general catch, catch only java.io.IOException
| override fun onActivityCreated(savedInstanceState: Bundle?) { | ||
| super.onActivityCreated(savedInstanceState) | ||
|
|
||
| viewModel.exception.observe(viewLifecycleOwner, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just calls view model logic. It doesn't do anything UI related. Why not put it in view model?
|
|
||
| companion object { | ||
|
|
||
| fun mapToGithubUserEntity(githubUserResponse: GithubUserResponse) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can implement an extension function instead of this. The call is cleaner IMO and we cannot use it without an instance of GithubUserResponse
| } | ||
| } | ||
|
|
||
| fun getRemoteGithubUsers() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the logic of fetching from network or local database should be inside the use case, but I need to read more about this in order to provide good feedback.
…f there is an active network connection before making the call. If we want to know if the error occurred because of no network, we can check that when getting 404 response
… the Github users errors to handle in this way the errors by usecases
…odel. Moved the logic inside GithubUsersFragment -> onActivitCreated() to GithubUserViewModel. Implemented the SingleLiveEvent class to not clear every time the live data value
… GithubUserEntity
This branch contains: