Try to simplify server method definition

core/src/main/java/io/grpc/ServerServiceDefinition.java

@@ -31,10 +31,13 @@
 
 package io.grpc;
 
-import com.google.common.base.Preconditions;
-import com.google.common.collect.ImmutableList;
+import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Preconditions.checkState;
+
 import com.google.common.collect.ImmutableMap;
 
+import java.util.Collection;
 import java.util.HashMap;
 import java.util.Map;
 
@@ -45,14 +48,12 @@ public final class ServerServiceDefinition {
   }
 
   private final String name;
-  private final ImmutableList<ServerMethodDefinition<?, ?>> methods;
-  private final ImmutableMap<String, ServerMethodDefinition<?, ?>> methodLookup;
+  private final ImmutableMap<String, ServerMethodDefinition<?, ?>> methods;
 
-  private ServerServiceDefinition(String name, ImmutableList<ServerMethodDefinition<?, ?>> methods,
-      Map<String, ServerMethodDefinition<?, ?>> methodLookup) {
-    this.name = name;
-    this.methods = methods;
-    this.methodLookup = ImmutableMap.copyOf(methodLookup);
+  private ServerServiceDefinition(
+      String name, Map<String, ServerMethodDefinition<?, ?>> methods) {
+    this.name = checkNotNull(name);
+    this.methods = ImmutableMap.copyOf(methods);
   }
 
   /** Simple name of the service. It is not an absolute path. */
@@ -60,8 +61,8 @@ public final class ServerServiceDefinition {
     return name;
   }
 
-  public ImmutableList<ServerMethodDefinition<?, ?>> getMethods() {
-    return methods;
+  public Collection<ServerMethodDefinition<?, ?>> getMethods() {
+    return methods.values();
   }
 
   /**
@@ -70,16 +71,14 @@ public final class ServerServiceDefinition {
    * @param name the fully qualified name without leading slash. E.g., "com.foo.Foo/Bar"
    */
   public ServerMethodDefinition<?, ?> getMethod(String name) {
-    return methodLookup.get(name);
+    return methods.get(name);
   }
 
   /** Builder for constructing Service instances. */
   public static final class Builder {
     private final String serviceName;
-    private final ImmutableList.Builder<ServerMethodDefinition<?, ?>> methods
-        = ImmutableList.builder();
-    private final Map<String, ServerMethodDefinition<?, ?>> methodLookup
-        = new HashMap<String, ServerMethodDefinition<?, ?>>();
+    private final Map<String, ServerMethodDefinition<?, ?>> methods = 
+        new HashMap<String, ServerMethodDefinition<?, ?>>();
 
     private Builder(String serviceName) {
       this.serviceName = serviceName;
@@ -96,29 +95,27 @@ public final class ServerServiceDefinition {
       // TODO(zhangkun83): since the handler map uses fully qualified names as keys, we should
       // consider removing ServerServiceDefinition to and let the registry to have a big map of
       // handlers.
-      Preconditions.checkArgument(
+      checkArgument(
           serviceName.equals(MethodDescriptor.extractFullServiceName(method.getFullMethodName())),
           "Service name mismatch. Expected service name: '%s'. Actual method name: '%s'.",
           this.serviceName, method.getFullMethodName());
 
       return addMethod(new ServerMethodDefinition<ReqT, RespT>(
-          Preconditions.checkNotNull(method, "method must not be null"),
-          Preconditions.checkNotNull(handler, "handler must not be null")));
+          checkNotNull(method, "method must not be null"),
+          checkNotNull(handler, "handler must not be null")));
     }
 
     /** Add a method to be supported by the service. */
     public <ReqT, RespT> Builder addMethod(ServerMethodDefinition<ReqT, RespT> def) {
-      if (methodLookup.containsKey(def.getMethodDescriptor().getFullMethodName())) {
-        throw new IllegalStateException("Method by same name already registered");
-      }
-      methodLookup.put(def.getMethodDescriptor().getFullMethodName(), def);
-      methods.add(def);
+      String name = def.getMethodDescriptor().getFullMethodName();
+      checkState(!methods.containsKey(name), "Method by same name already registered: %s", name);
+      methods.put(name, def);
       return this;
     }
 
     /** Construct new ServerServiceDefinition. */
     public ServerServiceDefinition build() {
-      return new ServerServiceDefinition(serviceName, methods.build(), methodLookup);
+      return new ServerServiceDefinition(serviceName, methods);
     }
   }
 }

core/src/test/java/io/grpc/MutableHandlerRegistryImplTest.java

@@ -31,6 +31,8 @@
 
 package io.grpc;
 
+import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.collect.Iterables.getOnlyElement;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
@@ -38,8 +40,6 @@ import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.Mockito.mock;
 
-import com.google.common.base.Preconditions;
-
 import io.grpc.HandlerRegistry.Method;
 import io.grpc.MethodDescriptor.MethodType;
 
@@ -65,7 +65,8 @@ public class MutableHandlerRegistryImplTest {
               requestMarshaller, responseMarshaller),
           handler).build();
   @SuppressWarnings("rawtypes")
-  private ServerMethodDefinition flowMethodDefinition = basicServiceDefinition.getMethods().get(0);
+  private ServerMethodDefinition flowMethodDefinition = 
+      getOnlyElement(basicServiceDefinition.getMethods());
   private ServerServiceDefinition multiServiceDefinition = ServerServiceDefinition.builder("multi")
       .addMethod(
           MethodDescriptor.create(MethodType.UNKNOWN, "multi", "couple",
@@ -77,10 +78,10 @@ public class MutableHandlerRegistryImplTest {
           handler).build();
   @SuppressWarnings("rawtypes")
   private ServerMethodDefinition coupleMethodDefinition =
-      Preconditions.checkNotNull(multiServiceDefinition.getMethod("multi/couple"));
+      checkNotNull(multiServiceDefinition.getMethod("multi/couple"));
   @SuppressWarnings("rawtypes")
   private ServerMethodDefinition fewMethodDefinition =
-      Preconditions.checkNotNull(multiServiceDefinition.getMethod("multi/few"));
+      checkNotNull(multiServiceDefinition.getMethod("multi/few"));
 
   /** Final checks for all tests. */
   @After
@@ -143,7 +144,7 @@ public class MutableHandlerRegistryImplTest {
         .addMethod(MethodDescriptor.create(MethodType.UNKNOWN, "basic", "another",
               requestMarshaller, responseMarshaller), handler).build();
     ServerMethodDefinition<?, ?> anotherMethodDefinition =
-        replaceServiceDefinition.getMethods().get(0);
+        replaceServiceDefinition.getMethod("basic/another");
     assertSame(basicServiceDefinition, registry.addService(replaceServiceDefinition));
 
     assertNull(registry.lookupMethod("basic/flow"));

core/src/test/java/io/grpc/ServerInterceptorsTest.java

@@ -31,6 +31,7 @@
 
 package io.grpc;
 
+import static com.google.common.collect.Iterables.getOnlyElement;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertSame;
 import static org.mockito.Matchers.same;
@@ -243,7 +244,7 @@ public class ServerInterceptorsTest {
     if (serviceDef.getMethods().size() != 1) {
       throw new AssertionError("Not exactly one method present");
     }
-    return (ServerMethodDefinition<String, Integer>) serviceDef.getMethods().get(0);
+    return (ServerMethodDefinition<String, Integer>) getOnlyElement(serviceDef.getMethods());
   }
 
   @SuppressWarnings("unchecked")