DATACMNS-578 - Polishing.

Added some JavaDoc where missing. Better variable names and slightly parameter ordering changes. Formatting in the unit tests and reference to the original ticket.

Original pull request: #98.

Author: odrotbohm

src/main/java/org/springframework/data/convert/BytecodeGeneratingEntityInstantiator.java

@@ -42,14 +42,13 @@
 import org.springframework.util.ClassUtils;
 
 /**
- * An {@link EntityInstantiator} that can generate byte code to speed-up dynamic object instantiation.
- * <p>
- * Uses the {@link PersistentEntity}'s {@link PreferredConstructor} to instantiate an instance of the entity by
- * dynamically generating factory methods with appropriate constructor invocations via ASM.
- * <p>
- * If we cannot generate byte code for a type, we gracefully fall-back to the {@link ReflectionEntityInstantiator}.
+ * An {@link EntityInstantiator} that can generate byte code to speed-up dynamic object instantiation. Uses the
+ * {@link PersistentEntity}'s {@link PreferredConstructor} to instantiate an instance of the entity by dynamically
+ * generating factory methods with appropriate constructor invocations via ASM. If we cannot generate byte code for a
+ * type, we gracefully fall-back to the {@link ReflectionEntityInstantiator}.
  * 
  * @author Thomas Darimont
+ * @author Oliver Gierke
  * @since 1.10
  */
 public enum BytecodeGeneratingEntityInstantiator implements EntityInstantiator {
@@ -68,15 +67,13 @@ public enum BytecodeGeneratingEntityInstantiator implements EntityInstantiator {
 	public <T, E extends PersistentEntity<? extends T, P>, P extends PersistentProperty<P>> T createInstance(E entity,
 			ParameterValueProvider<P> provider) {
 
-		EntityInstantiator ei = this.entityInstantiators.get(entity.getTypeInformation());
+		EntityInstantiator instantiator = this.entityInstantiators.get(entity.getTypeInformation());
 
-		if (ei != null) {
-			return ei.createInstance(entity, provider);
+		if (instantiator == null) {
+			instantiator = potentiallyCreateAndRegisterEntityInstantiator(entity);
 		}
 
-		ei = potentiallyCreateAndRegisterEntityInstantiator(entity);
-
-		return ei.createInstance(entity, provider);
+		return instantiator.createInstance(entity, provider);
 	}
 
 	/**
@@ -86,20 +83,20 @@ public <T, E extends PersistentEntity<? extends T, P>, P extends PersistentPrope
 	private synchronized EntityInstantiator potentiallyCreateAndRegisterEntityInstantiator(PersistentEntity<?, ?> entity) {
 
 		Map<TypeInformation<?>, EntityInstantiator> map = this.entityInstantiators;
+		EntityInstantiator instantiator = map.get(entity.getTypeInformation());
 
-		EntityInstantiator ei = map.get(entity.getTypeInformation());
-		if (ei != null) {
-			return ei;
+		if (instantiator != null) {
+			return instantiator;
 		}
 
-		ei = createEntityInstantiator(entity);
+		instantiator = createEntityInstantiator(entity);
 
 		map = new HashMap<TypeInformation<?>, EntityInstantiator>(map);
-		map.put(entity.getTypeInformation(), ei);
+		map.put(entity.getTypeInformation(), instantiator);
 
 		this.entityInstantiators = map;
 
-		return ei;
+		return instantiator;
 	}
 
 	/**
@@ -194,7 +191,10 @@ public Class<?> loadClass(String name, byte[] bytes) {
 	}
 
 	/**
+	 * Adapter to forward an invocation of the {@link EntityInstantiator} API to an {@link ObjectInstantiator}.
+	 * 
 	 * @author Thomas Darimont
+	 * @author Oliver Gierke
 	 */
 	private static class EntityInstantiatorAdapter implements EntityInstantiator {
 
@@ -203,18 +203,25 @@ private static class EntityInstantiatorAdapter implements EntityInstantiator {
 		private final ObjectInstantiator instantiator;
 
 		/**
-		 * @param instantiator
+		 * Creates a new {@link EntityInstantiatorAdapter} for the given {@link ObjectInstantiator}.
+		 * 
+		 * @param instantiator must not be {@literal null}.
 		 */
 		public EntityInstantiatorAdapter(ObjectInstantiator instantiator) {
 			this.instantiator = instantiator;
 		}
 
+		/*
+		 * (non-Javadoc)
+		 * @see org.springframework.data.convert.EntityInstantiator#createInstance(org.springframework.data.mapping.PersistentEntity, org.springframework.data.mapping.model.ParameterValueProvider)
+		 */
 		@Override
 		@SuppressWarnings("unchecked")
 		public <T, E extends PersistentEntity<? extends T, P>, P extends PersistentProperty<P>> T createInstance(E entity,
 				ParameterValueProvider<P> provider) {
 
-			Object[] params = extractInvocationArguments(provider, entity.getPersistenceConstructor());
+			Object[] params = extractInvocationArguments(entity.getPersistenceConstructor(), provider);
+
 			try {
 				return (T) instantiator.newInstance(params);
 			} catch (Exception e) {
@@ -223,19 +230,22 @@ public <T, E extends PersistentEntity<? extends T, P>, P extends PersistentPrope
 		}
 
 		/**
-		 * @param provider
-		 * @param prefCtor
+		 * Extracts the arguments required to invoce the given constructor from the given {@link ParameterValueProvider}.
+		 * 
+		 * @param constructor can be {@literal null}.
+		 * @param provider can be {@literal null}.
 		 * @return
 		 */
 		private <P extends PersistentProperty<P>, T> Object[] extractInvocationArguments(
-				ParameterValueProvider<P> provider, PreferredConstructor<? extends T, P> prefCtor) {
+				PreferredConstructor<? extends T, P> constructor, ParameterValueProvider<P> provider) {
 
-			if (provider == null || prefCtor == null || !prefCtor.hasParameters()) {
+			if (provider == null || constructor == null || !constructor.hasParameters()) {
 				return EMPTY_ARRAY;
 			}
 
 			List<Object> params = new ArrayList<Object>();
-			for (Parameter<?, P> parameter : prefCtor.getParameters()) {
+
+			for (Parameter<?, P> parameter : constructor.getParameters()) {
 				params.add(provider.getParameterValue(parameter));
 			}
 

src/test/java/org/springframework/data/convert/BytecodeGeneratingEntityInstantiatorUnitTests.java

@@ -47,6 +47,7 @@
  * Unit tests for {@link BytecodeGeneratingEntityInstantiator}.
  * 
  * @author Thomas Darimont
+ * @author Oliver Gierke
  */
 @RunWith(MockitoJUnitRunner.class)
 public class BytecodeGeneratingEntityInstantiatorUnitTests<P extends PersistentProperty<P>> {
@@ -88,7 +89,7 @@ public void instantiatesTypeWithPreferredConstructorUsingParameterValueProvider(
 	}
 
 	/**
-	 * @see DATACMNS-300
+	 * @see DATACMNS-300, DATACMNS-578
 	 */
 	@Test(expected = MappingInstantiationException.class)
 	@SuppressWarnings({ "unchecked", "rawtypes" })
@@ -101,7 +102,7 @@ public void throwsExceptionOnBeanInstantiationException() {
 	}
 
 	/**
-	 * @see DATACMNS-134
+	 * @see DATACMNS-134, DATACMNS-578
 	 */
 	@Test
 	public void createsInnerClassInstanceCorrectly() {
@@ -129,7 +130,7 @@ public void doWith(Field field) throws IllegalArgumentException, IllegalAccessEx
 	}
 
 	/**
-	 * @see DATACMNS-283
+	 * @see DATACMNS-283, DATACMNS-578
 	 */
 	@Test
 	@SuppressWarnings({ "unchecked", "rawtypes" })
@@ -237,15 +238,15 @@ public void instantiateObjCtor2ParamStringString() {
 
 		for (int i = 0; i < 2; i++) {
 			when(provider.getParameterValue(Mockito.any(Parameter.class))).thenReturn("FOO").thenReturn("BAR");
-			
+
 			Object instance = INSTANCE.createInstance(entity, provider);
 			assertTrue(instance instanceof ObjCtor2ParamStringString);
 			assertTrue(((ObjCtor2ParamStringString) instance).ctorInvoked);
 			assertThat(((ObjCtor2ParamStringString) instance).param1, is("FOO"));
 			assertThat(((ObjCtor2ParamStringString) instance).param2, is("BAR"));
 		}
 	}
-	
+
 	/**
 	 * @see DATACMNS-578
 	 */
@@ -260,14 +261,15 @@ public void instantiateObjectCtor1ParamInt() {
 		when(entity.getPersistenceConstructor()).thenReturn(constructor);
 
 		for (int i = 0; i < 2; i++) {
+
 			when(provider.getParameterValue(Mockito.any(Parameter.class))).thenReturn(42);
-			
+
 			Object instance = INSTANCE.createInstance(entity, provider);
 			assertTrue(instance instanceof ObjectCtor1ParamInt);
 			assertTrue("matches", ((ObjectCtor1ParamInt) instance).param1 == 42);
 		}
 	}
-	
+
 	/**
 	 * @see DATACMNS-578
 	 */
@@ -282,8 +284,9 @@ public void instantiateObjectCtor7ParamsString5IntsString() {
 		when(entity.getPersistenceConstructor()).thenReturn(constructor);
 
 		for (int i = 0; i < 2; i++) {
-			when(provider.getParameterValue(Mockito.any(Parameter.class))).thenReturn("A").thenReturn(1).thenReturn(2).thenReturn(3).thenReturn(4).thenReturn(5).thenReturn("B");
-			
+			when(provider.getParameterValue(Mockito.any(Parameter.class))).thenReturn("A").thenReturn(1).thenReturn(2)
+					.thenReturn(3).thenReturn(4).thenReturn(5).thenReturn("B");
+
 			Object instance = INSTANCE.createInstance(entity, provider);
 			assertTrue(instance instanceof ObjectCtor7ParamsString5IntsString);
 			assertThat(((ObjectCtor7ParamsString5IntsString) instance).param1, is("A"));
@@ -295,7 +298,7 @@ public void instantiateObjectCtor7ParamsString5IntsString() {
 			assertThat(((ObjectCtor7ParamsString5IntsString) instance).param7, is("B"));
 		}
 	}
-	
+
 	static class Foo {
 
 		Foo(String foo) {
@@ -324,13 +327,11 @@ public Sample(Long id, String name) {
 
 	/**
 	 * @author Thomas Darimont
-	 *
 	 */
 	public static class ObjCtorDefault {}
 
 	/**
 	 * @author Thomas Darimont
-	 *
 	 */
 	public static class ObjCtorNoArgs {
 
@@ -343,7 +344,6 @@ public ObjCtorNoArgs() {
 
 	/**
 	 * @author Thomas Darimont
-	 *
 	 */
 	public static class ObjCtor1ParamString {
 
@@ -358,7 +358,6 @@ public ObjCtor1ParamString(String param1) {
 
 	/**
 	 * @author Thomas Darimont
-	 *
 	 */
 	public static class ObjCtor2ParamStringString {
 
@@ -372,25 +371,23 @@ public ObjCtor2ParamStringString(String param1, String param2) {
 			this.param2 = param2;
 		}
 	}
-	
+
 	/**
 	 * @author Thomas Darimont
-	 *
 	 */
-	public static class ObjectCtor1ParamInt{
+	public static class ObjectCtor1ParamInt {
 
 		public int param1;
 
 		public ObjectCtor1ParamInt(int param1) {
 			this.param1 = param1;
 		}
 	}
-	
+
 	/**
 	 * @author Thomas Darimont
-	 *
 	 */
-	public static class ObjectCtor7ParamsString5IntsString{
+	public static class ObjectCtor7ParamsString5IntsString {
 
 		public String param1;
 		public int param2;