Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion app/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ android {

buildTypes {
release {
minifyEnabled true
minifyEnabled false
Copy link
Copy Markdown
Owner Author

@prabuddaFernando prabuddaFernando Mar 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

put minifyEnable true in release mode only. create separate debug mode for development (security concern)

proguardFiles getDefaultProguardFile('proguard-android-optimize.txt'), 'proguard-rules.pro'
}
}
Expand All @@ -34,6 +34,9 @@ android {
}

dependencies {
implementation project(":shoplist")


implementation "org.jetbrains.kotlin:kotlin-stdlib:$kotlin_version"
implementation 'androidx.core:core-ktx:1.3.2'
implementation 'androidx.appcompat:appcompat:1.2.0'
Expand Down
5 changes: 4 additions & 1 deletion app/src/main/java/com/netguru/codereview/MainActivity.kt
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
package com.netguru.codereview

import androidx.appcompat.app.AppCompatActivity
import android.os.Bundle
import androidx.appcompat.app.AppCompatActivity
import com.netguru.codereview.ui.MainFragment

class MainActivity : AppCompatActivity() {
override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)
setContentView(R.layout.activity_main)

supportFragmentManager.beginTransaction().replace(R.id.container, MainFragment()).commitNow()
}
}
1 change: 1 addition & 0 deletions settings.gradle
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
include ':shoplist'
include ':app'
rootProject.name = "Code Review"
59 changes: 59 additions & 0 deletions shoplist/build.gradle
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
plugins {
id 'com.android.library'
id 'kotlin-android'
}

android {
compileSdkVersion 30
buildToolsVersion "30.0.3"

defaultConfig {
minSdkVersion 21
targetSdkVersion 30
versionCode 1
versionName "1.0"

testInstrumentationRunner "androidx.test.runner.AndroidJUnitRunner"
consumerProguardFiles "consumer-rules.pro"
}

buildTypes {
release {
minifyEnabled false
proguardFiles getDefaultProguardFile('proguard-android-optimize.txt'), 'proguard-rules.pro'
}
}
compileOptions {
sourceCompatibility JavaVersion.VERSION_1_8
targetCompatibility JavaVersion.VERSION_1_8
}
kotlinOptions {
jvmTarget = '1.8'
}
buildFeatures.viewBinding true
}

dependencies {

api "org.jetbrains.kotlin:kotlin-stdlib:$kotlin_version"
api 'androidx.core:core-ktx:1.3.2'
api 'androidx.appcompat:appcompat:1.2.0'
api 'com.google.android.material:material:1.3.0'
api 'androidx.lifecycle:lifecycle-livedata-ktx:2.3.1'
api 'androidx.lifecycle:lifecycle-viewmodel-ktx:2.3.1'
api 'org.jetbrains.kotlinx:kotlinx-coroutines-android:1.4.3'
api 'io.coil-kt:coil:1.2.1'

api "com.google.dagger:dagger-android:2.33"
api "com.google.dagger:dagger-android-support:2.33"
api 'com.squareup.okhttp3:okhttp:4.9.1'
api 'com.squareup.okhttp3:logging-interceptor:4.9.1'
api 'com.squareup.retrofit2:retrofit:2.9.0'
api 'com.jakewharton.retrofit:retrofit2-kotlinx-serialization-converter:0.8.0'
api 'org.jetbrains.kotlinx:kotlinx-serialization-core:1.1.0'
api 'org.jetbrains.kotlinx:kotlinx-serialization-json:1.1.0'

testImplementation 'junit:junit:4.+'
androidTestImplementation 'androidx.test.ext:junit:1.1.2'
androidTestImplementation 'androidx.test.espresso:espresso-core:3.3.0'
}
Empty file added shoplist/consumer-rules.pro
Empty file.
21 changes: 21 additions & 0 deletions shoplist/proguard-rules.pro
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# Add project specific ProGuard rules here.
# You can control the set of applied configuration files using the
# proguardFiles setting in build.gradle.
#
# For more details, see
# http://developer.android.com/guide/developing/tools/proguard.html

# If your project uses WebView with JS, uncomment the following
# and specify the fully qualified class name to the JavaScript interface
# class:
#-keepclassmembers class fqcn.of.javascript.interface.for.webview {
# public *;
#}

# Uncomment this to preserve the line number information for
# debugging stack traces.
#-keepattributes SourceFile,LineNumberTable

# If you keep the line number information, uncomment this to
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

specify the excluded classes list here

# hide the original source file name.
#-renamesourcefileattribute SourceFile
2 changes: 2 additions & 0 deletions shoplist/src/main/AndroidManifest.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
<?xml version="1.0" encoding="utf-8"?>
<manifest package="com.netguru.codereview.shoplist" />
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package com.netguru.codereview.network

import com.netguru.codereview.network.model.ShopListItemResponse
import com.netguru.codereview.network.model.ShopListResponse
import kotlinx.coroutines.flow.Flow

