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

@@ -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$
 

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 new ClassCastException(msg);
-                        throw ex;
                     }
                     
                     // Ignore exception, continue. Presumably the classloader was the

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 = null != Class.forName("org.apache.log4j.Logger");
-                log4jIsAvailable = true;
-            } else {
-                log4jIsAvailable = false;
-            }
         } catch (Throwable t) {
             log4jIsAvailable = false;
         }
 
         // Is JDK 1.4 Logging Available?
         try {
-            if ((null != Class.forName("java.util.logging.Logger")) &&
+            jdk14IsAvailable = (null != Class.forName("java.util.logging.Logger")) &&
-                (null != Class.forName("org.apache.commons.logging.impl.Jdk14Logger"))) {
+                    (null != Class.forName("org.apache.commons.logging.impl.Jdk14Logger"));
-                jdk14IsAvailable = true;
-            } else {
-                jdk14IsAvailable = false;
-            }
         } 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];
+            Object[] args = { name };
-            args[0] = name;
             log = (Log) (logImplctor.newInstance(args));
         } catch (Throwable t) {
             log = null;

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;
     }
 
     

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() {
-
+        return (String[]) attributes.keySet().toArray(new String[attributes.size()]);
-        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);
-
     }
 
 
@@ -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 == null ? e : c);
-                throw new LogConfigurationException(c);
-            } else {
-                throw new LogConfigurationException(e);
-            }
         } 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()) {

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

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 = result && (this.hashCode() == otherKey.hashCode());
-                        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