Conversation
| internal val NEW_MEDICATION = "New Medication" | ||
| var medicine : MutableLiveData<List<Medicine>> = MutableLiveData<List<Medicine>>() | ||
| var selectedMedicine by mutableStateOf(Medicine()) | ||
| private var selectedMedicine by mutableStateOf(Medicine()) |
There was a problem hiding this comment.
Making the mutableStateOf variable private makes sure changes to it are in one class only.
(https://developer.android.com/kotlin/coroutines/coroutines-best-practices)
There was a problem hiding this comment.
A mutableState object will likely be accessed by an Activity class, as that's the nature of having a mutableState in the first place. private will hide this access. I urge caution when changing an access modifier in a ViewModel.
| firestore.collection("users").document(user.uid).collection("medications").document() | ||
| } | ||
| selectedMedicine.medicationID = document.id | ||
| selectedMedicine.id = document.id |
There was a problem hiding this comment.
Changed selectedMedicine.medicationID to id to reflect the existing constructor/attribute that is in the Medicine DTO
There was a problem hiding this comment.
Either one works. No significant change to technical debt.
There was a problem hiding this comment.
Having the constructors set to val, or read only, was causing a compiling error when trying to run the program. Making it writable resolved the error.
| app:layout_constraintStart_toStartOf="parent" | ||
| tools:layout_editor_absoluteY="100dp" | ||
| tools:text="Medicine Name" /> | ||
| tools:text="Medication Name" /> |
There was a problem hiding this comment.
Changed Medicine lines to Medication. Medication was used more frequently in the app then medicine was, so making this change made the UI more consistent.
|
|
||
|
|
||
| </androidx.constraintlayout.widget.ConstraintLayout> | ||
| </androidx.constraintlayout.widget.ConstraintLayout> No newline at end of file |
There was a problem hiding this comment.
Added an end of line for the androidx.constraintlayout.widget.ConstraintLayout block
| val id: String = "", | ||
| val uid: String = "", | ||
| val name: String = "", | ||
| val quantity: Int = 0, | ||
| val prescriptionStrength: String = "", | ||
| val startDate: LocalDate = LocalDate.now(), | ||
| val prescriptionLength: String = "", | ||
| val time: Long = 0, | ||
| var id: String = "", | ||
| var uid: String = "", | ||
| var name: String = "", | ||
| var quantity: Int = 0, | ||
| var prescriptionStrength: String = "", | ||
| var startDate: LocalDate = LocalDate.now(), | ||
| var prescriptionLength: String = "", | ||
| var time: Long = 0, | ||
| // Frequency per 24 hours | ||
| val frequency: Int = 0) { | ||
| var frequency: Int = 0) { |
There was a problem hiding this comment.
Leave this as it were. Assume variables are immutable (val), unless we have a very good reason for reassigning them.
|
I don't recommend merging this branch. Some of the recommendations increase tech debt instead of decreasing it. |
2 participants
Analysis of the program.
This application is to help with keeping track of what kind of medication is taken and when to take it. It will also integrate with Android in the future to set up reminders.
Updates from second review:
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 your changes.
Links to three specific commits that you made to your own group's GitHub repository before the end of sprint deadline.
Next, list three specific technical concepts that you learned from this code review. Copy and paste samples to demonstrate your point.
Layout XML files - Writing a UI from XML files (https://developer.android.com/develop/ui/views/layout/declaring-layout)
Mutable types - Best practice to keep these private to one class to improve debugging. (https://developer.android.com/kotlin/coroutines/coroutines-best-practices#mutable-types)
It might not be something quite new, but was interesting to see that this group also used Firebase DB to create separate databases/collections to keep medicine stored in.