kaungkhantsoe / nimbletest

Technical Test for Nimble

NimbleTest

Technical Test for Nimble

Coding test app. Using Nimble Survey api and OAuth 2.

nimbletest's Issues

Handle specific response on the extension with generic type

The executeOrThrow() extension is created with the generic type <T>, your implementation validates LoginResponse even though this extension is being used for different API endpoint, it means that different response classes, I think we could update to generic type for common usage instead.

NimbleTest/app/src/main/java/com/kks/nimbletest/util/OkHttpExt.kt

Lines 21 to 27 in dc472de

	if (errCode in 400..499)
	throw HttpException(
	Response.error<BaseResponse<LoginResponse>>(
	errCode,
	it
	)
	)

NimbleTest/app/src/main/java/com/kks/nimbletest/util/OkHttpExt.kt

Lines 50 to 58 in dc472de

	} catch (httpException: HttpException) {
	response?.errorBody()?.let {
	throw HttpException(
	Response.error<BaseResponse<LoginResponse>>(
	httpException.code(),
	it
	)
	)
	}

Clean unused resources from the project

I see there are some classes located on the project but no reference. It would be better if we get rid of them, how do you think?

NimbleTest/app/src/main/res/transition/change_image_transform.xml

Lines 1 to 4 in dc472de

	<?xml version="1.0" encoding="utf-8"?>
	<transitionSet xmlns:android="http://schemas.android.com/apk/res/android">
	<changeImageTransform/>
	</transitionSet>

NimbleTest/app/src/main/java/com/kks/nimbletest/util/interceptors/FakeCallInterceptor.kt

Lines 14 to 35 in dc472de

	override fun intercept(chain: Interceptor.Chain): Response {
	if (BuildConfig.DEBUG) {
	val uri = chain.request().url.toUri()
	val query = uri.query
	val parsedQuery = query.split("&").associate {
	val (left, right) = it.split("=")
	left to right
	}
	val responseString = when {
	uri.toString().endsWith("token") -> {
	if (parsedQuery["client_id"].isNullOrEmpty() || parsedQuery["client_secret"].isNullOrEmpty()) {
	MockResponseFileReader("login_invalid_client.json").content
	}
	else if (parsedQuery["email"].isNullOrEmpty() || parsedQuery["password"].isNullOrEmpty()) {
	MockResponseFileReader("login_invalid_grant.json").content
	}
	else {
	MockResponseFileReader("login_success.json").content
	}
	}
	else -> ""
	}

Implement validation and loading status in the Repository layer

Following the Android app architecture fundamental, the Repository layer is responsible for handling fetching, storing,... the data for the app. Therefore, the logic to validate username and password, or loading state should be handled in the ViewModel layer instead. How do you think? 😄

NimbleTest/app/src/main/java/com/kks/nimbletest/repo/login/LoginRepoImpl.kt

Lines 29 to 51 in dc472de

	override fun loginWithEmailAndPassword(
	email: String,
	password: String
	): Flow<ResourceState<LoginResponse>> =
	flow {
	emit(ResourceState.Loading)
	when {
	email.isEmpty() -> emit(ResourceState.Error(AppConstants.error_email_empty))
	password.isEmpty() -> emit(ResourceState.Error(AppConstants.error_password_empty))
	else -> {
	val apiResult = safeApiCall(Dispatchers.IO) {
	apiInterface.loginUser(
	LoginRequest(
	email = email,
	password = password,
	client_id = customKeyProvider.getClientId(),
	client_secret = customKeyProvider.getClientSecret()
	)
	)
	.executeOrThrow()
	}

Inject dependencies with Android Hilt

It's a good point to see you are applying the Hilt for DI pattern on projects to manage dependencies. Besides that, I am wondering why do you need the TokenAuthenticatorEntryPoint inside the CustomAccessTokenInterceptor 🤔 It looks like a workaround solution to provide the preferenceManager and tokenRepo. Can we get rid of this and perform the injection when the class's instance is initialized instead?

NimbleTest/app/src/main/java/com/kks/nimbletest/util/interceptors/CustomAccessTokenInterceptor.kt

Lines 33 to 38 in dc472de

	@EntryPoint
	@InstallIn(SingletonComponent::class)
	interface TokenAuthenticatorEntryPoint {
	fun preferenceManager(): PreferenceManager
	fun tokenRepo(): TokenRepoImpl
	}

Conditional expression to handle error cases

I am curious there is a missing is on the ResourceState.NetworkError branch 😄 So would it be possible to add is for consistency to validate the particular type , or do you have any supposed idea to do that?

NimbleTest/app/src/main/java/com/kks/nimbletest/ui/login/LoginActivity.kt

Lines 58 to 60 in dc472de

	is ResourceState.Error -> handleError(state.error)
	is ResourceState.GenericError -> handleError(state.error)
	ResourceState.NetworkError -> handleError(AppConstants.NETWORK_ERROR)

NimbleTest/app/src/main/java/com/kks/nimbletest/ui/home/HomeActivity.kt

Lines 223 to 225 in dc472de

	is ResourceState.Error -> handleError(it.error)
	is ResourceState.GenericError -> handleError(it.error)
	ResourceState.NetworkError -> handleError(AppConstants.NETWORK_ERROR)

NimbleTest/app/src/main/java/com/kks/nimbletest/ui/forget/ForgetPasswordActivity.kt

Lines 57 to 59 in dc472de

	is ResourceState.Error -> handleError(it.error)
	is ResourceState.GenericError -> handleError(it.error)
	ResourceState.NetworkError -> handleError(AppConstants.NETWORK_ERROR)

Common practices for effective Kotlin

Wrapping const with Object is old-fashioned with Java-style, Kotlin allows us to define the constants on the top-class level, it's a better approach to drop unnecessary class or object.

NimbleTest/app/src/main/java/com/kks/nimbletest/constants/AppConstants.kt

Lines 7 to 16 in dc472de

	object AppConstants {
	const val UNKNOWN_ERROR = "Unknown error"
	const val SUCCESS_WITH_NULL_ERROR = "Success with null error"
	const val UNKNOWN_ERROR_MESSAGE = "Unknown error message"
	const val NETWORK_ERROR = "Network Error"
	const val error_email_empty = "Email cannot be empty"
	const val error_password_empty = "Password cannot be empty"
	const val success = "success"
	}

NimbleTest/app/src/main/java/com/kks/nimbletest/constants/DateConstants.kt

Lines 7 to 10 in dc472de

	object DateConstants {
	const val EEEE_comma_MMMMM_d = "EEEE,MMMM d"
	}

NimbleTest/app/src/main/java/com/kks/nimbletest/constants/PrefConstants.kt

Lines 7 to 11 in dc472de

	object PrefConstants {
	const val PREF_ACCESS_TOKEN = "access_token"
	const val PREF_REFRESH_TOKEN = "refresh_token"
	const val PREF_LOGGED_IN = "logged_in"
	}

Furthermore, the properties in the Request/Response data classes violate the most common Kotlin convention. I understand that it matches the JSON key for serializing/deserializing the objects automatically but we could use annotation to serialize rather than leaving the properties name like this.

NimbleTest/app/src/main/java/com/kks/nimbletest/data/network/request/RefreshTokenRequest.kt

Lines 7 to 12 in dc472de

	data class RefreshTokenRequest(
	val grant_type: String = "refresh_token",
	val refresh_token: String,
	val client_id: String,
	val client_secret: String
	)

Greeting and starting the reviewing process 👋

Hello Kaung Khant Soe,

Nice to e-meet you 👋 and thanks for your effort on this submission. My name is Huey - an Android Developer at Nimble and I would like to conduct the feedback via Issue tickets regarding your performance here.

Keep in mind, this is a bidirectional process, so we would love to hear back from you as well, so please don’t hesitate to raise your question/correction in case we missed anything.

If we’re aligned on any issue here and you’d like to correct them, please go ahead and open a Pull Request per fix, merge it when you feel confident and we will follow up on those fixes 😇

I hope you find the process enjoyable. Good luck and happy coding 🤘

Create extension to handle loading image with Glide

In my opinion, the PresentationModule is unnecessary because the RequestOptions and RequestManager are useless without ImageView to load the image from URL or resource, I would suggest creating an extension to load the image with the Glide configuration for common usage? Don't hesitate to lemme know about your idea 😄

NimbleTest/app/src/main/java/com/kks/nimbletest/di/PresentationModule.kt

Lines 19 to 41 in dc472de

	@Module
	@InstallIn(SingletonComponent::class)
	class PresentationModule {
	@Singleton
	@Provides
	fun provideRequestOptions(): RequestOptions {
	return RequestOptions
	.placeholderOf(R.drawable.placeholder)
	.error(R.drawable.placeholder)
	.centerCrop()
	}
	@Singleton
	@Provides
	fun provideGlideInstance(
	@ApplicationContext context: Context,
	requestOptions: RequestOptions
	): RequestManager {
	return Glide.with(context)
	.setDefaultRequestOptions(requestOptions)
	}
	}