Closed vteague closed 2 months ago
Changes and tests LGTM! I briefly considered if the booleans (isPlurality, isIRV and isSTV) could be simplified a bit (perhaps using !isPlurality in the definitions for isIRV and isSTV but it's possible that would miss a 4th possibility of all 3 booleans being false.
Yes, indeed, those three cases do not cover all possible values @charliecarlton, only the ones we expect to encounter. I guess I decided that, on something a bit unexpected, it's probably better to raise an error than to let it past. The obvious example would be something that has both a "Vote For=" and a "Number of positions=" header. This would actually make perfect sense as a plurality contest for which the number of available positions doesn't match the number you're permitted to vote for. There's no particular reason they need to be the same - they just are the same at the moment for all the contests in CO.
So the code I've written will throw an error at that particular combination of options. I think this is the right thing to do, because if we let it through I'm not sure that the winners_allowed and votes_allowed options would be set properly (they probably wouldn't). In short, if some new combination of options appears, I think somebody probably needs to go through and make sure it does all the right things.
So there's a much simpler way of writing it, like
// If winners and allowed votes are as expected for plurality, this is a plurality contest.
final boolean isPlurality = pluralityVotesAllowed > 0;
final boolean isIRV = !isPlurality && irvVotesAllowed > 0 && irvWinners == 1;
// If it looks like IRV but has more than one winner, it's an STV contest.
final boolean isSTV = !isPlurality && irvVotesAllowed > 0 && irvWinners > 1;
But this lacks the future-defensiveness as above. I can't think of a nicer way of writing it that is both easier to read and defensive against future unexpected combinations.
Drops all trace of STV contests, as if they'd been cut out of the CVR before upload.