From 6f9fdc11981c92c8ddfd80a0cdd02ff95f41c66b Mon Sep 17 00:00:00 2001 From: Sebastian Bazley Date: Wed, 18 Jul 2012 17:58:22 +0000 Subject: [PATCH] LOGGING-128 - Static analysis suggests a number of potential improvements git-svn-id: https://svn.apache.org/repos/asf/commons/proper/logging/trunk@1363030 13f79535-47bb-0310-9956-ffa450edef68 --- RELEASE-NOTES.txt | 1 + .../apache/commons/logging/LogFactory.java | 17 ++++----- .../org/apache/commons/logging/LogSource.java | 21 +++-------- .../commons/logging/impl/Log4JLogger.java | 8 ++-- .../commons/logging/impl/LogFactoryImpl.java | 37 +++++-------------- .../logging/impl/ServletContextCleaner.java | 2 +- .../commons/logging/impl/WeakHashtable.java | 6 +-- 7 files changed, 33 insertions(+), 59 deletions(-) diff --git a/RELEASE-NOTES.txt b/RELEASE-NOTES.txt index eaa77b3..79c0eaf 100644 --- a/RELEASE-NOTES.txt +++ b/RELEASE-NOTES.txt @@ -21,6 +21,7 @@ TODO - rework notes to incorporate: Changes since 1.1.1 LOGGING-130 - Potential missing privileged block for class loader LOGGING-145 - LogFactoryImpl.setAttribute - possible NPE LOGGING-142 - Log4JLogger uses deprecated static members of Priority such as INFO +LOGGING-128 - Static analysis suggests a number of potential improvements $Id$ diff --git a/src/java/org/apache/commons/logging/LogFactory.java b/src/java/org/apache/commons/logging/LogFactory.java index 93c6413..6e7cce0 100644 --- a/src/java/org/apache/commons/logging/LogFactory.java +++ b/src/java/org/apache/commons/logging/LogFactory.java @@ -204,7 +204,7 @@ public abstract class LogFactory { * AccessControllers etc. It's more efficient to compute it once and * cache it here. */ - private static ClassLoader thisClassLoader; + private static final ClassLoader thisClassLoader; // ----------------------------------------------------------- Constructors @@ -720,6 +720,8 @@ public abstract class LogFactory { if (isDiagnosticsEnabled()) { logDiagnostic("Releasing factory for classloader " + objectId(classLoader)); } + // factories is not final and could be replaced in this block. + final Hashtable factories = LogFactory.factories; synchronized (factories) { if (classLoader == null) { if (nullClassLoaderFactory != null) { @@ -751,6 +753,8 @@ public abstract class LogFactory { if (isDiagnosticsEnabled()) { logDiagnostic("Releasing factory for all classloaders."); } + // factories is not final and could be replaced in this block. + final Hashtable factories = LogFactory.factories; synchronized (factories) { Enumeration elements = factories.elements(); while (elements.hasMoreElements()) { @@ -968,19 +972,15 @@ public abstract class LogFactory { */ private static LogFactory getCachedFactory(ClassLoader contextClassLoader) { - LogFactory factory = null; - if (contextClassLoader == null) { // We have to handle this specially, as factories is a Hashtable // and those don't accept null as a key value. // // nb: nullClassLoaderFactory might be null. That's ok. - factory = nullClassLoaderFactory; + return nullClassLoaderFactory; } else { - factory = (LogFactory) factories.get(contextClassLoader); + return (LogFactory) factories.get(contextClassLoader); } - - return factory; } /** @@ -1218,8 +1218,7 @@ public abstract class LogFactory { logDiagnostic(msg); } - ClassCastException ex = new ClassCastException(msg); - throw ex; + throw new ClassCastException(msg); } // Ignore exception, continue. Presumably the classloader was the diff --git a/src/java/org/apache/commons/logging/LogSource.java b/src/java/org/apache/commons/logging/LogSource.java index 58c47b5..546a82e 100644 --- a/src/java/org/apache/commons/logging/LogSource.java +++ b/src/java/org/apache/commons/logging/LogSource.java @@ -77,23 +77,15 @@ public class LogSource { // Is Log4J Available? try { - if (null != Class.forName("org.apache.log4j.Logger")) { - log4jIsAvailable = true; - } else { - log4jIsAvailable = false; - } + log4jIsAvailable = null != Class.forName("org.apache.log4j.Logger"); } catch (Throwable t) { log4jIsAvailable = false; } // Is JDK 1.4 Logging Available? try { - if ((null != Class.forName("java.util.logging.Logger")) && - (null != Class.forName("org.apache.commons.logging.impl.Jdk14Logger"))) { - jdk14IsAvailable = true; - } else { - jdk14IsAvailable = false; - } + jdk14IsAvailable = (null != Class.forName("java.util.logging.Logger")) && + (null != Class.forName("org.apache.commons.logging.impl.Jdk14Logger")); } catch (Throwable t) { jdk14IsAvailable = false; } @@ -162,7 +154,7 @@ public class LogSource { * of the log). */ static public void setLogImplementation(String classname) throws - LinkageError, ExceptionInInitializerError, + LinkageError, NoSuchMethodException, SecurityException, ClassNotFoundException { try { @@ -234,10 +226,9 @@ public class LogSource { */ static public Log makeNewLogInstance(String name) { - Log log = null; + Log log; try { - Object[] args = new Object[1]; - args[0] = name; + Object[] args = { name }; log = (Log) (logImplctor.newInstance(args)); } catch (Throwable t) { log = null; diff --git a/src/java/org/apache/commons/logging/impl/Log4JLogger.java b/src/java/org/apache/commons/logging/impl/Log4JLogger.java index ecf2c78..4332a44 100644 --- a/src/java/org/apache/commons/logging/impl/Log4JLogger.java +++ b/src/java/org/apache/commons/logging/impl/Log4JLogger.java @@ -60,7 +60,7 @@ public class Log4JLogger implements Log, Serializable { /** Logger name */ private String name = null; - private static Priority traceLevel; + private static final Priority traceLevel; // ------------------------------------------------------------ // Static Initializer. @@ -86,12 +86,14 @@ public class Log4JLogger implements Log, Serializable { // versions do not. If TRACE is not available, then we have to map // calls to Log.trace(...) onto the DEBUG level. + Priority _traceLevel; try { - traceLevel = (Priority) Level.class.getDeclaredField("TRACE").get(null); + _traceLevel = (Priority) Level.class.getDeclaredField("TRACE").get(null); } catch(Exception ex) { // ok, trace not available - traceLevel = Level.DEBUG; + _traceLevel = Level.DEBUG; } + traceLevel = _traceLevel; } diff --git a/src/java/org/apache/commons/logging/impl/LogFactoryImpl.java b/src/java/org/apache/commons/logging/impl/LogFactoryImpl.java index ea891cb..0f7aa50 100644 --- a/src/java/org/apache/commons/logging/impl/LogFactoryImpl.java +++ b/src/java/org/apache/commons/logging/impl/LogFactoryImpl.java @@ -24,9 +24,7 @@ import java.lang.reflect.Method; import java.net.URL; import java.security.AccessController; import java.security.PrivilegedAction; -import java.util.Enumeration; import java.util.Hashtable; -import java.util.Vector; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogConfigurationException; @@ -281,18 +279,7 @@ public class LogFactoryImpl extends LogFactory { * length array is returned. */ public String[] getAttributeNames() { - - Vector names = new Vector(); - Enumeration keys = attributes.keys(); - while (keys.hasMoreElements()) { - names.addElement((String) keys.nextElement()); - } - String results[] = new String[names.size()]; - for (int i = 0; i < results.length; i++) { - results[i] = (String) names.elementAt(i); - } - return (results); - + return (String[]) attributes.keySet().toArray(new String[attributes.size()]); } @@ -598,7 +585,7 @@ public class LogFactoryImpl extends LogFactory { */ protected Log newInstance(String name) throws LogConfigurationException { - Log instance = null; + Log instance; try { if (logConstructor == null) { instance = discoverLogImplementation(name); @@ -626,11 +613,7 @@ public class LogFactoryImpl extends LogFactory { // A problem occurred invoking the Constructor or Method // previously discovered Throwable c = e.getTargetException(); - if (c != null) { - throw new LogConfigurationException(c); - } else { - throw new LogConfigurationException(e); - } + throw new LogConfigurationException(c == null ? e : c); } catch (Throwable t) { // A problem occurred invoking the Constructor or Method // previously discovered @@ -1074,14 +1057,14 @@ public class LogFactoryImpl extends LogFactory { } } - Class c = null; + Class c; try { c = Class.forName(logAdapterClassName, true, currentCL); } catch (ClassNotFoundException originalClassNotFoundException) { // The current classloader was unable to find the log adapter // in this or any ancestor classloader. There's no point in // trying higher up in the hierarchy in this case.. - String msg = "" + originalClassNotFoundException.getMessage(); + String msg = originalClassNotFoundException.getMessage(); logDiagnostic( "The log adapter '" + logAdapterClassName @@ -1100,7 +1083,7 @@ public class LogFactoryImpl extends LogFactory { c = Class.forName(logAdapterClassName); } catch (ClassNotFoundException secondaryClassNotFoundException) { // no point continuing: this adapter isn't available - msg = "" + secondaryClassNotFoundException.getMessage(); + msg = secondaryClassNotFoundException.getMessage(); logDiagnostic( "The log adapter '" + logAdapterClassName @@ -1140,7 +1123,7 @@ public class LogFactoryImpl extends LogFactory { // the underlying logger library is not present in this or any // ancestor classloader. There's no point in trying higher up // in the hierarchy in this case.. - String msg = "" + e.getMessage(); + String msg = e.getMessage(); logDiagnostic( "The log adapter '" + logAdapterClassName @@ -1156,7 +1139,7 @@ public class LogFactoryImpl extends LogFactory { // // We treat this as meaning the adapter's underlying logging // library could not be found. - String msg = "" + e.getMessage(); + String msg = e.getMessage(); logDiagnostic( "The log adapter '" + logAdapterClassName @@ -1236,7 +1219,7 @@ public class LogFactoryImpl extends LogFactory { private ClassLoader getBaseClassLoader() throws LogConfigurationException { ClassLoader thisClassLoader = getClassLoader(LogFactoryImpl.class); - if (useTCCL == false) { + if (!useTCCL) { return thisClassLoader; } @@ -1354,7 +1337,7 @@ public class LogFactoryImpl extends LogFactory { * @throws LogConfigurationException ALWAYS */ private void handleFlawedDiscovery(String logAdapterClassName, - ClassLoader classLoader, + ClassLoader classLoader, // USED? Throwable discoveryFlaw) { if (isDiagnosticsEnabled()) { diff --git a/src/java/org/apache/commons/logging/impl/ServletContextCleaner.java b/src/java/org/apache/commons/logging/impl/ServletContextCleaner.java index 6061171..b966077 100644 --- a/src/java/org/apache/commons/logging/impl/ServletContextCleaner.java +++ b/src/java/org/apache/commons/logging/impl/ServletContextCleaner.java @@ -51,7 +51,7 @@ import org.apache.commons.logging.LogFactory; public class ServletContextCleaner implements ServletContextListener { - private Class[] RELEASE_SIGNATURE = {ClassLoader.class}; + private static final Class[] RELEASE_SIGNATURE = {ClassLoader.class}; /** * Invoked when a webapp is undeployed, this tells the LogFactory diff --git a/src/java/org/apache/commons/logging/impl/WeakHashtable.java b/src/java/org/apache/commons/logging/impl/WeakHashtable.java index 3a21979..f0b9d3f 100644 --- a/src/java/org/apache/commons/logging/impl/WeakHashtable.java +++ b/src/java/org/apache/commons/logging/impl/WeakHashtable.java @@ -129,7 +129,7 @@ public final class WeakHashtable extends Hashtable { private static final int PARTIAL_PURGE_COUNT = 10; /* ReferenceQueue we check for gc'd keys */ - private ReferenceQueue queue = new ReferenceQueue(); + private final ReferenceQueue queue = new ReferenceQueue(); /* Counter used to control how often we purge gc'd entries */ private int changeCount = 0; @@ -444,9 +444,7 @@ public final class WeakHashtable extends Hashtable { // objects could test equal but have different hashcodes. // We can reduce (not eliminate) the chance of this // happening by comparing hashcodes. - if (result == true) { - result = (this.hashCode() == otherKey.hashCode()); - } + result = result && (this.hashCode() == otherKey.hashCode()); // In any case, as our c'tor does not allow null referants // and Hashtable does not do equality checks between // existing keys, normal hashtable operations should never