Code Review Suggestions by derrickadkins · Pull Request #23 · jakeFarrish/MedMapper

derrickadkins · 2023-03-05T21:24:58Z

Description of Project:
This is a medication tracker/logger/reminder app

What I learned:
How to use ConstraintLayout
How to use RecyclerAdapter
How to use Fragments

derrickadkins · 2023-03-05T21:26:19Z

 # MEDMAPPER

-The purpose of this application is to assist a user with medication administration. This application recieves input from the user about their medications and when they should be administered. The application sends alerts and reminders to the user to take their medications at the appropriate time, and logs successful administration for the user as well as other parties that the user may choose to share their information with. This will allow a user to track their medication usage, and help ensure that they are taking each medication at the appropriate time each day.
+The purpose of this application is to assist a user with medication administration. This application receives input from the user about their medications and when they should be administered. The application sends alerts and reminders to the user to take their medications at the appropriate time, and logs successful administration for the user as well as other parties that the user may choose to share their information with. This will allow a user to track their medication usage, and help ensure that they are taking each medication at the appropriate time each day.


Corrected spelling error

derrickadkins · 2023-03-05T21:26:38Z

 SCRUM ROLES:

-  Intergration Developer: Cherissa Tan
+  Integration Developer: Cherissa Tan


Corrected spelling error

derrickadkins · 2023-03-05T21:27:55Z

@@ -1,6 +1,5 @@
 <?xml version="1.0" encoding="utf-8"?>
-<manifest xmlns:android="http://schemas.android.com/apk/res/android"
-    xmlns:tools="http://schemas.android.com/tools">


Removed unused tools

I urge caution making changes to the AndroidManifest.xml. This file describes the program. Most items are here for a reason. Are you certain they are not used? Is it a best practice to remove them?

derrickadkins · 2023-03-05T21:28:20Z

@@ -9,12 +8,10 @@
        android:icon="@mipmap/ic_launcher"
        android:label="@string/app_name"
        android:supportsRtl="true"
-        android:theme="@style/Theme.MedMapper"
-        tools:targetApi="31">


Unnecessary; SDK_INT is always >= 31

derrickadkins · 2023-03-05T21:28:47Z

        <activity
            android:name=".MainActivity"
            android:exported="true"
-            android:label="@string/app_name"


Redundant label can be removed

derrickadkins · 2023-03-05T21:29:57Z

+        setContentView(layoutInflater.inflate(R.layout.main_activity, null))
+        setSupportActionBar(findViewById(R.id.toolbar))
+        setFragment(ScheduleFragment())


initialize layout

There are significant changes to this file. It looks like you're moving it from Jetpack Compose to Fragments. What is the tech debt reduction you achieve with this change?

derrickadkins · 2023-03-05T21:30:49Z

+    override fun onCreateOptionsMenu(menu: Menu?): Boolean {
+        menuInflater.inflate(R.menu.main, menu)
+        return super.onCreateOptionsMenu(menu)
+    }
+
+    override fun onOptionsItemSelected(item: MenuItem): Boolean {
+        when (item.itemId) {
+            R.id.schedule_menu_item -> {
+                setFragment(ScheduleFragment())
+            }
+            R.id.medication_menu_item -> {
+                setFragment(MedicationsFragment())
+            }
+            R.id.share_menu_item -> {
+                //todo: share()
            }
+            else -> return false
        }
+
+        return true
    }


implemented menu options

Menus are not necessarily a good idea.

It's one more click for the user.

It often indicates a multi-screen app, where we try to minimize screens, where possible, to give a simpler experience.

derrickadkins · 2023-03-05T21:31:42Z