interface ShopListApi {
suspend fun getShopLists(): List<ShopListResponse>
suspend fun getShopListItems(listId: String): List<ShopListItemResponse>
fun getUpdateEvents(): Flow<String>
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
package com.netguru.codereview.network

import com.netguru.codereview.network.model.ShopListItemResponse
import com.netguru.codereview.network.model.ShopListResponse
import kotlinx.coroutines.coroutineScope
import kotlinx.coroutines.delay
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.flow

class ShopListApiMock : ShopListApi {
override suspend fun getShopLists(): List<ShopListResponse> =
coroutineScope {
List(9999) { index ->
ShopListResponse(
list_id = index.toString(),
userId = index,
listName = "ListName$index"
)
}
}

override suspend fun getShopListItems(listId: String): List<ShopListItemResponse> =
coroutineScope {
Thread.sleep(2)
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use delay() couroutines function for delay instead of Thead.sleep()

List(5) { index ->
ShopListItemResponse(
itemId = index.toString(),
name = "Name$index",
quantity = 2.0
)
}
}

override fun getUpdateEvents(): Flow<String> = flow {
var counter = 0
while (true) {
counter++
delay(5000)
emit("Update $counter")
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package com.netguru.codereview.network

class ShopListRepository(private val shopListApi: ShopListApi) {
Copy link
Copy Markdown
Owner Author

@prabuddaFernando prabuddaFernando Mar 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(this is not a must, I just put this comment with comparing to clean code architecture with MVVM)
there is no domain layer. its better to have a interface called ShopListRepository inside the repository list folder in domain folder.
in the network data layer you can use implementation class of it "ShopListRepositoryIml"


suspend fun getShopLists() = shopListApi.getShopLists()

suspend fun getShopListItems(listId: String) = shopListApi.getShopListItems(listId)

fun updateEvents() = shopListApi.getUpdateEvents()
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name is not meaning full. function is returing data. not update events

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package com.netguru.codereview.network.model

import kotlinx.serialization.SerialName
import kotlinx.serialization.Serializable

data class ShopListItemResponse(
val itemId: String,
val name: String,
val quantity: Double
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quantity must be a Integer

)
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package com.netguru.codereview.network.model

import kotlinx.serialization.SerialName
import kotlinx.serialization.Serializable

data class ShopListResponse(
val list_id: String,
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here list_id must be listId
please follow the camel case for local variable, to overcome this issue you can use @SerializedName("list_id") Gson Annotation to specify the variables comes from the api side.

val userId: Int,
val listName: String
)
65 changes: 65 additions & 0 deletions shoplist/src/main/java/com/netguru/codereview/ui/MainFragment.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
package com.netguru.codereview.ui

import android.os.Bundle
import android.util.Log
import android.view.LayoutInflater
import android.view.View
import android.view.ViewGroup
import android.widget.ImageView
import android.widget.ProgressBar
import android.widget.Toast
import androidx.core.view.isVisible
import androidx.fragment.app.Fragment
import androidx.lifecycle.ViewModelProvider
import coil.load
import com.netguru.codereview.network.model.ShopListItemResponse
import com.netguru.codereview.network.model.ShopListResponse
import com.netguru.codereview.shoplist.R
import com.netguru.codereview.ui.model.ShopList
import javax.inject.Inject

class MainFragment : Fragment() {

@Inject
private var viewModel: MainViewModel? = null
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use lateInit and make MainViewModel not null ( remove ?)

and you can remove force null check on line number 36, and 52


override fun onCreateView(
inflater: LayoutInflater, container: ViewGroup?,
savedInstanceState: Bundle?
): View =
layoutInflater.inflate(R.layout.main_fragment, container, false)

override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
super.onViewCreated(view, savedInstanceState)
viewModel = ViewModelProvider(this).get(MainViewModel::class.java)

viewModel!!.shopLists.observe(this, { lists ->
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change the owner of view model to 'viewLifecycleOwner' since it represent the fragmentview`s lifecycle.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move lambda argument out of the parentheses

val progressBar = view.findViewById<ProgressBar>(R.id.message)
Copy link
Copy Markdown
Owner Author

@prabuddaFernando prabuddaFernando Mar 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

widget fields must be class scope and should not init inside observe functions. this assigin must be out side the observe function and inside onViewCreated

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

val latestIcon = view.findViewById(R.id.latest_list_icon) should be the same above

val latestIcon = view.findViewById<ImageView>(R.id.latest_list_icon)

val shopLists = lists.map { mapShopList(it.first, it.second) }.also {
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here application logics and model conversons should not inside the fragment or activities. keep them inside the viewModel.
(move "mapShopList" function inside to the viewModel)

icon set Url also must be inside the ViewModel

latestIcon?.load(it.first().iconUrl)
}

progressBar?.isVisible = false
Copy link
Copy Markdown
Owner Author

@prabuddaFernando prabuddaFernando Mar 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make progress bar visible in init state.
this line is not working, use visibility parameter for visibility change.
progressBar?.visibility = View.GONE


Log.i("LOGTAG", "LOLOLOL Is it done already?")


// Display the list in recyclerview
// adapter.submitList(shopLists)
})
viewModel!!.events().observe(this, {
Toast.makeText(context, it, Toast.LENGTH_SHORT).show()
})
}

private fun mapShopList(list: ShopListResponse, items: List<ShopListItemResponse>) =
ShopList(
list.list_id,
list.userId,
list.listName,
list.listName,
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lastName passes to ShopList -> iconUrl filed
invalid data

items
)
}
43 changes: 43 additions & 0 deletions shoplist/src/main/java/com/netguru/codereview/ui/MainViewModel.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
package com.netguru.codereview.ui

import androidx.lifecycle.LiveData
import androidx.lifecycle.MutableLiveData
import androidx.lifecycle.ViewModel
import androidx.lifecycle.viewModelScope
import com.netguru.codereview.network.ShopListApiMock
import com.netguru.codereview.network.ShopListRepository
import com.netguru.codereview.network.model.ShopListItemResponse
import com.netguru.codereview.network.model.ShopListResponse
import kotlinx.coroutines.GlobalScope
import kotlinx.coroutines.flow.collect
import kotlinx.coroutines.launch

class MainViewModel : ViewModel() {

private val shopListRepository = ShopListRepository(ShopListApiMock())
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add this repository into the constructor in MainViewModel.
private val shopListRepository: ShopListRepository = ShopListRepository(ShopListApiMock())


val shopLists = MutableLiveData<List<Pair<ShopListResponse, List<ShopListItemResponse>>>>()
private val eventLiveData = MutableLiveData<String>()

init {
viewModelScope.launch {
val lists = shopListRepository.getShopLists()
val data = mutableListOf<Pair<ShopListResponse, List<ShopListItemResponse>>>()
for (list in lists) {
val items = shopListRepository.getShopListItems(list.list_id)
data.add(list to items)
}
shopLists.postValue(data)
}
getUpdateEvents()
}

fun events(): LiveData<String> = eventLiveData
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to pass LiveData with function here. just observe the eventLiveData filed from fragment. remove function here. and make eventLiveData public.

private fun getUpdateEvents() {
GlobalScope.launch {
shopListRepository.updateEvents().collect {
eventLiveData.postValue(it)
}
}
}
}
11 changes: 11 additions & 0 deletions shoplist/src/main/java/com/netguru/codereview/ui/model/ShopList.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package com.netguru.codereview.ui.model

import com.netguru.codereview.network.model.ShopListItemResponse

class ShopList(
Copy link
Copy Markdown
Owner Author

@prabuddaFernando prabuddaFernando Mar 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make this class int to a data class.

val id: String,
val userId: Int,
val listName: String,
val iconUrl: String,
val items: List<ShopListItemResponse>
)
44 changes: 44 additions & 0 deletions shoplist/src/main/res/layout/main_fragment.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
<?xml version="1.0" encoding="utf-8"?>
<androidx.constraintlayout.widget.ConstraintLayout xmlns:android="http://schemas.android.com/apk/res/android"
xmlns:app="http://schemas.android.com/apk/res-auto"
android:id="@+id/main"
android:layout_width="match_parent"
android:layout_height="match_parent"
android:padding="8dp">

<TextView
android:id="@+id/title"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:padding="8dp"
android:text="Check out your shopping lists!"
app:layout_constraintStart_toStartOf="parent"
app:layout_constraintTop_toTopOf="parent" />

<androidx.appcompat.widget.AppCompatImageView
android:id="@+id/latest_list_icon"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
app:layout_constraintEnd_toEndOf="parent"
app:layout_constraintStart_toStartOf="parent"
app:layout_constraintTop_toBottomOf="@+id/title" />

<ProgressBar
android:id="@+id/message"
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name must be meaning full. use name like progress_bar

android:layout_width="wrap_content"
android:layout_height="wrap_content"
app:layout_constraintBottom_toBottomOf="parent"
app:layout_constraintEnd_toEndOf="parent"
app:layout_constraintStart_toStartOf="parent"
app:layout_constraintTop_toTopOf="parent" />

<androidx.recyclerview.widget.RecyclerView
android:layout_width="wrap_content"
android:layout_height="match_parent"
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no id for the recyclerview

android:padding="8dp"
app:layout_constraintBottom_toBottomOf="@id/message"
app:layout_constraintEnd_toEndOf="@id/message"
app:layout_constraintStart_toStartOf="@id/message"
app:layout_constraintTop_toBottomOf="@id/latest_list_icon" />

</androidx.constraintlayout.widget.ConstraintLayout>