Conversation
| newWinnerSet = runTabulationForConfig(config, castVoteRecords); | ||
| } catch (TabulationCancelledException exception) { | ||
| Logger.severe("Tabulation was cancelled by the user!"); | ||
| Logger.severe(exception.getMessage()); |
There was a problem hiding this comment.
Here and below: I think you might be better off just logging exception, since that should include the message and additional useful info as well.
There was a problem hiding this comment.
I don't think we want other info; the purpose of this exception class is to communicate a specific message to the user in the output.
tarheel
left a comment
tarheel
left a comment
There was a problem hiding this comment.
Seems we need a test.
Not sure whether you saw my comment in the PR description: "I didn't add a test case because I don't believe we have any support (yet) for tests that confirm a failed tabulation."
Ideally, yes, we would have support for that kind of test, but I don't think it's worth doing at the moment. (Or at least I can't prioritize it right now.)
| newWinnerSet = runTabulationForConfig(config, castVoteRecords); | ||
| } catch (TabulationCancelledException exception) { | ||
| Logger.severe("Tabulation was cancelled by the user!"); | ||
| Logger.severe(exception.getMessage()); |
There was a problem hiding this comment.
I don't think we want other info; the purpose of this exception class is to communicate a specific message to the user in the output.
|
Yep, I saw. I'm not sure how to review this in the absence of a test. |
|
OK, I removed the default constructor. Regarding review: I did test it manually and confirmed that it aborts with the correct message if this situation occurs. So I think that suffices for now. |
| // One edge case: if everyone is below the threshold, we can't proceed. This would only | ||
| // happen in the first or (if we drop undeclared write-ins first) second round. | ||
| if (eliminated.size() == config.getNumDeclaredCandidates()) { | ||
| Logger.severe("Tabulation can't proceed because all declared candidates are below " |
There was a problem hiding this comment.
Maybe "Tabulation complete because..." ... the "can't proceed" language implies something is wrong, when in fact this is a normal (if edge) case.
There was a problem hiding this comment.
I don't think we should consider this to be a "normal" case. Instead of producing winners and generating output files, we're aborting.
| if (eliminated.size() == config.getNumDeclaredCandidates()) { | ||
| Logger.severe("Tabulation can't proceed because all declared candidates are below " | ||
| + "the minimum vote threshold."); | ||
| throw new TabulationCancelledException(false); |
There was a problem hiding this comment.
It's not really cancelled, right? It's just ending sooner than we thought. How about changing it to a TabulationException if it's going to server this purpose?
There was a problem hiding this comment.
Would you be more on board with "aborted" instead of "cancelled"? I think that captures both situations where this exception is used (giving up because no one meets the threshold or exiting because the user clicked a Cancel button). Just TabulationException is also OK, but I'd prefer to be more specific.
HEdingfield
left a comment
HEdingfield
left a comment
There was a problem hiding this comment.
Not sure whether you saw my comment in the PR description: "I didn't add a test case because I don't believe we have any support (yet) for tests that confirm a failed tabulation."
Ideally, yes, we would have support for that kind of test, but I don't think it's worth doing at the moment. (Or at least I can't prioritize it right now.)
Would you mind adding an issue for this, pointing to this PR?
Upon further reflection... it might not be a big deal to add a test for this. I'll follow up in this PR. |
tarheel
force-pushed
the
nobody
branch
2 times, most recently
from
386f5f5 to
207ee09
Compare
|
Added a test here also. It was awkward to find a way to make the test work, but I think the solution I came up with isn't... terrible. |
| tabulate(null); | ||
| } | ||
|
|
||
| void tabulate(List<String> exceptionsEncountered) { |
There was a problem hiding this comment.
Why not just have this return the List? That's a more typical api for error codes and then you don't need to do all the checks for if (exceptionsEncountered != null) here.
There was a problem hiding this comment.
You can also get rid of the wrapper methods
There was a problem hiding this comment.
I'm OK with that approach also. I was thinking it would be weird, but upon further reflection, I guess it's fine.
4 participants
Fixes #553.
I didn't add a test case because I don't believe we have any support (yet) for tests that confirm a failed tabulation.