-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add Sessionize #2
base: master
Are you sure you want to change the base?
Conversation
speakers/src/main/java/in/droidcon/speakers/domain/SpeakerRepositoryImplementation.kt
Outdated
Show resolved
Hide resolved
gson: Gson | ||
): Retrofit = Retrofit.Builder() | ||
.baseUrl( | ||
if (isDebug) { |
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.
Would be better if the baseUrl is configured as a part of the gradle configuration so that different product flavours could have different configurations. Since we are doing this to showcase various best practices, feel it would be good to have that kind of sample in here.
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.
Done
|
||
object ServiceFactory { | ||
/** | ||
* Provide "make" methods to create instances of [ServiceClass] |
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'm not too familiar with Clean Architecture so please correct me if I am wrong. We have Koin as a means to do DI and this ServiceFactory is essentially creating objects for us that we would use somewhere else. Wouldn't it make sense to have Koin do its work here?
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'm also not too familiar with Clean Architecture, I followed https://github.com/bufferapp/android-clean-architecture-boilerplate/blob/master/remote/src/main/java/org/buffer/android/boilerplate/remote/BufferooServiceFactory.kt
Not only that, another logic behind this factory is that, we will be using Retrofit in different Modules with different Service classes, and these boilerplates to Koin Module classes will bloat them, where we simply want to have an instance of the Service interface we defined there, so I created the Generic ServiceFactory classes, and in Module classes we simply need to call one method.
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.
Not sure I get this completely... Koin supports multi module DI out of the box, so it would be like defining one module with all the necessary things and just using it wherever we want to.
I see your point of having one method call to setup and run everything though.
Would it be a bad idea to just have these object creations in the koin Module class and use it directly ? I like the idea of having abstraction over the methods but in this case since koin will inject them for us I am not sure if having a generic abstraction class helps with anything...
@Hariofspades - Thoughts ?
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 I wasn't able to express myself well enough, so sorry for that.
My objective behind this file is as below, also please correct me if I'm wrong.
Right now there's only one Android module (speakers
) using retrofit, so it's perfectly ok to move these boilerplates to the KoinModule
class in that module. But we are going to have ~3 more different Android modules, all of them are expected to use retrofit. So, in that scenario, if we keep the retrofit boilerplates in particular modules inside Koin, then we will be ending up having to copy paste these boilerplates in each KoinModule
class inside each of the Android modules, resulting in redundant code, the only thing unique would be the ServiceClass
for each module.
We also can't move it inside Koin of the base module, since the ServiceClass
interface should be defined by each module separately.
Another approach can be to inject the object of retrofit through base module and create the ServiceClass
instance inside KoinModule
of different modules, but why inject Retrofit instance and block the memory?
That's the reason I created this methods, so that ServiceClass
instance can be created in KoinModule
of different Android modules easily without any redundancies/boilerplates.
@adnan-SM @Hariofspades @vivek1794
|
||
fun makeOkHttpClient(httpLoggingInterceptor: HttpLoggingInterceptor): OkHttpClient = OkHttpClient.Builder() | ||
.addInterceptor(httpLoggingInterceptor) | ||
.connectTimeout(120, TimeUnit.SECONDS) |
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.
NIT: Would be better to have this sort of magic numbers be stored and fetched from a constants file so that they can be easily referenced and changed later on.
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.
Done
dependencies.gradle
Outdated
androidAnnotations: "com.android.support:support-annotations:${androidAnnotationsVersion}", | ||
okHttp : "com.squareup.okhttp3:okhttp:${okHttpVersion}", | ||
okHttpLogger : "com.squareup.okhttp3:logging-interceptor:${okHttpVersion}", | ||
retrofit : "com.squareup.retrofit2:retrofit:${retrofitVersion}", |
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.
NIT would be better if we follow similar conventions for using variables throughout the file. ${someVersion}
or $someVersion
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.
Done
speakerName = type.fullName, | ||
|
||
/*Sessionize doesn't provide company name | ||
*However, it has an optional field for Company Website |
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 know it is a long shot but, we are collecting company name as a part of the talk. So if it would be possible, we can get the company name from the talk they submitted.
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.
Hey,
I know company name is collected as part of the talk, but the issue here is that they will share that inside the session object, not inside the speaker object, thus we will need to call /all
or /sessions
for that, which I believe wouldn't be a good idea.
Moreover they share those answers in a weird format like below, which makes it even more difficult to use.
"questionAnswers": [
{
"id": 106,
"question": "Where do you work?",
"questionType": "Short_Text",
"answer": "Byju's",
"sort": 5,
"answerExtra": null
}
]
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.
Ah yeah I guessed so. That's why called it a long shot 😄 No issues with the current one but wanted to know if the other thing is feasible. Thus the 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.
Got it. We can try something once schedule, sessions, and favorites modules are done.
return if (speakersCache == null) { | ||
speakerService.getSpeakers().map { | ||
it.map { | ||
SpeakerMapper.mapFromRemote(it) |
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.
NIT Could be simplified as,
speakerService.getSpeakers().map {
it.map { SpeakerMapper::mapFromRemote }
}
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.
Done
import `in`.droidcon.speakers.R | ||
import `in`.droidcon.speakers.model.SpeakerItem | ||
import android.view.LayoutInflater | ||
import android.view.View | ||
import android.view.ViewGroup |
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.
NIT I know this is not a part of this PR but the findViewById used here could be easily replaced by using kotlin synthetic like below,
with(itemView) {
speakerImgView...
}
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.
Done
1. Use BuildConfig, instead of booleain isDebug for `BASE_URL` 2. Used `KeyConstants` for Constants like TimeOut Seconds 3. Simplified `map` in `getSpeakers` 4. Used same variable convention in dependencies.gradle
base/build.gradle
Outdated
@@ -23,6 +22,12 @@ android { | |||
release { | |||
minifyEnabled false | |||
proguardFiles getDefaultProguardFile('proguard-android-optimize.txt'), 'proguard-rules.pro' | |||
|
|||
buildConfigField "String", "BASE_URL", "\"https://sessionize.com/api/v2/e94o5exw/view/\"" |
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 move the real URLs to local.properties
base/build.gradle
Outdated
@@ -54,9 +59,16 @@ dependencies { | |||
api appDependencies.lottie |
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.
Are we planning to add any animated graphics?
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.
@iamBedant - I think we wanted to have a few in the app that's why the dependency... But at the moment we have none...
emitter.onError(Throwable(task.exception)) | ||
} | ||
} | ||
return if (speakersCache == null) { |
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.
According to current implementation, speakerCache
will be always null
are we planning to populate it from persistence? In that case, should we consider persistence as the single source of truth? What about update policy? will it be a push or pull mechanism? Thoughts?
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.
@iamBedant Thanks for this, I totally missed saving the speakersList
to speakersCache
.
The idea here is to save the speakers list in memory, since these are not expected to change very often, at least in a single lifetime of the fragment.
However, our primary goal is to save the data in Room/SQLite/SQLDelight after fetching it for the first time, I couldn't figure out the proper way to configure Room yet for clean architecture where multiple modules have dependencies (SQL) on each other.
Please do it if you can.
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.
Saved speakerList in speakerCache
itemView.setOnClickListener { listItemClickListener.onSpeakerItemClicked(speakerItem) } | ||
Glide.with(speakerImgView.context) | ||
.load(speakerItem.speakerImg) | ||
.placeholder(R.drawable.ic_placeholder) |
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.
Since we are using shimmer view
, we can replace this placeholder with a shimmer drawable. But this is more of a UI task. You can ignore it for now.
1. Move URLs to gradle.properties 2. Fix `speakersCache` issue in `SpeakerRepositoryImplementation`
Added Sessionize+Retrofit support in base module
Refactored speakers module, to use Sessionize APIs with Retrofit instead of Firebase.