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.

Sign up for GitHub

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

Jump to bottom

Hart Auto-Load #919

Open

yezr wants to merge 2 commits into develop from feature/issue-918-hart-autoload

Collaborator

yezr commented Feb 14, 2025

fixes #918

yezr added 2 commits

February 14, 2025 15:06


          For Hart, build candidateCodeToCandidate lookup during readCastVoteRe…

709abc0

…cords. Use that in overriden gatherUnknownCandidates for auto-loading.


          code analysis fixes

eeaecb6

Collaborator Author

yezr commented Feb 14, 2025 •

edited

Loading

To facilitate getting v2.0 out the door, and with Armin out, I drafted a PR for #918. The logic works, but I would love a review of the code syntax.

artoonie reviewed

View reviewed changes

src/main/java/network/brightspots/rcv/HartCvrReader.java

+                            candidateCode = Tabulator.UNDECLARED_WRITE_IN_OUTPUT_LABEL;
+                          }
+                          if (this.candidateCodesToCandidates.containsKey(candidateCode)) {
+                            if (!this.candidateCodesToCandidates.get(candidateCode).Name.equals(candidateName)) {

Collaborator

artoonie Feb 20, 2025

These two ifs can be simplified a bit:

Candidate existingCandidate = this.candidateCodesToCandidates.get(candidateCode);
if (existingCandidate != null && existingCandidate.Name.equals(candidateName)) {

src/main/java/network/brightspots/rcv/HartCvrReader.java

+                                      Candidate Code %s associated with more than one candidate name.
+                                      Originally associated with name '%s'.
+                                      In CVR at '%s' it is associated with '%s'.
+                                      """.formatted(candidateCode,

Collaborator

artoonie Feb 20, 2025

This syntax will cause the error message to have a ton of spaces throughout the error message. Using +s will avoid that:

                String message =
                        "Candidate Code %s associated with more than one candidate name.  " +
                        "Originally associated with name '%s'. " + 
                        "In CVR at '%s' it is associated with '%s'."
                        .formatted(candidateCode,

src/main/java/network/brightspots/rcv/HartCvrReader.java

+                              throw new CastVoteRecord.CvrParseException();
+                            }
+                          } else {
+                            if (!candidateCode.equals(Tabulator.UNDECLARED_WRITE_IN_OUTPUT_LABEL)) {

Collaborator

artoonie Feb 20, 2025

Combine into else if { rather than else { if {

src/main/java/network/brightspots/rcv/HartCvrReader.java

+                            if (!candidateCode.equals(Tabulator.UNDECLARED_WRITE_IN_OUTPUT_LABEL)) {
+                              this.candidateCodesToCandidates.put(candidateCode,
+                                      new Candidate(candidateName, candidateCode));
+                            }

Collaborator

artoonie Feb 20, 2025

I think you can simplify this whole block using computeIfAbsent:

          for (Option option : contest.Options) {
            final String candidateCode = option.Id;
            final String candidateName = option.Name;
            final boolean isUndeclaredWriteIn = candidateCode.equals(
                    source.getUndeclaredWriteInLabel());

            if (!isUndeclaredWriteIn) {
              this.candidateCodesToCandidates.computeIfAbsent(candidateCode, k ->
                      new Candidate(candidateName, candidateCode));

              String candidateNameFromMap = this.candidateCodesToCandidates.get(candidateCode).Name;
              if (!candidateNameFromMap.equals(candidateName)) {
                String message = String.format(
                        "Candidate Code %s associated with more than one candidate name. "
                                + "Originally associated with name '%s'. "
                                + "In CVR at '%s' it is associated with '%s'.",
                        candidateCode, candidateNameFromMap, path.getFileName(), candidateName
                );
                Logger.severe(message);
                throw new CastVoteRecord.CvrParseException();
              }
            }

            // Hart RCV election ranks are indicated by a string read left to right:
            // each digit corresponds to a rank and is set to 1 if that rank was voted:
            // 0100 indicates rank 2 was voted
            // 0000 indicates no rank was voted (undervote)
            // 0101 indicates ranks 2 and 4 are voted (overvote)
            for (int rank = 1; rank < option.Value.length() + 1; rank++) {
              String rankValue = option.Value.substring(rank - 1, rank);
              if (rankValue.equals("1")) {
                if (isUndeclaredWriteIn) {
                  rankings.add(new Pair<>(rank, UNDECLARED_WRITE_IN_OUTPUT_LABEL));
                } else {
                  rankings.add(new Pair<>(rank, candidateCode));
                }
              }
            }
          }

src/main/java/network/brightspots/rcv/HartCvrReader.java

+                    } catch (CastVoteRecord.CvrParseException e) {
+                      Logger.severe("Error gathering Unknown Candidates\n%s", e);
+                      return new HashSet<>();
+                    } catch (IOException e) {

Collaborator

artoonie Feb 20, 2025

You can collapse the two catches into one:

      } catch (CastVoteRecord.CvrParseException | IOException e) {

src/main/java/network/brightspots/rcv/HartCvrReader.java

+                  Set<String> knownNames = config.getCandidateNames();
+                  if (this.candidateCodesToCandidates.entrySet().isEmpty()) {
+                    try {
+                      //Reading the CVRs will load this.candidateCodestoCandidates

Collaborator

artoonie Feb 20, 2025

Suggested change

      
                    //Reading the CVRs will load this.candidateCodestoCandidates
          
                    // Reading the CVRs will load this.candidateCodestoCandidates

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet