SWF-163 code review

spring-webflow/src/main/java/org/springframework/webflow/config/scope/AbstractWebFlowScope.java

@@ -22,19 +22,17 @@ import org.springframework.beans.factory.config.Scope;
 import org.springframework.webflow.core.collection.MutableAttributeMap;
 import org.springframework.webflow.execution.FlowExecutionContext;
 import org.springframework.webflow.execution.FlowExecutionContextHolder;
-import org.springframework.webflow.execution.RequestContext;
+import org.springframework.webflow.execution.FlowSession;
 
 /**
- * Abstract {@link Scope} implementation that reads from a particular scope in
- * the current thread-bound {@link FlowExecutionContext} object.
- * 
+ * Base class for {@link Scope} implementations that access a Web Flow scope
+ * from the current thread-bound {@link FlowExecutionContext} object.
  * <p>
- * Subclasses simply need to implement {@link #getScope()} to instruct this
- * class which {@link FlowExecutionContext} scope to read attributes from.
- * 
+ * Subclasses simply need to implement {@link #getScope()} to return the
+ * {@link MutableAttributeMap scope map} to access.
  * <p>
  * Relies on a thread-bound
- * @{link FlowExecutionContext} instance wich is located through a
+ * @{link FlowExecutionContext} instance located through the
  * @{link FlowExecutionContextHolder}.
  * 
  * @see FlowExecutionContext
@@ -50,48 +48,39 @@ public abstract class AbstractWebFlowScope implements Scope {
 	 */
 	protected final Log logger = LogFactory.getLog(getClass());
 
-	/**
-	 * Template method that determines the actual target scope.
-	 * @return the target scope
-	 * @see RequestContext#getConversationScope()
-	 * @see RequestContext#getFlowScope()
-	 * @see RequestContext#getRequestScope()
-	 * @see RequestContext#getFlashScope()
-	 */
-	protected abstract MutableAttributeMap getScope();
-
 	public Object get(String name, ObjectFactory objectFactory) {
-		MutableAttributeMap scope;
-		try {
-			scope = getScope();
-		} catch (IllegalStateException e) {
-			throw new ScopedBeanException("Cannot retrieve scoped bean '" + name
-					+ "' before the scope has been populated");
-		}
+		MutableAttributeMap scope = getScope();
 		Object scopedObject = scope.get(name);
 		if (scopedObject == null) {
 			if (logger.isDebugEnabled()) {
-				logger.debug("Could not find existing scoped instance of '" + name + "'; creating new instace");
+				logger.debug("No scoped instance '" + name + "' found; creating new instance");
 			}
 			scopedObject = objectFactory.getObject();
 			scope.put(name, scopedObject);
-		} else {
+		}
+		else {
 			if (logger.isDebugEnabled()) {
-				logger.debug("Found existing scoped instance of '" + name + "'");
+				logger.debug("Returning scoped instance '" + name + "'");
 			}
 		}
 		return scopedObject;
 	}
 
 	public Object remove(String name) {
-		try {
-			return getScope().remove(name);
-		} catch (IllegalStateException e) {
-			throw new ScopedBeanException("Cannot remove scoped bean '" + name
-					+ "' before the scope has been populated");
-		}
+		return getScope().remove(name);
 	}
 
+	/**
+	 * Template method that returns the target scope map.
+	 * @return the target scope map
+	 * @see FlowExecutionContext#getConversationScope()
+	 * @see FlowExecutionContext#getActiveSession()
+	 * @see FlowSession#getFlashMap()
+	 * @see FlowSession#getScope()
+	 * @throws IllegalStateException if the scope could not be accessed
+	 */
+	protected abstract MutableAttributeMap getScope() throws IllegalStateException;
+	
 	/**
 	 * Always returns <code>null</code> as most Spring Web Flow scopes do not
 	 * have obvious conversation ids. Subclasses should override this method
@@ -118,8 +107,9 @@ public abstract class AbstractWebFlowScope implements Scope {
 	 * Returns the current flow execution context. Used by subclasses to easily
 	 * get access to the thread-bound flow execution context.
 	 * @return the current thread-bound flow execution context
+	 * @throws IllegalStateException if the current flow execution context is not bound
 	 */
