Clean up Binder, improve Javadocs

- Remove TODOs, commented out stub methods
- Improve null safety, documentation

Change-Id: I2d524e64691dd71b38dc9ecd3b189d41f05800fe

server/src/main/java/com/vaadin/tokka/data/Binder.java

@@ -44,8 +44,10 @@ import com.vaadin.tokka.ui.components.Listing;
  * A binder instance can be bound to a single bean instance at a time, but can
  * be rebound as needed. This allows usage patterns like a <i>master-details</i>
  * view, where a select component is used to pick the bean to edit.
+ * <p>
+ * Unless otherwise specified, {@code Binder} method arguments cannot be null.
  * 
- * @author Vaadin Ltd
+ * @author Vaadin Ltd.
  *
  * @param <T>
  *            the bean type
@@ -74,9 +76,12 @@ public class Binder<T> implements Serializable {
          * registration order, when the field value is saved to the backing
          * property. If any validator returns a failure, the property value is
          * not updated.
+         * <p>
+         * Unless otherwise specified, {@code Binding} method arguments cannot
+         * be null.
          * 
          * @param validator
-         *            the validator to add, not null
+         *            the validator to add
          * @return this binding, for chaining
          * @throws IllegalStateException
          *             if {@code bind} has already been called
@@ -102,12 +107,15 @@ public class Binder<T> implements Serializable {
          * Completes this binding using the given getter and setter functions
          * representing a backing bean property. The functions are used to
          * update the field value from the property and to store the field value
-         * to the property, respectively.
+         * to the property, respectively. The setter may be null; in that case
+         * the bound field will be read-only.
          * 
          * @param getter
-         *            the function to get the value of the property to the field
+         *            the function to get the value of the property to the
+         *            field, not null
          * @param setter
-         *            the function to save the field value to the property
+         *            the function to save the field value to the property or
+         *            null if read-only
          * @throws IllegalStateException
          *             if {@code bind} has already been called
          */
@@ -117,9 +125,9 @@ public class Binder<T> implements Serializable {
     /**
      * An internal implementation of {@code Binding}.
      * 
-     * @param U
+     * @param <U>
      *            the field value type
-     * @param V
+     * @param <V>
      *            the property value type
      */
     // TODO make protected, allow customization
@@ -173,6 +181,7 @@ public class Binder<T> implements Serializable {
          *            the bean to fetch the property value from
          */
         private void setFieldValue(T bean) {
+            assert bean != null;
             field.setValue(converter.toPresentation(getter.apply(bean)));
         }
 
@@ -184,6 +193,7 @@ public class Binder<T> implements Serializable {
          *            the bean to set the property value to
          */
         private void storeFieldValue(T bean) {
+            assert bean != null;
             if (setter == null) {
                 return;
             }
@@ -228,7 +238,7 @@ public class Binder<T> implements Serializable {
      * @param <V>
      *            the value type of the field
      * @param field
-     *            the field to be bound
+     *            the field to be bound, not null
      * @return the new binding
      */
     public <V> Binding<T, V> addField(HasValue<V> field) {
@@ -240,7 +250,7 @@ public class Binder<T> implements Serializable {
      * pair. If the Binder is already bound to some item, the new field will be
      * also bound to it.
      * <p>
-     * Not providing a setter implicitly sets the field to be read only.
+     * The setter may be null; in that case the field will be read-only.
      * <p>
      * Getters and setters can be used to make conversions happen. They are also
      * a good place to update the state of the field. You should avoid making
@@ -253,7 +263,7 @@ public class Binder<T> implements Serializable {
      * @param <V>
      *            the value type of the field
      * @param field
-     *            editor field, not null
+     *            the editor field to bind, not null
      * @param getter
      *            a function to fetch data from the bean, not null
      * @param setter
@@ -266,8 +276,8 @@ public class Binder<T> implements Serializable {
     }
 
     /**
-     * Binds the given bean to all the fields added to this Binder. If the bean
-     * is null, removes any existing binding.
+     * Binds the given bean to all the fields added to this Binder. To remove
+     * the binding, call {@link #unbind()}.
      * <p>
      * When a bean is bound, the field values are updated by invoking their
      * corresponding getter functions. Field values are saved into the bean
@@ -275,21 +285,25 @@ public class Binder<T> implements Serializable {
      * {@link #save()} method is called.
      * 
      * @param bean
-     *            edited bean or null to bind nothing
+     *            the bean to edit, not null
      */
     public void bind(T bean) {
+        Objects.requireNonNull(bean, "bean cannot be null");
         this.bean = bean;
-
-        if (bean == null) {
-            // TODO: clean up?
-            return;
-        }
-
         for (BindingImpl<?, ?> binding : bindings) {
             binding.setFieldValue(bean);
         }
     }
 
+    /**
+     * Unbinds the currently bound bean if any. If there is no bound bean, does
+     * nothing.
+     */
+    public void unbind() {
+        this.bean = null;
+    }
+
+    // FIXME Javadoc
     public <V> void addSelect(Listing<V> listing, Function<T, V> getter,
             BiConsumer<T, V> setter) {
         if (listing.getSelectionModel() instanceof SelectionModel.Single) {
@@ -301,6 +315,7 @@ public class Binder<T> implements Serializable {
         }
     }
 
+    // FIXME Javadoc
     public <V> void addMultiSelect(Listing<V> listing,
             Function<T, Collection<V>> getter,
             BiConsumer<T, Collection<V>> setter) {
@@ -313,60 +328,25 @@ public class Binder<T> implements Serializable {
         }
     }
 
-    // TODO: Is this correct return value? How should we manage the error
-    // messages from custom validation? Documentation?
-    // public abstract void addValidator(ValueProvider<T, String> validator);
-    // TODO: Needs remove method as well?
-
     /**
      * Saves any changes from the bound fields to the edited bean. Values that
-     * do not pass validation are not saved. If there is no currently bound
-     * bean, does nothing.
+     * do not pass validation are not saved.
+     * 
+     * @throws IllegalStateException
+     *             if there is no bound bean
      */
     public void save() {
         if (bean == null) {
-            return;
+            throw new IllegalStateException("Cannot save: no bean bound");
         }
-
         for (BindingImpl<?, ?> binding : bindings) {
             binding.storeFieldValue(bean);
         }
-
-        // TODO: Postvalidation
     }
 
-    /**
-     * Resets any changes in the fields to match values from the edited bean.
-     */
-    // TODO: Do we need this?
-    public void reset() {
-        // Re-bind to refresh all fields
-        bind(bean);
-    }
-
-    // Exists for the sake of making something before / after field update is
-    // processed
-    /**
-     * This method does prevalidation for every changed field. By overriding
-     * this method you can react to changes that need to happen when certain
-     * fields are edited. e.g. re-apply conversions.
-     * 
-     * @param field
-     *            changed field
-     */
-    protected <V> void handleChangeEvent(HasValue<V> field) {
-        // TODO: pre-validation ?
-    }
-
-    /*
-     * FIXME: Validation needs an idea. // TODO: Document protected abstract
-     * void validate();
-     * 
-     * // TODO: Document protected abstract void doJSRValidation();
-     */
-
     private <V> BindingImpl<V, V> createBinding(HasValue<V> field) {
         Objects.requireNonNull(field, "field cannot be null");
+
         BindingImpl<V, V> b = new BindingImpl<>();
         b.field = field;
         b.converter = Converter.identity();

server/src/main/java/com/vaadin/tokka/data/Converter.java

@@ -17,7 +17,9 @@
 package com.vaadin.tokka.data;
 
 import java.io.Serializable;
+import java.util.Objects;
 import java.util.function.Function;
+import java.util.function.Supplier;
 
 import com.vaadin.tokka.data.util.Result;
 
@@ -28,6 +30,9 @@ import com.vaadin.tokka.data.util.Result;
  * corresponding type used on the data model layer. In general, some values of
  * the presentation type might not be convertible to the model type, so the
  * {@code Converter} interface is not quite symmetrical.
+ * <p>
+ * Unless otherwise specified, {@code Converter} method arguments cannot be
+ * null.
  * 
  * @author Vaadin Ltd.
  *
@@ -53,18 +58,27 @@ public interface Converter<P, M> extends Serializable {
      * Constructs a converter from two functions. Any {@code Exception}
      * instances thrown from the {@code toModel} function are converted into
      * error-bearing {@code Result} objects using the given {@code onError}
-     * function.
+     * function. The {@code toPresentation} function should always succeed.
+     * <p>
+     * For example, the following converter converts between strings and
+     * integers:
+     * 
+     * <pre>
+     * Converter&lt;String, Integer&gt; c = Converter.from(
+     *         String::valueOf, Integer::valueOf,
+     *         e -> "value is not a valid number");
+     * </pre>
      * 
      * @param <P>
      *            the presentation type
      * @param <M>
      *            the model type
      * @param toModel
-     *            the function to convert to model
+     *            the function to convert to model, not null
      * @param toPresentation
-     *            the function to convert to presentation
+     *            the function to convert to presentation, not null
      * @param onError
-     *            the function to provide error messages
+     *            the function to provide error messages, not null
      * @return the new converter
      * 
      * @see Result
@@ -74,28 +88,54 @@ public interface Converter<P, M> extends Serializable {
             Function<M, P> toPresentation,
             Function<Exception, String> onError) {
 
+        Objects.requireNonNull(toModel, "toModel cannot be null");
+        Objects.requireNonNull(toPresentation, "toPresentation cannot be null");
+        Objects.requireNonNull(onError, "onError cannot be null");
+
         return from(val -> Result.of(() -> toModel.apply(val), onError),
                 toPresentation);
     }
 
     /**
-     * Constructs a converter from a filter and a function.
+     * Constructs a converter from two functions. The {@code toModel} function
+     * returns a {@code Result} object to represent the success or failure of
+     * the conversion. The {@code toPresentation} function should always
+     * succeed.
+     * <p>
+     * For example, the following converter converts between strings and
+     * integers:
+     * 
+     * <pre>
+     * Converter<String, Integer> stringToInt = Converter.from(
+     *         str -> {
+     *             try {
+     *                 return Result.ok(Integer.valueOf(str));
+     *             } catch (NumberFormatException e) {
+     *                 return Result.error("not a valid number: " + str);
+     *             }
+     *         },
+     *         String::valueOf);
+     * </pre>
      * 
      * @param <P>
      *            the presentation type
      * @param <M>
      *            the model type
      * @param toModel
-     *            the function to convert to model
+     *            the function to convert to model, not null
      * @param toPresentation
-     *            the function to convert to presentation
+     *            the function to convert to presentation, not null
      * @return the new converter
      * 
-     * @see Filter
+     * @see Result
      * @see Function
+     * @see Result#of(Supplier, Function)
      */
     public static <P, M> Converter<P, M> from(Function<P, Result<M>> toModel,
             Function<M, P> toPresentation) {
+        Objects.requireNonNull(toModel, "toModel cannot be null");
+        Objects.requireNonNull(toPresentation, "toPresentation cannot be null");
+
         return new Converter<P, M>() {
 
             @Override
@@ -117,9 +157,12 @@ public interface Converter<P, M> extends Serializable {
      * <p>
      * This method should never throw exceptions in case of invalid input, only
      * in actual exceptional conditions.
+     * <p>
+     * The behavior in case of a null value is implementation-defined and should
+     * be documented.
      * 
      * @param value
-     *            the value to convert
+     *            the value to convert, null handling is implementation-defined
      * @return the result of the conversion
      * 
      * @see #toPresentation(M)
@@ -133,9 +176,12 @@ public interface Converter<P, M> extends Serializable {
      * {@link #toModel(Object) toModel}, the conversion is expected not to fail;
      * a failure should imply either a bug in the application logic or invalid
      * data in the model.
+     * <p>
+     * The behavior in case of a null value is implementation-defined and should
+     * be documented.
      * 
      * @param value
-     *            the value to convert
+     *            the value to convert, null handling is implementation-defined
      * @return the converted value
      * 
      * @see #toModel(P)
@@ -151,7 +197,7 @@ public interface Converter<P, M> extends Serializable {
      * @param <T>
      *            the model type of the resulting converter
      * @param other
-     *            the converter to chain
+     *            the converter to chain, not null
      * @return a chained converter
      */
     public default <T> Converter<P, T> chain(Converter<M, T> other) {

server/src/main/java/com/vaadin/tokka/data/Validator.java

@@ -17,6 +17,7 @@
 package com.vaadin.tokka.data;
 
 import java.io.Serializable;
+import java.util.Objects;
 import java.util.function.Function;
 import java.util.function.Predicate;
 
@@ -62,9 +63,9 @@ public interface Validator<T> extends Function<T, Result<T>>, Serializable {
 
     /**
      * Returns a validator that chains this validator with the given function.
-     * The function may be another validator. The resulting validator first
-     * applies this validator, and if the value passes, then the given
-     * validator.
+     * Specifically, the function may be another validator. The resulting
+     * validator first applies this validator, and if the value passes, then the
+     * given validator.
      * <p>
      * For instance, the following chained validator checks if a number is
      * between 0 and 10, inclusive:
@@ -76,12 +77,13 @@ public interface Validator<T> extends Function<T, Result<T>>, Serializable {
      * </pre>
      * 
      * @param next
-     *            the filter to apply next
-     * @return a chained filter
+     *            the validator to apply next, not null
+     * @return a chained validator
      * 
      * @see #from(Predicate, String)
      */
     public default Validator<T> chain(Function<T, Result<T>> next) {
+        Objects.requireNonNull(next, "next cannot be null");
         return val -> apply(val).flatMap(next);
     }
 
@@ -103,13 +105,15 @@ public interface Validator<T> extends Function<T, Result<T>>, Serializable {
      * @param <T>
      *            the value type
      * @param guard
-     *            the function used to validate
+     *            the function used to validate, not null
      * @param errorMessage
-     *            the message returned if validation fails
+     *            the message returned if validation fails, not null
      * @return the new validator using the function
      */
     public static <T> Validator<T> from(Predicate<T> guard,
             String errorMessage) {
+        Objects.requireNonNull(guard, "guard cannot be null");
+        Objects.requireNonNull(errorMessage, "errorMessage cannot be null");
         return value -> {
             try {
                 if (guard.test(value)) {

server/src/main/java/com/vaadin/tokka/data/util/Result.java

@@ -17,6 +17,7 @@
 package com.vaadin.tokka.data.util;
 
 import java.io.Serializable;
+import java.util.Objects;
 import java.util.Optional;
 import java.util.function.Consumer;
 import java.util.function.Function;
@@ -29,6 +30,8 @@ import java.util.function.Supplier;
  * <p>
  * Result instances are created using the factory methods {@link #ok(R)} and
  * {@link #error(String)}, denoting success and failure respectively.
+ * <p>
+ * Unless otherwise specified, {@code Result} method arguments cannot be null.
  *
  * @param <R>
  *            the result value type
@@ -41,7 +44,7 @@ public interface Result<R> extends Serializable {
      * @param <R>
      *            the result value type
      * @param value
-     *            the result value
+     *            the result value, can be null
      * @return a successful result
      */
     public static <R> Result<R> ok(R value) {
@@ -58,6 +61,7 @@ public interface Result<R> extends Serializable {
      * @return a failure result
      */
     public static <R> Result<R> error(String message) {
+        Objects.requireNonNull(message, "message cannot be null");
         return new ResultImpl<R>(null, message);
     }
 
@@ -77,6 +81,9 @@ public interface Result<R> extends Serializable {
      */
     public static <R> Result<R> of(Supplier<R> supplier,
             Function<Exception, String> onError) {
+        Objects.requireNonNull(supplier, "supplier cannot be null");
+        Objects.requireNonNull(onError, "onError cannot be null");
+
         try {
             return ok(supplier.get());
         } catch (Exception e) {
@@ -119,9 +126,9 @@ public interface Result<R> extends Serializable {
      * this Result denotes a success or a failure, respectively.
      * 
      * @param ifOk
-     *            the function to call if success, not null
+     *            the function to call if success
      * @param ifError
-     *            the function to call if failure, not null
+     *            the function to call if failure
      */
     public void handle(Consumer<R> ifOk, Consumer<String> ifError);
 
@@ -152,6 +159,8 @@ class ResultImpl<R> implements Result<R> {
     @Override
     @SuppressWarnings("unchecked")
     public <S> Result<S> flatMap(Function<R, Result<S>> mapper) {
+        Objects.requireNonNull(mapper, "mapper cannot be null");
+
         if (value != null) {
             return mapper.apply(value);
         } else {
@@ -162,6 +171,8 @@ class ResultImpl<R> implements Result<R> {
 
     @Override
     public void handle(Consumer<R> ifOk, Consumer<String> ifError) {
+        Objects.requireNonNull(ifOk, "ifOk cannot be null");
+        Objects.requireNonNull(ifError, "ifError cannot be null");
         if (message == null) {
             ifOk.accept(value);
         } else {
@@ -173,5 +184,4 @@ class ResultImpl<R> implements Result<R> {
     public Optional<String> getMessage() {
         return Optional.ofNullable(message);
     }
-
 }

server/src/test/java/com/vaadin/tokka/data/BinderTest.java

@@ -69,14 +69,18 @@ public class BinderTest {
         assertEquals("Johannes", nameField.getValue());
     }
 
-    @Test
-    public void testNoValueChangesAfterUnbind() {
+    @Test(expected = IllegalStateException.class)
+    public void testSaveBeforeBindThrows() {
+        binder.save();
+    }
+
+    @Test(expected = IllegalStateException.class)
+    public void testSaveAfterUnbindThrows() {
         bindName();
 
-        binder.bind(null);
+        binder.unbind();
         nameField.setValue("Teemu");
         binder.save();
-        assertEquals("Johannes", p.getFirstName());
     }
 
     @Test