-
Notifications
You must be signed in to change notification settings - Fork 5
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.
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
Coordinate validator changes #664
base: feature/n-dimensional-coordinates
Are you sure you want to change the base?
Coordinate validator changes #664
Conversation
Move the generally useful too few/many sig fig functions from the numeric validator to utils, and use them to check for correctness of significant figures in coordinate questions.
@@ -192,6 +199,9 @@ List<CoordinateItem> orderCoordinates(final List<CoordinateItem> items) { | |||
for (int i = 0; i < numDimensions; i++) { | |||
String valueA = a.getCoordinates().get(i); | |||
String valueB = b.getCoordinates().get(i); | |||
if (valueA.isEmpty() || valueB.isEmpty()) { | |||
return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think zero is right here, because zero would mean they are equal.
I think if one is empty, we should return something more like valueA.compareTo(valueB)
? That will at least put the empty ones somewhere stable (which is necessary to avoid runtime exceptions).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed - sorting the empty strings to the start of the list affects the feedback that is given in some cases (since we'll always check empty values before anything else), but that's probably an improvement.
|
||
boolean valuesMatch = ValidationUtils.numericValuesMatch(choiceValue, submittedValue, sigFigs, log); | ||
if (submittedValue.isEmpty()) { | ||
feedback = new Content("You did not provide a complete answer."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this feedback be a constant, like we have done already for some common ones? I can see it being used elsewhere, unlike the other ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done (only used here for now - there are some cloze/reorder cases where it might work, but their feedback is currently more specific).
Small changes to the coordinate question validator: