Conversation
# Conflicts: # app/src/main/java/com/ucmobiledevelopment/freeways/service/IncidentService.kt
| implementation "androidx.compose.ui:ui:$compose_version" | ||
| implementation "androidx.compose.material:material:$compose_version" | ||
| implementation "androidx.compose.ui:ui-tooling-preview:$compose_version" | ||
| implementation 'androidx.lifecycle:lifecycle-runtime-ktx:2.4.0' |
There was a problem hiding this comment.
Updated to the latest version to 2.4.1 as per IDE and the professor's lecture video recommendations.
| //TO DO: single<IIncidentService> { IncidentService } | ||
| //TO DO: add "get()" as the parameter in the MainViewModel in line 8 |
There was a problem hiding this comment.
Not a serious issue but it is better to give space when commenting as per Kotlin convention. https://kotlinlang.org/docs/coding-conventions.html#horizontal-whitespace
| private var firebaseUser: FirebaseUser? = FirebaseAuth.getInstance().currentUser | ||
| private var selectedIncident: Incident? = null | ||
| private val viewModel : MainViewModel by viewModel<MainViewModel>() | ||
| private val viewModel: MainViewModel by viewModel() |
There was a problem hiding this comment.
Deleted the space from the colon as per Kotlin conventions and after running the debugger MainViewModel in angular brackets was not necessary. https://kotlinlang.org/docs/coding-conventions.html#colon
| Box(Modifier.fillMaxWidth(), contentAlignment = Alignment.Center) { | ||
| Row(Modifier | ||
| .padding(5.dp), | ||
| Row( | ||
| Modifier | ||
| .padding(5.dp), |
There was a problem hiding this comment.
Reformated the code for easier and more consistent reading. https://kotlinlang.org/docs/coding-conventions.html#formatting
| ExtendedFloatingActionButton ( | ||
| text = { Text(text = "Report Incident") }, | ||
| onClick = { val intent = Intent(context, ReportIncidentActivity::class.java) | ||
| ExtendedFloatingActionButton( | ||
| text = { Text(text = "Report Incident") }, | ||
| onClick = { | ||
| val intent = Intent(context, ReportIncidentActivity::class.java) |
There was a problem hiding this comment.
Reformated the code. https://kotlinlang.org/docs/coding-conventions.html#formatting
| fun initMocksAndMainThread(){ | ||
| fun initMocksAndMainThread() { |
There was a problem hiding this comment.
| fun `given a view model with live data when populated with incidents between 2019 and 2020, in state 40, county 1, results show incident with caseId 400132`(){ | ||
| fun `given a view model with live data when populated with incidents between 2019 and 2020, in state 40, county 1, results show incident with caseId 400132`() { |
There was a problem hiding this comment.
| incidents.add(Incident(stateId = 40, stateName = "Oklahoma", caseId = "400132", countyId = 1, countyName = "Adair", latitude = "35.64110000", longitude = "-94.678300000")) | ||
| incidents.add(Incident(stateId = 40, stateName = "Oklahoma", caseId = "400429", countyId = 1, countyName = "Adair", latitude = "35.98840000", longitude = "-94.655000000")) | ||
| incidents.add(Incident(stateId = 40, stateName = "Oklahoma", caseId = "400434", countyId = 1, countyName = "Adair", latitude = "35.82660000", longitude = "-94.613000000")) |
There was a problem hiding this comment.
Broke really long lines into multiple lines with formating.
https://kotlinlang.org/docs/coding-conventions.html#formatting
https://kotlinlang.org/docs/coding-conventions.html#properties
| var allIncidents : List<Incident>? = ArrayList<Incident>() | ||
| var allIncidents: List<Incident>? = ArrayList<Incident>() |
There was a problem hiding this comment.
| if(it.caseId.equals("400132")){ | ||
| if (it.caseId == "400132") { |
There was a problem hiding this comment.
We are comparing the actual data for structural equality. https://agrawalsuneet.github.io/blogs/equality-in-kotlin/#:~:text=Referential%20equality%20%28%E2%80%98%3D%3D%3D%E2%80%99%29%20%3D%3D%3Doperator%20is%20used%20to%20compare,the%20values%20are%20not%20equal%20to%20each%20other.?msclkid=2df09cd9af1111eca7c1a300ad8d2350
| internal val CITY: String? = "", | ||
| internal val CITY_NAME: String? = "", | ||
| internal val COUNTY: String? = "", | ||
| internal val COUNTRY_NAME: String? = "", | ||
| internal val CASE_YEAR: String? = "", | ||
| internal val FATALS: String? = "", | ||
| internal val LATITUDE: String? = "", | ||
| internal val LONGITUDE: String? = "", | ||
| internal val STATE: String? = "", | ||
| internal val STATE_NAME: String? = "", | ||
| internal val ST_CASE: String? = "", | ||
| internal val TOTALVEHICLES: String? = "", | ||
| internal val TWAY_ID: String? = "", | ||
| internal val TWAY_ID2: String? = "", | ||
| internal val VE_FORMS: String? = "", |
| data class IncidentListDTO( | ||
| val Count: Int = 0, | ||
| val Message: String, | ||
| val Message: String = "", |
| val uid: String = "", | ||
| var displayName: String? = "", |
| internal val CITY: String? = "", | ||
| internal val CITY_NAME: String? = "", | ||
| internal val COUNTY: String? = "", | ||
| internal val COUNTRY_NAME: String? = "", | ||
| internal val CASE_YEAR: String? = "", | ||
| internal val FATALS: String? = "", | ||
| internal val LATITUDE: String? = "", | ||
| internal val LONGITUDE: String? = "", | ||
| internal val STATE: String? = "", | ||
| internal val STATE_NAME: String? = "", | ||
| internal val ST_CASE: String? = "", | ||
| internal val TOTALVEHICLES: String? = "", | ||
| internal val TWAY_ID: String? = "", | ||
| internal val TWAY_ID2: String? = "", | ||
| internal val VE_FORMS: String? = "" |
There was a problem hiding this comment.
I actually reverted this and added = "" in another commit.
| val Message: String = "", | ||
| val Message: String, | ||
| val Results: List<List<IncidentDTO>>, | ||
| val SearchCriteria: String = "" | ||
| val SearchCriteria: String, |
There was a problem hiding this comment.
I actually reverted this and added = "" in another commit.
| val uid: String = "", | ||
| var displayName: String? |
There was a problem hiding this comment.
I actually reverted this and added = "" in another commit.
| newIncident.longitude = it.LONGITUD ?: "" | ||
| newIncident.caseId = it.ST_CASE ?: "" |
There was a problem hiding this comment.
Typo and spacing.
| newIncident.vehiclesInvolved = if(it.TOTALVEHICLES != null) it.TOTALVEHICLES.toInt() else 0 | ||
| newIncident.vehiclesInvolved = | ||
| if (it.TOTALVEHICLES != null) it.TOTALVEHICLES.toInt() else 0 |
There was a problem hiding this comment.
Broke a long line into two.
| val TWAY_ID2: String? = "", | ||
| val VE_FORMS: String? = "" | ||
| internal val CITY: String? = "", | ||
| internal val CITY_NAME: String? = "", |
| var rule: TestRule = InstantTaskExecutorRule() | ||
|
|
||
| lateinit var incidentService : IncidentService | ||
| //var allIncidents : IncidentListDTO? = IncidentListDTO(Results = ArrayList<List<IncidentDTO>>()) |
There was a problem hiding this comment.
Good suggestion.
Avoid pushing commented code to Version Control. Version control has tools that makes this unnecessary.
- You can look at history to see what used to be there.
- You can use branches for experimental features.
|
A few good suggestions worth reviewing. |
2 participants
Was the program available in UC Github on time?
Answer) Yes.
Is the program documented/commented well enough for you to understand?
Answer) Yes. It was easy to read and understand.
Does the program compile?
Answer) Yes, the code does compile but the IncidentUnitTest fails. Maybe due to the unit test not knowing what firebase is.
The rationale behind your changes.
Answer) The code is mostly reformating, logic simplification, some deletion of not used imports, and updating an implementation.
My commits.
Smoofington/FindMyRecyclingV32001@56e4915
Smoofington/FindMyRecyclingV32001@56e4915
Smoofington/FindMyRecyclingV32001@5ac93f1
Smoofington/FindMyRecyclingV32001@69030d9
Smoofington/FindMyRecyclingV32001@953fb34
What I learned from this code review.
Answer) Further improvement on how to debug Kotlin code. Learned how to format according to Kotlin conventions. Naming conventions in Kotlin, and logic simplification in Kotlin. Learned how lambda functions work and what "it" does.