Rename Record "Value" to "Response" for consistency #44
Conversation
Also, the meaning of "Value" is broad and conflicts with other symbols.
There was a problem hiding this comment.
Good call! And looks good on the whole. I left a couple of comments on instances where value is still used in the binding term/variable even though Response is now the type. There's nothing wrong with this of course, but I think if we establish some symmetry between the two it establishes further that 'answer to a form element' == response, and it would signal the cases in which we're dealing with responses more strongly. Hopefully, that's not too pedantic--it's definitely a preferential thing so feel free to ignore (or correct me). I think clarifying it at the type level is clear enough.
gnd/src/main/java/com/google/android/gnd/ui/editrecord/EditRecordViewModel.java
Outdated
Show resolved
Hide resolved
gnd/src/main/java/com/google/android/gnd/ui/editrecord/EditRecordFragment.java
Outdated
Show resolved
Hide resolved
gnd/src/main/java/com/google/android/gnd/service/firestore/FirestoreDataService.java
Outdated
Show resolved
Hide resolved
gnd/src/main/java/com/google/android/gnd/service/firestore/FirestoreDataService.java
Outdated
Show resolved
Hide resolved
| String fieldId = field.getId(); | ||
| Optional<Value> originalValue = r.getValue(fieldId); | ||
| Optional<Value> currentValue = getValue(fieldId).filter(v -> !v.isEmpty()); | ||
| Optional<Response> originalValue = r.getValue(fieldId); |
There was a problem hiding this comment.
s/originalValue/originalResponse
gnd/src/main/java/com/google/android/gnd/ui/editrecord/EditRecordViewModel.java
Outdated
Show resolved
Hide resolved
gnd/src/main/java/com/google/android/gnd/ui/editrecord/EditRecordViewModel.java
Outdated
Show resolved
Hide resolved
gnd/src/main/java/com/google/android/gnd/ui/editrecord/MultiSelectDialogFactory.java
Outdated
Show resolved
Hide resolved
gnd/src/main/java/com/google/android/gnd/ui/editrecord/MultiSelectDialogFactory.java
Outdated
Show resolved
Hide resolved
|
Pushed an update. PTAL! |
gnd/src/main/java/com/google/android/gnd/service/firestore/FirestoreDataService.java
Outdated
Show resolved
Hide resolved
gnd/src/main/java/com/google/android/gnd/ui/editrecord/MultiSelectDialogFactory.java
Show resolved
Hide resolved
|
Caught a few more. Using "r" in lambdas, and "response" for variables. Also, renamed ambiguous and incorrect usages of "v ->" and "value ->". |
|
So... 🙄 Been thinking about this a bit more, and have an uncomfortable question to ask late in the review: We will want a way to represent changed values in the edit queue both at Feature level (currently only for location, in the future probably custom id, and maybe even custom attributes), and Record level (currently only for form responses, but perhaps eventually for fixed attributes as well). In that case, rather than make Value more specific (record), perhaps we should be making it more generic by moving it out of Record, and letting it represent the allowed user-provided data types (String, Float, Array of Strings, Coordinates) rather than the allowed form element types (Text, Number, Multiple Choice)? We could then even use the same JSON representation in the local db's edit queue as in remote Firestore. @navinsivakumar in case you have any amount of cents to add. |
|
I just took a cursory look at the current definitions, so lmk if I'm misunderstanding here. Sounds like a reasonable proposal. Just to make sure I'm getting it, essentially our Queue would take operations for any object that implements the value interface, which would enable us to stick pretty much whatever edits we want (feature, record, etc.) into the edit queue? That sounds like a good idea--I don't see any particular reason for needing distinct queues at the moment if we're trying to capture a sequence of operations on a set of entities, editBoo, editBah, etc. I think that makes sense. So we'd have a rather general interface Value that defines the methods each editable object in the queue must implement, with further specifications for particular value types (sort of how it's setup in the record over form elements today). So Value, StringValue, FloatValue, etc. and Feature and Record can implement these as needed, and anything that implements them can be put into the edit queue? As for the existing value interface, can we just call this FormElement, or FormField, or something equivalent? |
|
If we're looking at the data abstractions that are presented to application logic (which I think is what we have in this PR), I don't really see a case for trying to unify the Feature and Record value representations -- business logic of, say, a ViewModel, would never use them interchangeably, would it? We might want to do something clever about unifying the representations further down the stack in the data layer, but I don't think that needs to be decided right now (or if it does need to be decided right now, then we're not enforcing the abstraction layers properly). My two cents is that it might be easier to treat them as separate at that level as well, although I am not familiar with how Firestore plays into this. |
|
I think this particular discussion might be off topic for the rename PR; moved to: #19 (comment) |
|
@navinsivakumar as per our other discussions, at the moment we won't use the Response for Features, so applying YAGNI here and leaving as is; we can refactor later if necessary. Re reusing Value/Response in the local datastore, also agreed in the other thread that this would be bad practice for several reasons mentioned there. @scolsen, sorry for the detour. Would you proceed with merge if you think it still looks ok? |
|
You got it, merging! |
Also, the meaning of "Value" is broad and conflicts with other symbols. This became apparent when implementing local db support because edits can refer to user responses but also changes to featute coordinates, for example.