Changes request

app/src/main/java/com/hifeful/mealmania/domain/model/Meal.kt

@@ -6,6 +6,18 @@ import androidx.room.Entity
 import androidx.room.PrimaryKey
 import kotlinx.parcelize.Parcelize
 
+/**
+ *  1️⃣ It should be inside the data layer as it's a database entity(i.e., MealEntity)
+ *  2️⃣ In the domain layer you are going to have a model (i.e., Meal) which is mapped
+ *     in the data layer based on received entity
+ *  3️⃣ Domain has only models and repository contracts
+ *     In some cases, use-cases as well
+ *
+ *  🎁 When we have multi-modular architecture:
+ *     > the domain layer shouldn't have any dependencies, even to another domain
+ *     > the data layer should have only the dedicated domain dependency, and no another data layers
+ *     > of course, there might be exception, but usually we should avoid it
+ */
 @Parcelize
 @Entity(tableName = "meal")
 data class Meal(

app/src/main/java/com/hifeful/mealmania/presentation/myMeals/FavouriteMealsAdapter.kt

@@ -13,6 +13,9 @@ class FavouriteMealsAdapter : ListAdapter<Meal, FavouriteMealsAdapter.FavouriteM
     MealsDiffCallback()
 ) {
 
+    // It's not a listener, it's just a lambda expression.
+    // So, it would be better to have:
+    //     val onClickMeal: (String) -> Unit = {}
     var onMealClickListener: ((String) -> Unit)? = null
 
     override fun onCreateViewHolder(parent: ViewGroup, viewType: Int): FavouriteMealsViewHolder =
@@ -32,6 +35,7 @@ class FavouriteMealsAdapter : ListAdapter<Meal, FavouriteMealsAdapter.FavouriteM
 
         fun bind(meal: Meal) {
             with(binding) {
+                // Then, you could omit .invoke() and just use ()
                 root.setOnClickListener { onMealClickListener?.invoke(meal.id) }
                 imageViewFavouriteMeal.loadUrlCircleCrop(meal.thumbnail)
                 textViewFavouriteMealName.text = meal.name

app/src/main/java/com/hifeful/mealmania/presentation/myMeals/MyMealsFeature.kt

@@ -10,6 +10,52 @@ import io.reactivex.android.schedulers.AndroidSchedulers
 import io.reactivex.schedulers.Schedulers
 import javax.inject.Inject
 
+/**
+ * 💢 Questions
+ * 🟡 Please, answer on them before moving further and after changes would be made
+ * 1️⃣ How the configuration changes are handled with Feature (if so)?
+ * 2️⃣ Can we have MVI using ViewModel or Presenter?
+ * 3️⃣ Could the race conditions issue happen
+ *
+ * IMO, I don't think that the implementation of MVI should be like this.
+ * The architecture is the very crucial pilar for any app.
+ * Usually, the implementation is unique for a project as there is no the strict rules how to do it.
+ *
+ * 1️⃣ You are relying on the third-party framework which is used presumably in the closed group
+ *    of people. Meaning we are as externals won't know about actual motivation and appetite of
+ *    existed functionality and further updates of the framework.
+ *    Doing so, we explicitly agree with those limitation where we won;t be able to customize it.
+ *    The workarounds would appear. Plus, imagine the owner company is closed, and the framework
+ *    won't be supported. So, your app become outdated pretty fast. 🧐
+ *
+ * 2️⃣ Years ago, I've used Mavericks framework in one of pet projects. But there we decided
+ *    to do so as the Kotlin language wasn't powerful at that moment. But now it's pretty stable with
+ *    having Flows and Channels. Or... it's possible to use Rx instead where we have Observable and
+ *    PublishSubject. It's up to you what to use - Rx or Kotlin, but I would recommend to focus on
+ *    Kotlin 😉
+ *
+ *    Material:
+ *    💠 "Implementation drafts with using only Kotlin"
+ *       https://betterprogramming.pub/all-you-need-for-mvi-is-kotlin-how-to-reduce-without-reducer-5e986856610f
+ *
+ *    💢 But... please, check the second material as it would prevent writing a wrong solution
+ *    💠 "THIS Is the #1 Clean Code Mistake"
+ *       https://www.youtube.com/watch?v=jYetmv0xRRI
+ *
+ *    🙏 We shouldn't use base classes in all costs
+ *
+ * 3️⃣ Having Feature instead of ViewModel/Presenter makes confuse 😥. Usually, we are as an Android
+ *    engineers prefer work with the intermediate presentation component - ViewModel or Presenter only.
+ *    I don't think it's a good thing to keep alive as it increases the learning curve.
+ *    We should keep things as simple as possible. So, we can-should have VM or Presenter with
+ *    the implementation of MVI concept. Try to consider it as an upgrade/level-up of VM or Presenter.
+ *
+ *    Material:
+ *    💠 Not theoretical comparison of MVVM and MVI
+ *       https://www.youtube.com/watch?v=cnU2zMnmmpg
+ *
+ * 4️⃣ Regarding the naming convention we should stick to Event, State, SideEffect (if needed)
+ */
 class MyMealsFeature @Inject constructor(
     initialState: State,
     actor: Actor<State, Wish, Effect>,
@@ -19,7 +65,11 @@ class MyMealsFeature @Inject constructor(
         MyMealsFeature.State,
         Nothing>(initialState = initialState, actor = actor, reducer = reducer) {
 
+    // Should be extracted to outside 😉
+    // The same for Wish, Effect, etc.
     data class State(
+        // State should expose UI models or primitive types.
+        // Or UI model that holds the domain model
         val recentMeals: List<Meal>? = null,
         val favouriteMeals: List<Meal>? = null
     )
@@ -34,6 +84,8 @@ class MyMealsFeature @Inject constructor(
         data class LoadedFavouriteMeals(val meals: List<Meal>) : Effect()
     }
 
+    // Wish and Effect types seem are the same from the syntax sense at least.
+    // In other words, kinda of duplication that causes the complexity.
     class ActorImpl(
         private val mealsRepository: MealsRepository
     ) : Actor<State, Wish, Effect> {

app/src/main/java/com/hifeful/mealmania/presentation/myMeals/MyMealsFragment.kt

@@ -15,12 +15,25 @@ import dagger.hilt.android.AndroidEntryPoint
 import io.reactivex.functions.Consumer
 import javax.inject.Inject
 
+/**
+ * 1️⃣ Please, avoid custom base classes. The exceptions are:
+ *    👌 Framework obligation: Activity, Fragment, ViewModel, etc.
+ *    👌 Data classes modeling
+ *
+ * 💢 The architectural components (generated by us) mustn't rely on the inheritance
+ */
 @AndroidEntryPoint
 class MyMealsFragment : ObservableSourceFragment<MyMealsUiEvent>(), Consumer<MyMealsViewState> {
 
     @Inject
     lateinit var myMealsFeature: MyMealsFeature
 
+    /**
+     *  1️⃣ Things might be improved to avoid having the same two variables
+     *     https://zhuinden.medium.com/simple-one-liner-viewbinding-in-fragments-and-activities-with-kotlin-961430c6c07c
+     *  2️⃣ The not-null assertion operator (!!) mustn't be used in the code.
+     *     The only place where it's allowed is unit-tests.
+     */
     private var _binding: FragmentMyMealsBinding? = null
     private val binding get() = _binding!!
 
@@ -45,6 +58,10 @@ class MyMealsFragment : ObservableSourceFragment<MyMealsUiEvent>(), Consumer<MyM
     override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
         super.onViewCreated(view, savedInstanceState)
 
+        /**
+         *  Do you really need the separate setup MyMealsFragmentBindings?
+         *  I didn't get what is the benefit of it
+         */
         val bindings = MyMealsFragmentBindings(this, myMealsFeature)
         bindings.setup(this)
         setUpViews()
@@ -54,7 +71,11 @@ class MyMealsFragment : ObservableSourceFragment<MyMealsUiEvent>(), Consumer<MyM
 
     private fun setUpViews() {
         recentMealsAdapter = RecentMealsAdapter().apply {
-            onMealClickListener = { attachMealDetailsFragment(it) }
+            onMealClickListener = {
+                // You should fire Event to ViewModel/Presenter where it will be handled with
+                // SideEffect outcome (i.e., doing the navigation)
+                attachMealDetailsFragment(it)
+            }
         }
         binding.recyclerViewRecentMeals.apply {
             adapter = recentMealsAdapter
@@ -76,6 +97,10 @@ class MyMealsFragment : ObservableSourceFragment<MyMealsUiEvent>(), Consumer<MyM
         }
     }
 
+    /**
+     *  Not sure I am understand it. It should be a part of ViewModel/Presenter, no?
+     *  The state updates should be only allowed in VM/P and during events processing
+     */
     private fun setUpLoading() {
         onNext(MyMealsUiEvent.LoadRecentMeals)
         onNext(MyMealsUiEvent.LoadFavouriteMeals)
@@ -95,6 +120,7 @@ class MyMealsFragment : ObservableSourceFragment<MyMealsUiEvent>(), Consumer<MyM
         _binding = null
     }
 
+    // What's the purpose of this block?
     override fun accept(viewState: MyMealsViewState) {
         Log.d("MyMealsFragment", viewState.favouriteMeals.toString())
         binding.bind(viewState, recentMealsAdapter, favouriteMealsAdapter)

app/src/main/java/com/hifeful/mealmania/presentation/myMeals/MyMealsFragmentBinder.kt

@@ -8,7 +8,47 @@ fun FragmentMyMealsBinding.bind(
     favouriteMealsAdapter: FavouriteMealsAdapter?
 ) {
     recentMealsAdapter?.submitList(viewState.recentMeals)
+    /**
+     *  1️⃣ We shouldn't use .post or .postDelayed anymore
+     *
+     *     Materials:
+     *     💠 "Stop Using Post/PostDelayed in Your Android Views"
+     *        https://betterprogramming.pub/stop-using-post-postdelayed-in-your-android-views-9d1c8eeaadf2
+     */
     recyclerViewFavouriteMeals.post {
+        /**
+         *  2️⃣ Might be the buggy behavior if keep it as is.
+         *     For example, if the data is still loading and the call-site is executed
+         *
+         *     Materials:
+         *     💠 "When to scroll to a certain position?"
+         *        https://dev.to/aldok/how-to-scroll-recyclerview-to-a-certain-position-5ck4
+         *
+         *     In my personal experience, I've done in the following way:
+         *     1️⃣ introduced two functions
+         *
+                    fun itemRangeChangedObserver(block: () -> Unit?) =
+                        object : RecyclerView.AdapterDataObserver() {
+
+                            override fun onItemRangeChanged(positionStart: Int, itemCount: Int) {
+                                block() ?: super.onItemRangeChanged(positionStart, itemCount)
+                            }
+                        }
+
+                    fun itemRangeInsertedObserver(block: () -> Unit?) =
+                        object : RecyclerView.AdapterDataObserver() {
+
+                            override fun onItemRangeInserted(positionStart: Int, itemCount: Int) {
+                                block() ?: super.onItemRangeInserted(positionStart, itemCount)
+                            }
+                        }
+                2️⃣ added an observer to RecyclerView.Adapter
+                    RecentMealsAdapter().apply {
+                        registerAdapterDataObserver(
+                            itemRangeInsertedObserver { binding.content.scrollToPosition(0) }
+                        )
+                    }
+         */
         recyclerViewFavouriteMeals.smoothScrollToPosition(0)
     }
     favouriteMealsAdapter?.submitList(viewState.favouriteMeals)

app/src/main/java/com/hifeful/mealmania/presentation/myMeals/MyMealsUiEvent.kt

@@ -1,5 +1,6 @@
 package com.hifeful.mealmania.presentation.myMeals
 
+// Better to keep it as the sealed interface
 sealed class MyMealsUiEvent {
 
     object LoadRecentMeals : MyMealsUiEvent()