-
Notifications
You must be signed in to change notification settings - Fork 0
feature branch commit #1
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: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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() | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,3 @@ | ||
| include ':shoplist' | ||
| include ':app' | ||
| rootProject.name = "Code Review" |
| 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' | ||
| } |
| 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 | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
| 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) | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) |
||
|
|
||
| suspend fun getShopLists() = shopListApi.getShopLists() | ||
|
|
||
| suspend fun getShopListItems(listId: String) = shopListApi.getShopListItems(listId) | ||
|
|
||
| fun updateEvents() = shopListApi.getUpdateEvents() | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. here list_id must be listId |
||
| val userId: Int, | ||
| val listName: String | ||
| ) | ||
| 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 | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 -> | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. icon set Url also must be inside the ViewModel |
||
| latestIcon?.load(it.first().iconUrl) | ||
| } | ||
|
|
||
| progressBar?.isVisible = false | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. make progress bar visible in init state. |
||
|
|
||
| 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, | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lastName passes to ShopList -> iconUrl filed |
||
| items | ||
| ) | ||
| } | ||
| 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()) | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add this repository into the constructor in MainViewModel. |
||
|
|
||
| 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 | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| 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( | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> | ||
| ) | ||
| 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" | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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" | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
put minifyEnable true in release mode only. create separate debug mode for development (security concern)