Remove COW list based on feedback

Author: agentgt

jooby/src/main/java/io/jooby/spi/ValueConverters.java

@@ -1,7 +1,8 @@
 package io.jooby.spi;
 
+import java.util.ArrayList;
+import java.util.List;
 import java.util.ServiceLoader;
-import java.util.concurrent.CopyOnWriteArrayList;
 
 import javax.annotation.Nullable;
 
@@ -19,21 +20,62 @@
 public final class ValueConverters {
 
   // Allow thread safe adding of ValueConverters.
-  private final CopyOnWriteArrayList<ValueConverter> valueConverters;
+  private final Iterable<ValueConverter> valueConverters;
 
   // Initialization on demand
   private static final class Hidden {
 
-    private static final ValueConverters INSTANCE = ValueConverters.create().fromServiceLoader();
+    private static volatile ValueConverters instance = ValueConverters.builder().fromServiceLoader().build();
   }
 
-  private ValueConverters(CopyOnWriteArrayList<ValueConverter> valueConverters) {
+  private ValueConverters(Iterable<ValueConverter> valueConverters) {
     super();
     this.valueConverters = valueConverters;
   }
 
-  static ValueConverters create() {
-    return new ValueConverters(new CopyOnWriteArrayList<>());
+  static Builder builder() {
+    return new Builder();
+  }
+
+  static final class Builder {
+
+    private final List<ValueConverter> valueConverters = new ArrayList<>();
+
+    Builder fromServiceLoader() {
+      ServiceLoader<ValueConverter> sl = ServiceLoader.load(ValueConverter.class);
+      // If any failes to load we will fail entirely.
+      // The value converters found earlier in the classpath take precedence.
+      sl.forEach(this::add);
+      return this;
+    }
+
+    /**
+     * You can add value converters programmatic. For now its protected. Its
+     * also to aid unit testing since serviceloader is inherently static
+     * singleton.
+     *
+     * @param vc
+     * @return
+     */
+    Builder add(ValueConverter vc) {
+      valueConverters.add(vc);
+      return this;
+    }
+
+    Builder clear() {
+      valueConverters.clear();
+      return this;
+    }
+
+    ValueConverters build() {
+      return new ValueConverters(valueConverters);
+    }
+
+    ValueConverters set() {
+      ValueConverters vc = build();
+      Hidden.instance = vc;
+      return vc;
+    }
   }
 
   /**
@@ -44,7 +86,8 @@ static ValueConverters create() {
    * @param c
    *          desired type
    * @return the type if converted or null if conversion was not possible.
-   * @throws TypeMismatchException failure in a converter
+   * @throws TypeMismatchException
+   *           failure in a converter
    */
   public @Nullable Object convert(ValueContainer v, Class<?> c) throws TypeMismatchException {
     Object result = null;
@@ -59,37 +102,13 @@ static ValueConverters create() {
     return result;
   }
 
-  ValueConverters fromServiceLoader() {
-    ServiceLoader<ValueConverter> sl = ServiceLoader.load(ValueConverter.class);
-    // If any failes to load we will fail entirely.
-    // The value converters found earlier in the classpath take precedence.
-    sl.forEach(this::add);
-    return this;
-  }
-
-  /**
-   * You can add value converters programmatic. For now its protected. Its also
-   * to aid unit testing since serviceloader is inherently static singleton.
-   *
-   * @param vc
-   * @return
-   */
-  /* private */ ValueConverters add(ValueConverter vc) {
-    valueConverters.add(vc);
-    return this;
-  }
-
-  ValueConverters clear() {
-    valueConverters.clear();
-    return this;
-  }
-
   /**
    * The ValueConverters singleton usually preloaded by the ServiceLoader.
+   *
    * @return the shared singleton used by Jooby
    */
   public static ValueConverters getInstance() {
-    return ValueConverters.Hidden.INSTANCE;
+    return ValueConverters.Hidden.instance;
   }
 
 }

jooby/src/test/java/io/jooby/spi/ValueConvertersTest.java

@@ -11,31 +11,29 @@
 import io.jooby.QueryString;
 import io.jooby.internal.UrlParser;
 
-
 class ValueConvertersTest {
 
   @AfterAll
   static void restoreFromServiceLoader() {
-    //Restore ValueConvert list for other unit tests.
-    ValueConverters.getInstance().clear().fromServiceLoader();
+    // Restore ValueConvert list for other unit tests.
+    ValueConverters.builder().fromServiceLoader().set();
   }
-  
+
   @Test
   void testConvert() {
-    ValueConverters.getInstance()
-      .clear()
-      .add((value, type) -> { 
+    ValueConverters.builder().add((value, type) -> {
       if (type == MyValue.class) {
         MyValue mv = new MyValue();
-        // we have chosen simple parameters names to make sure we don't get a false positve 
+        // we have chosen simple parameters names to make sure we don't get a
+        // false positve
         // from the reflection converter.
         mv.name = value.get("n").value();
-        //TODO: ValueContainer probably should have primitive convert methods.
+        // TODO: ValueContainer probably should have primitive convert methods.
         mv.order = Integer.parseInt(value.get("o").value());
         return mv;
       }
       return null;
-    });
+    }).set();
     queryString("n=stuff&o=1", queryString -> {
       MyValue mv = queryString.to(MyValue.class);
       assertEquals("stuff", mv.name);
@@ -45,20 +43,18 @@ void testConvert() {
       try {
         queryString.to(MyValue.class);
         fail();
-      }
-      catch (MissingValueException mve) {
+      } catch (MissingValueException mve) {
         assertEquals("Missing value: 'o'", mve.getMessage());
       }
     });
   }
-  
+
   static class MyValue {
+
     public String name;
     public int order;
   }
-  
-  
-  
+
   private void queryString(String queryString, Consumer<QueryString> consumer) {
     consumer.accept(UrlParser.queryString(queryString));
   }