harrie7: Code Review 2#24
Conversation
|
The tests still fail, but are now failing in the same way they did in the most recent commit to master. |
|
ANALYSIS: Technical concepts: |
| Button( | ||
| onClick = { | ||
| var incidentInfo = Incident().apply { | ||
| var incidentInfo = Incident("1").apply { |
There was a problem hiding this comment.
What's the meaning of the "1" here? It looks like a magic number. Make a constant from this, and give the constant a descriptive name, so that the code is self-describing.
| snapshot?.let { | ||
| val allIncidents = ArrayList<Incident>() | ||
| allIncidents.add(Incident(caseId = NEW_INCIDENT)) | ||
| allIncidents.add(Incident(incidentId = "1", caseId = NEW_INCIDENT)) |
| handle.addOnFailureListener { Log.e("Firebase", "Save failed $it") } | ||
| handle.addOnFailureListener { Log.e("Firebase", "Save failed $user") } |
There was a problem hiding this comment.
These are different variables:
it == the error message
user == a user
I'd keep this as it were, as the error message is the most important thing to see in a log.
| handle.addOnFailureListener { Log.e("Firebase", "User save failed $it") } | ||
| handle.addOnFailureListener { Log.e("Firebase", "User save failed $user") } |
|
|
||
| data class Incident( | ||
| var incidentId: String = "", | ||
| var incidentId: String, |
|
I don't see enough technical debt reduction in this branch to warrant a merge. |
2 participants
No description provided.