Skip to content

Wryther code review2 #32

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
wrytherUC wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Wryther code review2 #32

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
64f5484
Corrected selectedMedicine attribute to mirror Medicine id attribute
wrytherUC Mar 31, 2023
a99a71c
Changed Medicine DTO constructors to vars
wrytherUC Mar 31, 2023
cd5b697
Changed medicine to medication for better consistency
wrytherUC Mar 31, 2023
a160681
Fixed end of xml file
wrytherUC Apr 1, 2023
2760c77
Making mutableStateOf private
wrytherUC Apr 2, 2023
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
6 changes: 3 additions & 3 deletions app/src/main/java/com/medmapper/v33001/MainViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ class MainViewModel(var medicineService : IMedicineService) /*= MedicineService(

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())
Copy link
Copy Markdown
Collaborator Author

@wrytherUC wrytherUC Apr 2, 2023

Choose a reason for hiding this comment

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

Making the mutableStateOf variable private makes sure changes to it are in one class only.

(https://developer.android.com/kotlin/coroutines/coroutines-best-practices)

Copy link
Copy Markdown

@discospiff discospiff Apr 3, 2023

Choose a reason for hiding this comment

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

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.

var user: User? = null

private lateinit var firestore: FirebaseFirestore
Expand Down Expand Up @@ -67,14 +67,14 @@ class MainViewModel(var medicineService : IMedicineService) /*= MedicineService(
user?.let {
user ->
val document =
if (selectedMedicine.medicationID == null || selectedMedicine.medicationID.isEmpty()) {
if (selectedMedicine.id == null || selectedMedicine.id.isEmpty()) {
// create a new medicine
firestore.collection("users").document(user.uid).collection("medications").document()
} else {
// update an existing specimen
firestore.collection("users").document(user.uid).collection("medications").document()
}
selectedMedicine.medicationID = document.id
selectedMedicine.id = document.id
Copy link
Copy Markdown
Collaborator Author

@wrytherUC wrytherUC Apr 2, 2023

Choose a reason for hiding this comment

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

Changed selectedMedicine.medicationID to id to reflect the existing constructor/attribute that is in the Medicine DTO

Copy link
Copy Markdown

@discospiff discospiff Apr 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Either one works. No significant change to technical debt.

val handle = document.set(selectedMedicine)
handle.addOnSuccessListener { Log.d("Firebase", "Document Saved") }
handle.addOnFailureListener { Log.d("Firebase", "Save failed $it") }
Expand Down
18 changes: 9 additions & 9 deletions app/src/main/java/com/medmapper/v33001/dto/Medicine.kt
View file
Open in desktop
Copy link
Copy Markdown
Collaborator Author

@wrytherUC wrytherUC Apr 2, 2023

Choose a reason for hiding this comment

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

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.

Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,16 @@ package com.medmapper.v33001.dto
import java.time.LocalDate

data class Medicine(
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) {
Comment on lines -6 to +15
Copy link
Copy Markdown

@discospiff discospiff Apr 3, 2023

Choose a reason for hiding this comment

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

Leave this as it were. Assume variables are immutable (val), unless we have a very good reason for reassigning them.

override fun toString(): String {
return "$name, $quantity , $prescriptionStrength"
}
Expand Down
5 changes: 3 additions & 2 deletions app/src/main/res/layout/layout3.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@
android:id="@+id/button10"
android:layout_width="234dp"
android:layout_height="78dp"
android:text="Add new Medicine "
android:text="Add new Medication "
app:layout_constraintEnd_toEndOf="parent"
app:layout_constraintHorizontal_bias="0.497"
app:layout_constraintStart_toStartOf="parent"
Expand All @@ -93,7 +93,7 @@
android:text="Name"
app:layout_constraintStart_toStartOf="parent"
tools:layout_editor_absoluteY="100dp"
tools:text="Medicine Name" />
tools:text="Medication Name" />
Copy link
Copy Markdown
Collaborator Author

@wrytherUC wrytherUC Apr 2, 2023

Choose a reason for hiding this comment

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

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.


<EditText
android:id="@+id/editTextTextEmailAddress2"
Expand Down Expand Up @@ -155,3 +155,4 @@


</androidx.constraintlayout.widget.ConstraintLayout>
</androidx.constraintlayout.widget.ConstraintLayout>
Copy link
Copy Markdown
Collaborator Author

@wrytherUC wrytherUC Apr 2, 2023

Choose a reason for hiding this comment

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

Added an end of line for the androidx.constraintlayout.widget.ConstraintLayout block