Add Sessionize #2

RivuChk · 2019-06-18T03:50:08Z

Added Sessionize+Retrofit support in base module
Refactored speakers module, to use Sessionize APIs with Retrofit instead of Firebase.

speakers/src/main/java/in/droidcon/speakers/domain/SpeakerRepositoryImplementation.kt

RivuChk · 2019-06-18T04:45:49Z

vivek1794 · 2019-06-18T04:57:24Z

base/src/main/java/in/droidcon/base/repo/remote/ServiceFactory.kt

+        gson: Gson
+    ): Retrofit = Retrofit.Builder()
+        .baseUrl(
+            if (isDebug) {


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.

vivek1794 · 2019-06-18T04:59:24Z

base/src/main/java/in/droidcon/base/repo/remote/ServiceFactory.kt

+
+object ServiceFactory {
+    /**
+     * Provide "make" methods to create instances of [ServiceClass]


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?

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.

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 ?

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

vivek1794 · 2019-06-18T05:01:39Z

base/src/main/java/in/droidcon/base/repo/remote/ServiceFactory.kt

+
+    fun makeOkHttpClient(httpLoggingInterceptor: HttpLoggingInterceptor): OkHttpClient = OkHttpClient.Builder()
+        .addInterceptor(httpLoggingInterceptor)
+        .connectTimeout(120, TimeUnit.SECONDS)


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.

vivek1794 · 2019-06-18T05:03:04Z

dependencies.gradle

+            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}",


NIT would be better if we follow similar conventions for using variables throughout the file. ${someVersion} or $someVersion

vivek1794 · 2019-06-18T05:16:31Z

speakers/src/main/java/in/droidcon/speakers/remote/SpeakerMapper.kt

+            speakerName = type.fullName,
+
+            /*Sessionize doesn't provide company name
+             *However, it has an optional field for Company Website


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.

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 } ]

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 :)

Got it. We can try something once schedule, sessions, and favorites modules are done.

vivek1794 · 2019-06-18T05:30:25Z

speakers/src/main/java/in/droidcon/speakers/domain/SpeakerRepositoryImplementation.kt

+        return if (speakersCache == null) {
+            speakerService.getSpeakers().map {
+                it.map {
+                    SpeakerMapper.mapFromRemote(it)


NIT Could be simplified as,

speakerService.getSpeakers().map { it.map { SpeakerMapper::mapFromRemote } }

vivek1794 · 2019-06-18T05:35:41Z

speakers/src/main/java/in/droidcon/speakers/presentation/ui/SpeakersAdapter.kt

 import `in`.droidcon.speakers.R
-import `in`.droidcon.speakers.model.SpeakerItem
 import android.view.LayoutInflater
 import android.view.View
 import android.view.ViewGroup


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... }

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

…ame`

RivuChk · 2019-06-18T12:00:31Z

Setup Retrofit & Room

iamBedant · 2019-06-18T19:40:39Z

base/build.gradle

@@ -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/\""


We can move the real URLs to local.properties

iamBedant · 2019-06-18T19:42:08Z

base/build.gradle

@@ -54,9 +59,16 @@ dependencies {
    api appDependencies.lottie


Are we planning to add any animated graphics?

@iamBedant - I think we wanted to have a few in the app that's why the dependency... But at the moment we have none...

iamBedant · 2019-06-18T20:16:01Z

speakers/src/main/java/in/droidcon/speakers/domain/SpeakerRepositoryImplementation.kt

-                        emitter.onError(Throwable(task.exception))
-                    }
-                }
+        return if (speakersCache == null) {


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?

@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.

Saved speakerList in speakerCache

iamBedant · 2019-06-18T20:18:52Z

speakers/src/main/java/in/droidcon/speakers/presentation/ui/SpeakersAdapter.kt

+                itemView.setOnClickListener { listItemClickListener.onSpeakerItemClicked(speakerItem) }
+                Glide.with(speakerImgView.context)
+                    .load(speakerItem.speakerImg)
+                    .placeholder(R.drawable.ic_placeholder)


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`

…s well.

…into rivu

Rivu Chakraborty added 2 commits June 18, 2019 09:09

base: Add retrofit support for sessionize

c5557e4

Refactor Speaker Module, use Sessionize APIs instead of Firebase

f24d332

RivuChk added the enhancement New feature or request label Jun 18, 2019

RivuChk requested a review from adnan-SM June 18, 2019 03:50

RivuChk assigned adnan-SM, brijeshmasrani, iamBedant and Hariofspades Jun 18, 2019

RivuChk commented Jun 18, 2019

View reviewed changes

speakers/src/main/java/in/droidcon/speakers/domain/SpeakerRepositoryImplementation.kt Outdated Show resolved Hide resolved

use Koin to inject SpeakerService

4b232d7

vivek1794 reviewed Jun 18, 2019

View reviewed changes

Rivu Chakraborty added 3 commits June 18, 2019 14:19

Address PR Comments

7b1c627

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

Optimised and formatted code

183f1fb

Corrected SPEAKER_COMPARATOR, used speakerId instead of `speakerN…

8b6ff76

…ame`

iamBedant reviewed Jun 18, 2019

View reviewed changes

Rivu Chakraborty added 3 commits June 19, 2019 12:34

Address PR Comments

b476134

1. Move URLs to gradle.properties 2. Fix `speakersCache` issue in `SpeakerRepositoryImplementation`

Move TastState to base, in order to use it in other Android Modules a…

3ba9996

…s well.

Merge branch 'master' of https://github.com/adnan-SM/droidconChennaiV2 …

c99b788

…into rivu

RivuChk requested a review from Hariofspades June 19, 2019 12:07

Merge branch 'master' of https://github.com/adnan-SM/droidconChennaiV2 …

317197d

…into rivu

iamBedant self-requested a review June 19, 2019 14:26

RivuChk added 2 commits June 28, 2019 16:22

setup testing frameworks - spek, junit5, mockito, mockitokotlin

bb980fe

write tests for

cf07d0c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Sessionize #2

Add Sessionize #2

RivuChk commented Jun 18, 2019

RivuChk commented Jun 18, 2019

vivek1794 Jun 18, 2019

RivuChk Jun 18, 2019

vivek1794 Jun 18, 2019

RivuChk Jun 18, 2019

adnan-SM Jun 19, 2019

RivuChk Jun 19, 2019

vivek1794 Jun 18, 2019

RivuChk Jun 18, 2019

vivek1794 Jun 18, 2019

RivuChk Jun 18, 2019

vivek1794 Jun 18, 2019

RivuChk Jun 18, 2019

vivek1794 Jun 18, 2019

RivuChk Jun 18, 2019

vivek1794 Jun 18, 2019

RivuChk Jun 18, 2019

vivek1794 Jun 18, 2019

RivuChk Jun 18, 2019

RivuChk commented Jun 18, 2019

iamBedant Jun 18, 2019

iamBedant Jun 18, 2019

adnan-SM Jun 19, 2019

iamBedant Jun 18, 2019

RivuChk Jun 19, 2019

RivuChk Jun 19, 2019

iamBedant Jun 18, 2019

Add Sessionize #2

Are you sure you want to change the base?

Add Sessionize #2

Conversation

RivuChk commented Jun 18, 2019

RivuChk commented Jun 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RivuChk commented Jun 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment