fix:

- NPE in ConverterPool
- InvocationTargetException in ExperimentFactory
improve:
- type safety of the converters
add:
- TODOs/FIXMEs regarding the usage of the Converter.setFormat() method

SVN: 417

common/source/java/ch/systemsx/cisd/common/converter/Converter.java

@@ -24,12 +24,25 @@ package ch.systemsx.cisd.common.converter;
  */
 public interface Converter<T>
 {
-    /** Convert the specified input object into an output object of the specified type. */
+    /**
+     * Converts the <var>value</var> <code>String</code> into an object of type <code>T</code>.
+     * 
+     * @param value The <code>String</code> to convert. <code>null</code> is acceptable here.
+     * @return The converted value, or <code>null</code>, if the <var>value</var> was <code>null</code>.
+     */
     public T convert(String value);
 
-    /** Returns the default in case the value we try to convert is <code>null</code>. */
+    /**
+     * @return The default in case the value we try to convert is <code>null</code>. If the converter does not
+     *         support a default value, the return value of this method may be <code>null</code> as well.
+     */
     public T getDefaultValue();
-    
-    /** Sets a <code>String</code> format for this <code>Converter</code>. */
+
+    /**
+     * Sets a <code>String</code> format for this <code>Converter</code>. What this means exactly or whether this
+     * makes sense at all, depends on the type <code>T</code> of course.
+     * 
+     *  FIXME 2007-06-09, Bernd Rinn: Is this method reasonable? How will it be used?
+     */
     public void setFormat(String format);
 }

common/source/java/ch/systemsx/cisd/common/converter/ConverterPool.java

@@ -20,7 +20,7 @@ import java.util.HashMap;
 import java.util.Map;
 
 /**
- * A pool of {@link Converter}.
+ * A pool of {@link Converter}s.
  * <p>
  * You can have your own instance of this class or you can use the static accessor to get a 'public' instance.
  * </p>
@@ -29,7 +29,7 @@ import java.util.Map;
  */
 public final class ConverterPool
 {
-    
+
     /** A 'public' instance of this class. */
     private final static ConverterPool instance = new ConverterPool();
 
@@ -60,24 +60,31 @@ public final class ConverterPool
     }
 
     /**
-     * Unegisters corresponding <code>Converter</code> for given <code>Class</code>.
+     * Unregisters corresponding <code>Converter</code> for given <code>Class</code>.
      */
     public final <T> void unregisterConverter(Class<T> type)
     {
         converters.remove(type);
     }
 
-    /** Does the conversion. */
-    public final Object convert(String value, String format, Class type)
+    /** 
+     * Performs the conversion.
+     * 
+     *  @throws IllegalArgumentException If there is no converter for <var>type</var>.
+     */
+    public final <T> T convert(String value, String format, Class<T> type)
     {
-        Converter converter = converters.get(type);
-        if (format != null)
+        final Converter<T> converter = getConverter(type);
+        if (converter == null)
         {
-            converter.setFormat(format);
+            throw new IllegalArgumentException("No converter for type '" + type.getCanonicalName() + "'.");
         }
-        if (converter == null)
+        if (format != null)
         {
-            return value;
+            // TODO 2007-06-09, Bernd Rinn: Is it reasonable that we change the state of the converter when performing a
+            // conversion? If we use the format is such a way like here, does it make sense to have the format be a part
+            // of the Converter at all?
+            converter.setFormat(format);
         }
         if (value == null)
         {
@@ -85,4 +92,11 @@ public final class ConverterPool
         }
         return converter.convert(value);
     }
+
+    // The setter registerConverter() will ensure that no converter can be entered that is of the wrong type. 
+    @SuppressWarnings("unchecked")
+    private <T> Converter<T> getConverter(Class<T> type)
+    {
+        return converters.get(type);
+    }
 }
\ No newline at end of file

common/source/java/ch/systemsx/cisd/common/converter/DateConverter.java

@@ -67,17 +67,19 @@ public final class DateConverter implements Converter<Date>
      */
     public final Date convert(String value)
     {
-        Date defaultValue = getDefaultValue();
         if (dateFormat == null)
         {
-            return defaultValue;
+            // FIXME 2007-06-09, Bernd Rinn: This shoulnd't return a default value, because that is not a conversion of
+            // the value at all. It is, tought, unlclear, what it should do. Why is it possible to construct a
+            // DateConverer without a dateFormat in the first place?
+            return getDefaultValue();
         }
         try
         {
             return dateFormat.parse(value);
         } catch (ParseException ex)
         {
-            return defaultValue;
+            return getDefaultValue();
         }
     }