-	protected FlowExecutionContext getFlowExecutionContext() {
+	protected FlowExecutionContext getFlowExecutionContext() throws IllegalStateException {
 		return FlowExecutionContextHolder.getFlowExecutionContext();
 	}
 

spring-webflow/src/main/java/org/springframework/webflow/config/scope/ConversationScope.java

@@ -28,9 +28,7 @@ import org.springframework.webflow.execution.FlowExecution;
  * @since 1.1
  */
 public class ConversationScope extends AbstractWebFlowScope {
-
 	protected MutableAttributeMap getScope() {
 		return getFlowExecutionContext().getConversationScope();
 	}
-
 }

spring-webflow/src/main/java/org/springframework/webflow/config/scope/FlashScope.java

@@ -28,9 +28,7 @@ import org.springframework.webflow.execution.FlowSession;
  * @since 1.1
  */
 public class FlashScope extends AbstractWebFlowScope {
-
 	protected MutableAttributeMap getScope() {
 		return getFlowExecutionContext().getActiveSession().getFlashMap();
 	}
-
 }

spring-webflow/src/main/java/org/springframework/webflow/config/scope/FlowScope.java

@@ -28,9 +28,7 @@ import org.springframework.webflow.execution.FlowSession;
  * @since 1.1
  */
 public class FlowScope extends AbstractWebFlowScope {
-
 	protected MutableAttributeMap getScope() {
 		return getFlowExecutionContext().getActiveSession().getScope();
 	}
-
 }

spring-webflow/src/main/java/org/springframework/webflow/config/scope/ScopedBeanException.java

@@ -1,47 +0,0 @@
-/*
- * Copyright 2004-2007 the original author or authors.
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *      http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-package org.springframework.webflow.config.scope;
-
-import org.springframework.webflow.core.FlowException;
-
-/**
- * Thrown when a bean cannot be retrieved because the scope it resides in has
- * not yet been populated. This might occur if the bean is being referenced
- * before the FlowExecution has been retrieved.
- * 
- * @author Ben Hale
- * @since 1.1
- */
-public class ScopedBeanException extends FlowException {
-
-	/**
-	 * Create a new scoped bean exception.
-	 * @param message the message
-	 * @param cause the root cause of the failure
-	 */
-	public ScopedBeanException(String message, Throwable cause) {
-		super(message, cause);
-	}
-
-	/**
-	 * Create a new scoped bean exception.
-	 * @param message the message
-	 */
-	public ScopedBeanException(String message) {
-		super(message);
-	}
-
-}

spring-webflow/src/main/java/org/springframework/webflow/execution/FlowExecutionContextHolder.java

@@ -51,7 +51,7 @@ public class FlowExecutionContextHolder {
 	 * @throws IllegalStateException if no FlowExecutionContext is bound to this
 	 * thread
 	 */
-	public static FlowExecutionContext getFlowExecutionContext() {
+	public static FlowExecutionContext getFlowExecutionContext() throws IllegalStateException {
 		Assert.state(flowExecutionContextHolder.get() != null,
 				"No flow execution context is bound to this thread");
 		return (FlowExecutionContext) flowExecutionContextHolder.get();

spring-webflow/src/test/java/org/springframework/webflow/config/scope/ConversationScopeTests.java

@@ -70,8 +70,8 @@ public class ConversationScopeTests extends TestCase {
 		StubObjectFactory factory = new StubObjectFactory();
 		try {
 			scope.get("name", factory);
-			fail("Should have thrown a ScopedBeanException without a request context");
-		} catch (ScopedBeanException e) {
+			fail("Should have thrown a IllegalStateException without a request context");
+		} catch (IllegalStateException e) {
 		}
 	}
 
@@ -98,8 +98,8 @@ public class ConversationScopeTests extends TestCase {
 		FlowExecutionContextHolder.setFlowExecutionContext(null);
 		try {
 			scope.remove("name");
-			fail("Should have thrown a ScopedBeanException without a request context");
-		} catch (ScopedBeanException e) {
+			fail("Should have thrown a IllegalStateException without a request context");
+		} catch (IllegalStateException e) {
 		}
 	}
 

spring-webflow/src/test/java/org/springframework/webflow/config/scope/FlashScopeTests.java

@@ -70,8 +70,8 @@ public class FlashScopeTests extends TestCase {
 		StubObjectFactory factory = new StubObjectFactory();
 		try {
 			scope.get("name", factory);
-			fail("Should have thrown a ScopedBeanException without a request context");
-		} catch (ScopedBeanException e) {
+			fail("Should have thrown a IllegalStateException without a request context");
+		} catch (IllegalStateException e) {
 		}
 	}
 
@@ -100,8 +100,8 @@ public class FlashScopeTests extends TestCase {
 		FlowExecutionContextHolder.setFlowExecutionContext(null);
 		try {
 			scope.remove("name");
-			fail("Should have thrown a ScopedBeanException without a request context");
-		} catch (ScopedBeanException e) {
+			fail("Should have thrown a IllegalStateException without a request context");
+		} catch (IllegalStateException e) {
 		}
 	}
 

spring-webflow/src/test/java/org/springframework/webflow/config/scope/FlowScopeTests.java

@@ -70,8 +70,7 @@ public class FlowScopeTests extends TestCase {
 		StubObjectFactory factory = new StubObjectFactory();
 		try {
 			scope.get("name", factory);
-			fail("Should have thrown a ScopedBeanException without a request context");
-		} catch (ScopedBeanException e) {
+		} catch (IllegalStateException e) {
 		}
 	}
 
@@ -98,8 +97,8 @@ public class FlowScopeTests extends TestCase {
 		FlowExecutionContextHolder.setFlowExecutionContext(null);
 		try {
 			scope.remove("name");
-			fail("Should have thrown a ScopedBeanException without a request context");
-		} catch (ScopedBeanException e) {
+			fail("Should have thrown a IllegalStateException without a request context");
+		} catch (IllegalStateException e) {
 		}
 	}