My code review suggestions: grisbyAr by grisbyar · Pull Request #24 · jakeFarrish/MedMapper

grisbyar · 2023-03-06T00:35:06Z

Analysis of the program:

This program is a medicine tracking application that uses a database to store the medications one is taking at a given time. A lot of functionality can be garnered out of this already. The app uses a DAO, Room Database, and a view model.

Was the program available in UC GitHub on time?

Yes.

Is the program documented/commented well enough for you to understand?

Yes, the program is documented and commented well enough to understand. Names of functions, placement, and separation in the program are all self explanatory and there are comments when needed.

Does the program compile?

The program built successfully. The program did not run successfully. I made some changes which I ignored pushing to get it to run properly. It seemed it was a problem with the build.gradle?

Rationale behind changes

The suggestions given are mainly to clear functionality up and keep the codebase clean and readable when it comes to improvement. The other things I've added was for database normalization/sanitation. The other suggestions I've given are additional functionality which would be useful.

Specific Suggestions to Keep in Mind:
Database normalization and sanitization should be a priority with the database. Keeping these handled will keep a clean app and issues from coming up.
Error Handling - Error handling should be including as you are interfacing with another language(SQLite), which might not handle errors well. An error handling suggestion for insertion of data has been added.
Check database functions for opportunities to make them async and use coroutines.
UI suggestions:
Color selection - The UI is already extremely accessible for a healthcare app.
Consider options like blue, green, white, lavender, pink, and grey - all options which are soothing/calming and commonly used in healthcare applications.
Consider a clear and readable font, and accessibility options -> depending on the type of user, they could be looking for something easy to read.

Sources:

1. https://developer.android.com/topic/architecture
2. https://kotlinlang.org/docs/coding-conventions.html#function-names
3. https://github.com/futurice/android-best-practices
4. https://docs.sonarqube.org/latest/user-guide/clean-as-you-code/
5. https://unity-technologies.github.io/kotlin-guide/
6. https://www.ego-cms.com/post/great-healthcare-app-design

Links to three specific commits that you made to your own group's GitHub repository before the end of sprint deadline.

1 Commit
2 Commit
3 Commit

Specific technical concepts that you learned from this code review:

-@volatile - in a singleton, makes sure the instance variable is always up to date and read from main memory, because the variable can be modified differently
-Type Flow variable - Flow is a part of the coroutine library that can do a lot of things. It defines a datastream of different objects asynchronously, I haven't seen this format before: Flow<List<Medicine>>
-Use of SQLite in terms of a function in Kotlin
-@composable - declares a UI function, the greeting is a nice touch

grisbyar · 2023-03-06T00:36:08Z


