Skip to content

My code review suggestions: grisbyAr #24

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.

Sign up for GitHub

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

Open
grisbyar wants to merge 1 commit into
base: master
Choose a base branch
from
Open

My code review suggestions: grisbyAr #24

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 1 addition & 2 deletions app/src/main/java/com/medmapper/v33001/MainActivity.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,7 @@ class MainActivity : ComponentActivity() {
}
}

@Composable
fun Greeting(name: String) {
val Greeting: @Composable (String) -> Unit = { name ->
Copy link
Copy Markdown
Collaborator Author

@grisbyar grisbyar Mar 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown

@discospiff discospiff Mar 6, 2023

Choose a reason for hiding this comment

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

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.

Text(text = "Hello $name!")
}

Expand Down
2 changes: 1 addition & 1 deletion app/src/main/java/com/medmapper/v33001/dao/MedicineDAO.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import kotlinx.coroutines.flow.Flow
@Dao
interface MedicineDAO {
@Query("SELECT * FROM Medicine ORDER BY name ASC")
fun getAlphabetizedMedicine(): Flow<List<Medicine>>
fun getAlphabetizedMedicine() : Flow<List<Medicine>>
Copy link
Copy Markdown
Collaborator Author

@grisbyar grisbyar Mar 6, 2023

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown

@discospiff discospiff Mar 6, 2023

Choose a reason for hiding this comment

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

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.


@Insert(onConflict = OnConflictStrategy.IGNORE)
suspend fun insert(medicine: Medicine)
Expand Down
8 changes: 5 additions & 3 deletions app/src/main/java/com/medmapper/v33001/dto/Medicine.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,12 @@ import androidx.room.PrimaryKey

@Entity(tableName = "Medicine")
data class Medicine(
@PrimaryKey(autoGenerate = true) val medID: Int,
@ColumnInfo(name = "name") val name: String?,
@PrimaryKey(autoGenerate = true) var medID: Int,
Copy link
Copy Markdown
Collaborator Author

@grisbyar grisbyar Mar 6, 2023

Choose a reason for hiding this comment

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

My mistake, confused mutable var vs val.

Copy link
Copy Markdown

@discospiff discospiff Mar 6, 2023

Choose a reason for hiding this comment

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

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

@ColumnInfo(name = "name") val name: String,
@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?,
Copy link
Copy Markdown
Collaborator Author

@grisbyar grisbyar Mar 6, 2023

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown

@discospiff discospiff Mar 6, 2023

Choose a reason for hiding this comment

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

Assume not nullable. That is safer.

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

@ColumnInfo(name = "frequency") val frequency: String?,
@ColumnInfo(name = "reason") val reason: String?
Copy link
Copy Markdown
Collaborator Author

@grisbyar grisbyar Mar 6, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown

@discospiff discospiff Mar 6, 2023

Choose a reason for hiding this comment

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

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.

//,@ColumnInfo() val endDate: Date = startDate
)
13 changes: 10 additions & 3 deletions app/src/main/java/com/medmapper/v33001/repository/MedicineRepository.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
package com.medmapper.v33001.repository

import android.util.Log
import androidx.annotation.WorkerThread
import com.medmapper.v33001.dao.MedicineDAO
import com.medmapper.v33001.dto.Medicine
import kotlinx.coroutines.flow.Flow

// Declares teh DAO as private property in the constructor. Pass in the DAO
// Declares the DAO as private property in the constructor. Pass in the DAO
// instead of the whole database, because you only need access to the DAO
class MedicineRepository(private val medicineDAO: MedicineDAO) {

Expand All @@ -18,7 +19,13 @@ class MedicineRepository(private val medicineDAO: MedicineDAO) {
// long running database work off the main thread
@Suppress("RedundantSuspendModifier")
@WorkerThread
suspend fun insert(medicine: Medicine) {
medicineDAO.insert(medicine)
suspend fun insertMedicine(medicine: Medicine) {
Copy link
Copy Markdown
Collaborator Author

@grisbyar grisbyar Mar 6, 2023

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown

@discospiff discospiff Mar 6, 2023

Choose a reason for hiding this comment

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

Logging is good.

try {
medicineDAO.insert(medicine)
}
catch (e: Exception){
Log.e("MedicineRepository", "Error inserting medicine: ${e.message}")
throw Exception("Error inserting medicine",e)
}
}
}
4 changes: 2 additions & 2 deletions app/src/main/java/com/medmapper/v33001/room/MedicineRoomDatabase.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@ import androidx.room.RoomDatabase
import com.medmapper.v33001.dao.MedicineDAO
import com.medmapper.v33001.dto.Medicine

@Database(entities = arrayOf(Medicine::class), version = 1, exportSchema = false)
@Database(entities = [Medicine::class], version = 1, exportSchema = false)
Copy link
Copy Markdown
Collaborator Author

@grisbyar grisbyar Mar 6, 2023

Choose a reason for hiding this comment

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

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

public abstract class MedicineRoomDatabase : RoomDatabase() {

abstract fun medicineDao(): MedicineDAO
abstract suspend fun medicineDao(): MedicineDAO
Copy link
Copy Markdown
Collaborator Author

@grisbyar grisbyar Mar 6, 2023

Choose a reason for hiding this comment

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

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


companion object {
// Singleton prevents multiple instances of database opening at the same time.
Expand Down
12 changes: 0 additions & 12 deletions app/src/main/java/com/medmapper/v33001/ui/theme/Type.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,4 @@ val Typography = Typography(
fontWeight = FontWeight.Normal,
fontSize = 16.sp
)
/* Other default text styles to override
Copy link
Copy Markdown
Collaborator Author

@grisbyar grisbyar Mar 6, 2023

Choose a reason for hiding this comment

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

Suggesting removing comments, these are noise.

button = TextStyle(
fontFamily = FontFamily.Default,
fontWeight = FontWeight.W500,
fontSize = 14.sp
),
caption = TextStyle(
fontFamily = FontFamily.Default,
fontWeight = FontWeight.Normal,
fontSize = 12.sp
)
*/
)
2 changes: 2 additions & 0 deletions app/src/main/res/layout/layout2.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@

<EditText
android:id="@+id/editTextTextEmailAddress"
android:gravity="center"
Copy link
Copy Markdown
Collaborator Author

@grisbyar grisbyar Mar 6, 2023

Choose a reason for hiding this comment

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

Suggesting adding centering for this element, looks cleaner

android:layout_width="350dp"
android:layout_height="50dp"
android:ems="10"
Expand Down Expand Up @@ -64,6 +65,7 @@
android:id="@+id/textView2"
android:layout_width="200dp"
android:layout_height="99dp"
android:gravity="right"
Copy link
Copy Markdown
Collaborator Author

@grisbyar grisbyar Mar 6, 2023

Choose a reason for hiding this comment

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

Suggesting adding right alignment for this element, looks cleaner

android:text="@string/name_here"
android:textAlignment="viewEnd"
android:textAllCaps="false"
Expand Down