From 8f19f1d70630f210f33ebd51d42d04ca9727ff36 Mon Sep 17 00:00:00 2001 From: Michal Kordas Date: Wed, 8 Jul 2015 22:23:58 +0200 Subject: [PATCH] Remove non-testable reflection code from HandlerFactory. #1270 In `HandlerFactory` any reflection-related exception would mean programmer's mistake with no recovery. In such cases checked exceptions are useless, so new methods in utils are provided to wrap any checked exceptions into unchecked ones. --- pom.xml | 2 +- .../puppycrawl/tools/checkstyle/Utils.java | 33 ++++++++++++ .../checks/indentation/HandlerFactory.java | 51 +++++-------------- .../tools/checkstyle/UtilsTest.java | 43 ++++++++++++++++ 4 files changed, 89 insertions(+), 40 deletions(-) diff --git a/pom.xml b/pom.xml index 7667a9891..a039b2e29 100644 --- a/pom.xml +++ b/pom.xml @@ -1138,7 +1138,7 @@ .*.checks.indentation.ElseHandler75100 .*.checks.indentation.AbstractExpressionHandler9197 .*.checks.indentation.ForHandler7595 - .*.checks.indentation.HandlerFactory7781 + .*.checks.indentation.HandlerFactory81100 .*.checks.indentation.ImportHandler5087 .*.checks.indentation.IndentationCheck10093 .*.checks.indentation.IndexHandler10075 diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/Utils.java b/src/main/java/com/puppycrawl/tools/checkstyle/Utils.java index 4f4d24b11..3ade1fc75 100644 --- a/src/main/java/com/puppycrawl/tools/checkstyle/Utils.java +++ b/src/main/java/com/puppycrawl/tools/checkstyle/Utils.java @@ -24,7 +24,9 @@ import com.puppycrawl.tools.checkstyle.api.TokenTypes; import org.apache.commons.beanutils.ConversionException; import java.io.File; +import java.lang.reflect.Constructor; import java.lang.reflect.Field; +import java.lang.reflect.InvocationTargetException; import java.nio.file.Path; import java.nio.file.Paths; import java.util.ResourceBundle; @@ -336,4 +338,35 @@ public final class Utils { public static boolean isCommentType(String type) { return isCommentType(getTokenId(type)); } + + /** + * @param targetClass from which constructor is returned + * @param parameterTypes of constructor + * @return constructor of targetClass or {@link IllegalStateException} if any exception occurs + * @see Class#getConstructor(Class[]) + */ + public static Constructor getConstructor(Class targetClass, Class... parameterTypes) { + try { + return targetClass.getConstructor(parameterTypes); + } + catch (NoSuchMethodException ex) { + throw new IllegalStateException(ex); + } + } + + /** + * @param constructor to invoke + * @param parameters to pass to constructor + * @param type of constructor + * @return new instance of class or {@link IllegalStateException} if any exception occurs + * @see Constructor#newInstance(Object...) + */ + public static T invokeConstructor(Constructor constructor, Object... parameters) { + try { + return constructor.newInstance(parameters); + } + catch (InstantiationException | IllegalAccessException | InvocationTargetException ex) { + throw new IllegalStateException(ex); + } + } } diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/checks/indentation/HandlerFactory.java b/src/main/java/com/puppycrawl/tools/checkstyle/checks/indentation/HandlerFactory.java index cec1e7305..186ed7fb0 100644 --- a/src/main/java/com/puppycrawl/tools/checkstyle/checks/indentation/HandlerFactory.java +++ b/src/main/java/com/puppycrawl/tools/checkstyle/checks/indentation/HandlerFactory.java @@ -20,14 +20,12 @@ package com.puppycrawl.tools.checkstyle.checks.indentation; import com.google.common.collect.Maps; +import com.puppycrawl.tools.checkstyle.Utils; import com.puppycrawl.tools.checkstyle.api.DetailAST; import com.puppycrawl.tools.checkstyle.api.TokenTypes; import java.lang.reflect.Constructor; -import java.lang.reflect.InvocationTargetException; import java.util.Map; import java.util.Set; -import org.apache.commons.logging.Log; -import org.apache.commons.logging.LogFactory; /** * Factory for handlers. Looks up constructor via reflection. @@ -35,9 +33,6 @@ import org.apache.commons.logging.LogFactory; * @author jrichard */ public class HandlerFactory { - /** Logger for indentation check. */ - private static final Log LOG = LogFactory.getLog(HandlerFactory.class); - /** * Registered handlers. */ @@ -90,19 +85,12 @@ public class HandlerFactory { * the handler to register */ private void register(int type, Class handlerClass) { - try { - final Constructor ctor = handlerClass - .getConstructor(new Class[] {IndentationCheck.class, - DetailAST.class, // current AST - AbstractExpressionHandler.class, // parent - }); - typeHandlers.put(type, ctor); - } - catch (final NoSuchMethodException | SecurityException e) { - final String message = "couldn't find ctor for " + handlerClass; - LOG.debug(message, e); - throw new RuntimeException(message); - } + final Constructor ctor = Utils.getConstructor(handlerClass, + IndentationCheck.class, + DetailAST.class, // current AST + AbstractExpressionHandler.class // parent + ); + typeHandlers.put(type, ctor); } /** @@ -154,26 +142,11 @@ public class HandlerFactory { } AbstractExpressionHandler expHandler = null; - try { - final Constructor handlerCtor = - typeHandlers.get(ast.getType()); - if (handlerCtor != null) { - expHandler = (AbstractExpressionHandler) handlerCtor.newInstance( - indentCheck, ast, parent); - } - } - catch (final InstantiationException | InvocationTargetException e) { - final String message = "couldn't instantiate constructor for " + ast; - LOG.debug(message, e); - throw new RuntimeException(message); - } - catch (final IllegalAccessException e) { - final String message = "couldn't access constructor for " + ast; - LOG.debug(message, e); - throw new RuntimeException(message); - } - if (expHandler == null) { - throw new RuntimeException("no handler for type " + ast.getType()); + final Constructor handlerCtor = + typeHandlers.get(ast.getType()); + if (handlerCtor != null) { + expHandler = (AbstractExpressionHandler) Utils.invokeConstructor( + handlerCtor, indentCheck, ast, parent); } return expHandler; } diff --git a/src/test/java/com/puppycrawl/tools/checkstyle/UtilsTest.java b/src/test/java/com/puppycrawl/tools/checkstyle/UtilsTest.java index 2dbf60e78..70452d279 100644 --- a/src/test/java/com/puppycrawl/tools/checkstyle/UtilsTest.java +++ b/src/test/java/com/puppycrawl/tools/checkstyle/UtilsTest.java @@ -26,9 +26,12 @@ import static com.puppycrawl.tools.checkstyle.Utils.fileExtensionMatches; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; +import java.lang.reflect.Constructor; import java.io.File; import java.io.IOException; +import java.util.Dictionary; import org.apache.commons.beanutils.ConversionException; import org.junit.Test; @@ -124,4 +127,44 @@ public class UtilsTest { boolean result = Utils.isPatternValid("some[invalidPattern"); assertFalse(result); } + + @Test + public void testGetExistingConstructor() throws NoSuchMethodException { + Constructor constructor = Utils.getConstructor(String.class, String.class); + + assertEquals(String.class.getConstructor(String.class), constructor); + } + + @Test + public void testGetNonExistingConstructor() throws NoSuchMethodException { + try { + Utils.getConstructor(Math.class); + fail(); + } + catch (IllegalStateException expected) { + assertEquals(NoSuchMethodException.class, expected.getCause().getClass()); + } + } + + @Test + public void testInvokeConstructor() throws NoSuchMethodException { + Constructor constructor = String.class.getConstructor(String.class); + + String constructedString = Utils.invokeConstructor(constructor, "string"); + + assertEquals("string", constructedString); + } + + @Test + public void testInvokeConstructorThatFails() throws NoSuchMethodException { + Constructor constructor = Dictionary.class.getConstructor(); + + try { + Utils.invokeConstructor(constructor); + fail(); + } + catch (IllegalStateException expected) { + assertEquals(InstantiationException.class, expected.getCause().getClass()); + } + } }