-@Composable
-fun Greeting(name: String) {
+val Greeting: @Composable (String) -> Unit = { name ->


Changed this to a Lambda - this looks neater
We could have probably added a default value for first time use, and then replaced with the name every time afterwards, after this functionality is implemented.

I kinda like it the way it was. Putting all of that on one line is a lot of symbols... and the @composable annotation is hidden, where it is usually on top of a Composable function.

Putting too much on one line can make debugging tricky.

grisbyar · 2023-03-06T00:37:37Z

 interface MedicineDAO {
    @Query("SELECT * FROM Medicine ORDER BY name ASC")
-    fun getAlphabetizedMedicine(): Flow<List<Medicine>>
+    fun getAlphabetizedMedicine() : Flow<List<Medicine>>


In the style guide, only type declarations should have a colon and no space.

Formatting can, and should, be performed automatically, and consistently, by the IDE. Formatting code is really programmer's preference anyway, and doesn't necessarily decrease technical debt.

grisbyar · 2023-03-06T00:40:09Z

 data class Medicine(
-    @PrimaryKey(autoGenerate = true) val medID: Int,
-    @ColumnInfo(name = "name") val name: String?,
+    @PrimaryKey(autoGenerate = true) var medID: Int,


My mistake, confused mutable var vs val.

Keep this as val. That's the default. Only make var if it will be reassigned.

grisbyar · 2023-03-06T00:40:58Z

    @ColumnInfo(name = "strength") val strength: String?,
    @ColumnInfo(name = "start date") val startDate: String?,
-    @ColumnInfo(name = "prescription length") val lengthInDays: Int
+    @ColumnInfo(name = "prescription length") val lengthInDays: Int?,


Suggesting Nullable - this might not be filled out or an indefinite prescription

Assume not nullable. That is safer.

Only make something nullable if you know it will be nullable, and have proper handling around nullability throughout.

grisbyar · 2023-03-06T00:42:08Z

-    @ColumnInfo(name = "prescription length") val lengthInDays: Int
+    @ColumnInfo(name = "prescription length") val lengthInDays: Int?,
+    @ColumnInfo(name = "frequency") val frequency: String?,
+    @ColumnInfo(name = "reason") val reason: String?


Added Frequency and Reason columns -> This could be useful for people who take medications twice a day, evening or night, or as needed, etc. Added Reason so this could be used as a guide when people talk to their doctors and not have to keep track of why they were prescribed what.

This looks like an incomplete feature.
Are you using these anywhere?

Recommendation: if not used, remove it, or keep it in a separate development branch until it is complete. Keep the codebase as small as possible to do the job you want to do. Unused code is clutter, and bad documentation, which increases technical debt.
Only complete features, which meet the team's Definition of Done, should be merged to Main/Master.

grisbyar · 2023-03-06T00:42:39Z

    @WorkerThread
-    suspend fun insert(medicine: Medicine) {
-        medicineDAO.insert(medicine)
+    suspend fun insertMedicine(medicine: Medicine) {


Added Try Catch here - suggesting that there shouldn't be any errant entries in the database.

Logging is good.

grisbyar · 2023-03-06T00:43:15Z

 import com.medmapper.v33001.dto.Medicine

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


Suggested changing to Array Literal -> This is more readable and concise

grisbyar · 2023-03-06T00:44:27Z

 public abstract class MedicineRoomDatabase : RoomDatabase() {

-    abstract fun medicineDao(): MedicineDAO
+    abstract suspend fun medicineDao(): MedicineDAO


Suggested added coroutine suspend here, performing this database call asynchronously should help with performance

grisbyar · 2023-03-06T00:44:51Z

        fontWeight = FontWeight.Normal,
        fontSize = 16.sp
    )
-    /* Other default text styles to override


Suggesting removing comments, these are noise.

grisbyar · 2023-03-06T00:45:18Z


    <EditText
        android:id="@+id/editTextTextEmailAddress"
+        android:gravity="center"


Suggesting adding centering for this element, looks cleaner

grisbyar · 2023-03-06T00:45:37Z

        android:id="@+id/textView2"
        android:layout_width="200dp"
        android:layout_height="99dp"
+        android:gravity="right"


Suggesting adding right alignment for this element, looks cleaner

discospiff · 2023-03-06T02:00:08Z

From this review, I'd harvest logging and removing commented out code.

I would skip the other suggestions, though, and I would not merge the branch itself, as there's not significant tech debt reduction to warrant a merge.

My code review suggestions: grisbyAr

6cc7b6e

grisbyar commented Mar 6, 2023

View reviewed changes

Conversation

grisbyar commented Mar 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Analysis of the program:

Was the program available in UC GitHub on time?

Is the program documented/commented well enough for you to understand?

Does the program compile?

Rationale behind changes

Sources:

Links to three specific commits that you made to your own group's GitHub repository before the end of sprint deadline.

Specific technical concepts that you learned from this code review:

Uh oh!

grisbyar Mar 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

discospiff commented Mar 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

grisbyar commented Mar 6, 2023 •

edited

Loading

grisbyar Mar 6, 2023 •

edited

Loading