Simplify DeleteVote to only use a single loop

The old logic was a little confusing:
 - Looked up approvals twice.
 - Looped over label types.
 - Created the message in the loop body.

Change-Id: Ib59df4cb3032800a305efb6928567e4d67dac07e

gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteVote.java

@@ -14,8 +14,9 @@
 
 package com.google.gerrit.server.change;
 
+import static com.google.common.base.Preconditions.checkNotNull;
+
 import com.google.gerrit.common.TimeUtil;
-import com.google.gerrit.common.data.LabelType;
 import com.google.gerrit.common.data.LabelTypes;
 import com.google.gerrit.extensions.api.changes.DeleteVoteInput;
 import com.google.gerrit.extensions.api.changes.NotifyHandling;
@@ -28,6 +29,7 @@ import com.google.gerrit.extensions.restapi.RestModifyView;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.ChangeMessage;
+import com.google.gerrit.reviewdb.client.LabelId;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.server.ReviewDb;
@@ -44,6 +46,7 @@ import com.google.gerrit.server.git.UpdateException;
 import com.google.gerrit.server.mail.DeleteVoteSender;
 import com.google.gerrit.server.mail.ReplyToChangeSender;
 import com.google.gerrit.server.project.ChangeControl;
+import com.google.gerrit.server.util.LabelVote;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
@@ -137,64 +140,66 @@ public class DeleteVote
       PatchSet.Id psId = change.currentPatchSetId();
       ps = psUtil.current(db.get(), ctl.getNotes());
 
-      PatchSetApproval psa = null;
+      boolean found = false;
-      StringBuilder msg = new StringBuilder();
-
-      // get all of the current approvals
       LabelTypes labelTypes = ctx.getControl().getLabelTypes();
-      Map<String, Short> currentApprovals = new HashMap<>();
+
-      for (LabelType lt : labelTypes.getLabelTypes()) {
-        currentApprovals.put(lt.getName(), (short) 0);
       for (PatchSetApproval a : approvalsUtil.byPatchSetUser(
           ctx.getDb(), ctl, psId, accountId)) {
-          if (lt.getLabelId().equals(a.getLabelId())) {
+        if (labelTypes.byLabel(a.getLabelId()) == null) {
-            currentApprovals.put(lt.getName(), a.getValue());
+          continue; // Ignore undefined labels.
-          }
+        } else if (!a.getLabel().equals(label)) {
-        }
+          // Populate map for non-matching labels, needed by VoteDeleted.
-      }
+          newApprovals.put(a.getLabel(), a.getValue());
-      // removing votes so we need to determine the new set of approval scores
+          continue;
-      newApprovals.putAll(currentApprovals);
+        } else if (!ctl.canRemoveReviewer(a)) {
-      for (PatchSetApproval a : approvalsUtil.byPatchSetUser(
-            ctx.getDb(), ctl, psId, accountId)) {
-        if (ctl.canRemoveReviewer(a)) {
-          if (a.getLabel().equals(label)) {
-            // set the approval to 0 if vote is being removed
-            newApprovals.put(a.getLabel(), (short) 0);
-            // set old value only if the vote changed
-            oldApprovals.put(a.getLabel(), a.getValue());
-            msg.append("Removed ")
-                .append(a.getLabel()).append(formatLabelValue(a.getValue()))
-                .append(" by ").append(userFactory.create(a.getAccountId())
-                    .getNameEmail())
-                .append("\n");
-            psa = a;
-            a.setValue((short)0);
-            ctx.getUpdate(psId).removeApprovalFor(a.getAccountId(), label);
-            break;
-          }
-        } else {
           throw new AuthException("delete vote not permitted");
         }
+        // Set the approval to 0 if vote is being removed.
+        newApprovals.put(a.getLabel(), (short) 0);
+        found = true;
+
+        // Set old value, as required by VoteDeleted.
+        oldApprovals.put(a.getLabel(), a.getValue());
+        break;
       }
-      if (psa == null) {
+      if (!found) {
         throw new ResourceNotFoundException();
       }
-      ctx.getDb().patchSetApprovals().upsert(Collections.singleton(psa));
 
-      if (msg.length() > 0) {
+      ctx.getUpdate(psId).removeApprovalFor(accountId, label);
+      ctx.getDb().patchSetApprovals().upsert(
+          Collections.singleton(deletedApproval(ctx)));
+
       changeMessage =
           new ChangeMessage(new ChangeMessage.Key(change.getId(),
               ChangeUtil.messageUUID(ctx.getDb())),
               ctx.getAccountId(),
               ctx.getWhen(),
               change.currentPatchSetId());
-        changeMessage.setMessage(msg.toString());
+      StringBuilder msg = new StringBuilder();
+      msg.append("Removed ");
+      LabelVote.appendTo(msg, label, checkNotNull(oldApprovals.get(label)));
+      changeMessage.setMessage(
+          msg.append(" by ")
+              .append(userFactory.create(accountId).getNameEmail())
+              .append("\n")
+              .toString());
       cmUtil.addChangeMessage(ctx.getDb(), ctx.getUpdate(psId),
           changeMessage);
-      }
+
       return true;
     }
 
+    private PatchSetApproval deletedApproval(ChangeContext ctx) {
+      return new PatchSetApproval(
+          new PatchSetApproval.Key(
+              ps.getId(),
+              accountId,
+              new LabelId(label)),
+          (short) 0,
+          ctx.getWhen());
+    }
+
     @Override
     public void postUpdate(Context ctx) {
       if (changeMessage == null) {
@@ -220,11 +225,4 @@ public class DeleteVote
           user.getAccount(), ctx.getWhen());
     }
   }
-
-  private static String formatLabelValue(short value) {
-    if (value > 0) {
-      return "+" + value;
-    }
-    return Short.toString(value);
-  }
 }

gerrit-server/src/main/java/com/google/gerrit/server/util/LabelVote.java

@@ -58,6 +58,16 @@ public abstract class LabelVote {
         Short.parseShort(text.substring(e + 1), text.length()));
   }
 
+  public static StringBuilder appendTo(StringBuilder sb, String label,
+      short value) {
+    if (value == (short) 0) {
+      return sb.append('-').append(label);
+    } else if (value < 0) {
+      return sb.append(label).append(value);
+    }
+    return sb.append(label).append('+').append(value);
+  }
+
   public static LabelVote create(String label, short value) {
     return new AutoValue_LabelVote(LabelType.checkNameInternal(label), value);
   }
@@ -70,13 +80,9 @@ public abstract class LabelVote {
   public abstract short value();
 
   public String format() {
-    if (value() == (short) 0) {
+    // Max short string length is "-32768".length() == 6.
-      return '-' + label();
+    return appendTo(new StringBuilder(label().length() + 6), label(), value())
-    } else if (value() < 0) {
+        .toString();
-      return label() + value();
-    } else {
-      return label() + '+' + value();
-    }
   }
 
   public String formatWithEquals() {