+class MedRecyclerAdapter(private val inflater: LayoutInflater, var meds: ArrayList<Medicine>): RecyclerView.Adapter<MedRecyclerAdapter.ViewHolder>() {
+    override fun onCreateViewHolder(parent: ViewGroup, viewType: Int): ViewHolder {
+        return ViewHolder(inflater.inflate(R.layout.medication_line_item, parent, false))
+    }
+
+    override fun getItemCount(): Int {
+        return meds.size
+    }
+
+    override fun onBindViewHolder(holder: ViewHolder, position: Int) {
+        holder.name.text = meds[position].name
+    }
+
+    class ViewHolder(itemView: View) : RecyclerView.ViewHolder(itemView) {
+        val name: TextView = itemView.findViewById(R.id.medication_name)
+    }


added recycler adapter for med list

In Compose, we'd use LazyColumn for repeating lists. RecyclerView is essentially deprecated, except in XML based layouts, which predates Compose.

derrickadkins · 2023-03-05T21:32:01Z

+class MedicationsFragment : Fragment() {
+    lateinit var recyclerView: RecyclerView
+    override fun onCreateView(
+        inflater: LayoutInflater,
+        container: ViewGroup?,
+        savedInstanceState: Bundle?
+    ): View? {
+        val view = inflater.inflate(R.layout.medications_fragment, container, false)
+        recyclerView = view.findViewById(R.id.medications)
+        recyclerView.layoutManager = LinearLayoutManager(context)
+        recyclerView.adapter = MedRecyclerAdapter(inflater, ArrayList())
+        return view
+    }


added fragment for med list

derrickadkins · 2023-03-05T21:32:15Z

+class ScheduleFragment: Fragment() {
+    lateinit var email: EditText
+    lateinit var schedule: TextView
+    override fun onCreateView(
+        inflater: LayoutInflater,
+        container: ViewGroup?,
+        savedInstanceState: Bundle?
+    ): View? {
+        val view = inflater.inflate(R.layout.schedule_fragment, container, false)
+        schedule = view.findViewById(R.id.schedule)
+        email = view.findViewById(R.id.editTextTextEmailAddress)
+        return view
+    }


added fragment for schedule

derrickadkins · 2023-03-05T21:32:34Z

 import com.medmapper.v33001.dto.Medicine

-@Database(entities = arrayOf(Medicine::class), version = 1, exportSchema = false)
+@Database(entities = [Medicine::class], version = 1, exportSchema = false)


'arrayOf' call should be replaced with array literal [...]

This change was covered in a previously submitted review.

derrickadkins · 2023-03-05T21:32:56Z

+<vector xmlns:android="http://schemas.android.com/apk/res/android"
+    android:width="48dp"
+    android:height="48dp"
+    android:viewportWidth="960"
+    android:viewportHeight="960"
+    android:tint="?attr/colorControlNormal">
+  <path
+      android:fillColor="@android:color/white"
+      android:pathData="M345,840Q251,840 185.5,774.5Q120,709 120,615Q120,570 137,529Q154,488 186,456L456,186Q488,154 529,137Q570,120 615,120Q709,120 774.5,185.5Q840,251 840,345Q840,390 823,431Q806,472 774,504L504,774Q472,806 431,823Q390,840 345,840ZM618,575L732,462Q755,439 767.5,408.5Q780,378 780,345Q780,276 732,228Q684,180 615,180Q582,180 551.5,192.5Q521,205 498,228L385,342L618,575ZM345,780Q377,780 408,767.5Q439,755 462,732L575,618L342,385L228,498Q205,521 192.5,551.5Q180,582 180,615Q180,684 228,732Q276,780 345,780Z"/>


added pill icon drawable

derrickadkins · 2023-03-05T21:33:15Z

+<vector xmlns:android="http://schemas.android.com/apk/res/android"
+    android:width="48dp"
+    android:height="48dp"
+    android:viewportWidth="960"
+    android:viewportHeight="960"
+    android:tint="?attr/colorControlNormal">
+  <path
+      android:fillColor="@android:color/white"
+      android:pathData="M627,673L672,628L513,468L513,267L453,267L453,492L627,673ZM480,880Q398,880 325,848.5Q252,817 197.5,762.5Q143,708 111.5,635Q80,562 80,480Q80,398 111.5,325Q143,252 197.5,197.5Q252,143 325,111.5Q398,80 480,80Q562,80 635,111.5Q708,143 762.5,197.5Q817,252 848.5,325Q880,398 880,480Q880,562 848.5,635Q817,708 762.5,762.5Q708,817 635,848.5Q562,880 480,880ZM480,480Q480,480 480,480Q480,480 480,480Q480,480 480,480Q480,480 480,480Q480,480 480,480Q480,480 480,480Q480,480 480,480Q480,480 480,480ZM480,820Q620,820 720,720Q820,620 820,480Q820,340 720,240Q620,140 480,140Q340,140 240,240Q140,340 140,480Q140,620 240,720Q340,820 480,820Z"/>


added schedule icon drawable

derrickadkins · 2023-03-05T21:33:30Z

+    android:width="48dp"
+    android:height="48dp"
+    android:viewportWidth="960"
+    android:viewportHeight="960"
+    android:tint="?attr/colorControlNormal">
+  <path
+      android:fillColor="@android:color/white"
+      android:pathData="M727,880Q679.5,880 646.25,846.65Q613,813.31 613,765.67Q613,759 614.5,749.36Q616,739.71 619,732L316,556Q301,573 279,583.5Q257,594 234,594Q186.5,594 153.25,560.75Q120,527.5 120,480Q120,432.5 153.25,399.25Q186.5,366 234,366Q257,366 278,375Q299,384 316,401L619,227Q616,219.93 614.5,211.09Q613,202.25 613,194Q613,146.5 646.25,113.25Q679.5,80 727,80Q774.5,80 807.75,113.25Q841,146.5 841,194Q841,241.5 807.75,274.75Q774.5,308 727,308Q703.65,308 682.32,300.5Q661,293 646,276L343,444Q345,452 346.5,462.5Q348,473 348,480.24Q348,487.48 346.5,495.24Q345,503 343,511L646,683Q661,669 681,660.5Q701,652 727,652Q774.5,652 807.75,685.25Q841,718.5 841,766Q841,813.5 807.75,846.75Q774.5,880 727,880ZM727.03,248Q750,248 765.5,232.46Q781,216.93 781,193.96Q781,171 765.47,155.5Q749.93,140 726.97,140Q704,140 688.5,155.54Q673,171.07 673,194.04Q673,217 688.53,232.5Q704.07,248 727.03,248ZM234.04,534Q257,534 272.5,518.47Q288,502.93 288,479.97Q288,457 272.46,441.5Q256.93,426 233.96,426Q211,426 195.5,441.53Q180,457.07 180,480.03Q180,503 195.54,518.5Q211.07,534 234.04,534ZM727.03,820Q750,820 765.5,804.47Q781,788.93 781,765.97Q781,743 765.47,727.5Q749.93,712 726.97,712Q704,712 688.5,727.53Q673,743.07 673,766.03Q673,789 688.53,804.5Q704.07,820 727.03,820ZM727,194Q727,194 727,194Q727,194 727,194Q727,194 727,194Q727,194 727,194Q727,194 727,194Q727,194 727,194Q727,194 727,194Q727,194 727,194ZM234,480Q234,480 234,480Q234,480 234,480Q234,480 234,480Q234,480 234,480Q234,480 234,480Q234,480 234,480Q234,480 234,480Q234,480 234,480ZM727,766Q727,766 727,766Q727,766 727,766Q727,766 727,766Q727,766 727,766Q727,766 727,766Q727,766 727,766Q727,766 727,766Q727,766 727,766Z"/>


added share icon drawable

derrickadkins · 2023-03-05T21:34:29Z

@@ -0,0 +1,33 @@
+<?xml version="1.0" encoding="utf-8"?>
+<androidx.constraintlayout.widget.ConstraintLayout


ConstraintLayout is better for flexible UI

What tech debt reduction do you achieve in moving from Jetpack Compose to an XML based layout?

derrickadkins · 2023-03-05T21:34:50Z

+    <com.google.android.material.appbar.AppBarLayout
+        android:id="@+id/app_bar_layout"
+        android:layout_width="match_parent"
+        android:layout_height="wrap_content"
+        app:layout_constraintTop_toTopOf="parent">
+
+        <com.google.android.material.appbar.MaterialToolbar
+            android:id="@+id/toolbar"
+            app:menu="@menu/main"
+            app:title="@string/name_here"
+            app:titleTextColor="@android:color/white"
+            android:layout_width="match_parent"
+            android:layout_height="?attr/actionBarSize" >
+
+        </com.google.android.material.appbar.MaterialToolbar>
+
+    </com.google.android.material.appbar.AppBarLayout>


replaced clunky buttons with app bar

derrickadkins · 2023-03-05T21:35:56Z

+    <FrameLayout
+        android:id="@+id/fragment_view"
+        app:layout_constraintTop_toBottomOf="@id/app_bar_layout"
+        app:layout_constraintBottom_toBottomOf="parent"
+        android:layout_width="match_parent"
+        android:layout_height="0dp"/>


use fragments for different views inside an activity

derrickadkins · 2023-03-05T21:36:57Z

+<androidx.constraintlayout.widget.ConstraintLayout xmlns:android="http://schemas.android.com/apk/res/android"
+    android:layout_width="match_parent"
+    android:layout_height="50dp"
+    xmlns:app="http://schemas.android.com/apk/res-auto">
+    <TextView
+        android:id="@+id/medication_name"
+        android:text="@string/medication_name"
+        app:layout_constraintStart_toStartOf="parent"
+        app:layout_constraintTop_toTopOf="parent"
+        android:layout_width="wrap_content"
+        android:layout_height="wrap_content"/>
+    <TextView
+        android:id="@+id/prescription_date"
+        app:layout_constraintStart_toStartOf="parent"
+        app:layout_constraintTop_toBottomOf="@id/medication_name"
+        android:layout_width="wrap_content"
+        android:layout_height="wrap_content"/>


added layout for med recycler view item

derrickadkins · 2023-03-05T21:37:20Z

+<androidx.constraintlayout.widget.ConstraintLayout xmlns:android="http://schemas.android.com/apk/res/android"
+    android:layout_width="match_parent"
+    android:layout_height="match_parent">
+
+    <androidx.recyclerview.widget.RecyclerView
+        android:id="@+id/medications"
+        android:layout_width="match_parent"
+        android:layout_height="match_parent" />
+


added fragment layout for med list

derrickadkins · 2023-03-05T21:38:58Z

+<androidx.constraintlayout.widget.ConstraintLayout xmlns:android="http://schemas.android.com/apk/res/android"
+    android:layout_width="match_parent"
+    android:layout_height="match_parent"
+    xmlns:app="http://schemas.android.com/apk/res-auto">
+
+    <TextView
+        android:id="@+id/schedule_label"
+        android:layout_width="wrap_content"
+        android:layout_height="wrap_content"
+        android:layout_margin="24dp"
+        app:layout_constraintTop_toTopOf="parent"
+        app:layout_constraintStart_toStartOf="parent"
+        android:text="@string/schedule_"
+        android:textColor="#000000"
+        android:textSize="24sp"
+        android:textStyle="bold" />
+
+    <TextView
+        android:id="@+id/schedule"
+        android:layout_width="match_parent"
+        android:layout_height="0dp"
+        android:layout_margin="24dp"
+        app:layout_constraintBottom_toTopOf="@id/editTextTextEmailAddress"
+        app:layout_constraintTop_toBottomOf="@id/schedule_label" />
+
+    <EditText
+        android:id="@+id/editTextTextEmailAddress"
+        android:layout_width="match_parent"
+        android:layout_height="48dp"
+        android:layout_margin="24dp"
+        android:hint="@string/enter_email_of_recipient"


extracted schedule view from layout into separate fragment

derrickadkins · 2023-03-05T21:39:24Z

+<menu xmlns:android="http://schemas.android.com/apk/res/android"
+    xmlns:app="http://schemas.android.com/apk/res-auto">
+
+    <item
+        android:id="@+id/schedule_menu_item"
+        android:title="@string/schedule"
+        android:icon="@drawable/schedule"
+        app:showAsAction="always"/>
+    <item
+        android:id="@+id/medication_menu_item"
+        android:title="@string/medication"
+        android:icon="@drawable/pill"
+        app:showAsAction="always"/>
+    <item
+        android:id="@+id/share_menu_item"
+        android:title="@string/share"
+        android:icon="@drawable/share"
+        app:showAsAction="always"/>


replaced clunky nav buttons with menu

derrickadkins · 2023-03-05T21:39:57Z

+    <string name="schedule_">Schedule:</string>
    <string name="enter_email_of_recipient">Enter Email of Recipient</string>
+    <string name="schedule">Schedule</string>
+    <string name="medication">Medication</string>
+    <string name="share">Share</string>
+    <string name="email">Email</string>
+    <string name="medication_name">Medication Name</string>


added string resources for UI

derrickadkins · 2023-03-05T21:42:40Z

I recommend merging these changes

discospiff · 2023-03-05T22:12:21Z

+    implementation 'androidx.constraintlayout:constraintlayout:2.1.4'
+    implementation 'com.google.android.material:material:1.8.0'


Are these used?

discospiff · 2023-03-05T22:21:15Z

From this review, I'd harvest fixing spelling errors, which can be done manually. Otherwise, I don't recommend merging this branch.

There are a lot of changes in this review, but it's not clear on how they reduce technical debt.

It appears that the reviewer is removing the newer Jetpack Compose UI, and replacing it with the older XML based UI.
There are not best practice sources cited, or a discussion of how tech debt is reduced.

Code Review Suggestions

b3846fa

derrickadkins commented Mar 5, 2023

View reviewed changes

discospiff reviewed Mar 5, 2023

View reviewed changes

		@@ -0,0 +1,33 @@
		<?xml version="1.0" encoding="utf-8"?>
		<androidx.constraintlayout.widget.ConstraintLayout

		implementation 'androidx.constraintlayout:constraintlayout:2.1.4'
		implementation 'com.google.android.material:material:1.8.0'

Conversation

derrickadkins commented Mar 5, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derrickadkins commented Mar 5, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

discospiff commented Mar 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants