Atif Khan’s Blog

A lot of Java applications run on Sun’s JDK due to various reasons. In fact till recently Sun had a very restrictive license policy for Java. So, effectively Sun’s JDK has been a de facto standard. That’s the reason Sun’s JDK code is supposed and expected to be of high quality and following at least the coding conventions recommended by Sun in a 1999 article that is widely used.

If you look under the hood of JDK, you will find a completely different picture. Here is an example from the Java 6 source code of StringBuilder class. (I have noticed similar stuff in 1.4 and 1.5 releases.)

• Indentation - not sure if it was edited in notepad (even vi does indentation) or a result of source control merge?.
• If clause doesn’t use brackets- Here is an excerpt from Sun’s Java coding conventions. The if-else class of statements should have the following form: if (condition) { statements; } if (condition) { statements; } else { statements; } if (condition) { statements; } else if (condition) { statements; } else { statements; } Note: if statements always use braces {}. Avoid the following error-prone form: if (condition) //AVOID! THIS OMITS THE BRACES {}! statement;

  Need I say more?

• Creating “null” string instance- Why can’t it be represented by a static final constant and reused? All the string literals are stored in JVM in a String pool, even the ones defined as constants. Isn’t it more optimized if you have a direct reference to it rather than JVM doing a lookup in the pool every time you call the method?
  Note: After looking at the bytecode generated by the compiler, it looks like a direct reference is pushed on the stack for the string literal. So, it effectively doesn’t matter if you use a literal or a constant. Still, if you are using a string literal over and over again, it may be a better idea to create a constant. This way if you need to change the string literal, there is only one place where you need to make change. So, you protect yourself against future changes. Thanks to Messi for pointing this out in the comments :).
• Variables can be made final-The method parameter sb and local variables len and newcount can all be declared final for optimization. This article has good explanation for it.
  Note: Again after analyzing the bytecode it seems like compiler really doesn’t care about the final declaration for the local variables. The method parameter are a different case as by declaring them final you make sure that they are not modified by mistake. Also, the local variables can be declared as final as a coding practice to mark them as immutable even though it doesn’t affect the byte code. But it makes it clear for a person looking at your code that the variable can not be modified. Thanks to Messi and Kevin Riff for pointing this out in the comments

This is just an example. There are instances like this all over the JDK code base. I want to make it clear that I am not commenting on the performance aspect of JDK, just the code conventions and formatting (for the obvious reasons that the code should be properly formatted). Now, since Sun has released the JDK source code under GPL license, do we expect to see things like this continue? Do you think people will take more responsibility and clean up the JDK code? I hope so. I also hope that Sun does not blacklist me after this post

One of the biggest feature added with JDK 1.5 is enum support. At last you can use a language construct to define constants in the system rather than using the static final to define the constants. The obvious benefits of enums are type safety and type checking.

Also enums let you define meaningful values for the constants. e.g.

As you can see in the above example, you can use the enum constant PI across your code, but if you wanted to get the value of PI, you can do that by using PI.getValue().

This is where things get a little fuzzy. What if you have a double value representing either PI or LOG_E and you want to find out which constant in MathEnum represents that value? This is the reverse lookup of enum. The Java implementation of enum does not allow reverse lookup by value. It is a very useful feature if you are working with an existing code base and an external system returns values for the constants that need to be translated into Java Enums.

Here is how this problem can be solved by using the Reversible Enum Pattern. First we need to create a ReversibleEnum interface that will mark the Enums as reversible and define the required method.

Next, we need to define a Map that will be used to store the enum constant values and for reverse lookup.

You will notice that in both the ReversibleEnum interface and the ReverseEnumMap class the Generics template is used. This is done to customize these based on the type of the enum that will use them and the type of the value of enum constants. If this sounds confusing, it will become more clear after we see the modified version of the MathEnum class from earlier.

The changes you will notice here are:

• MathEnum implements the ReversibleEnum interface.
• New variable for ReversibleEnumMap
• New method reverse(final Double value) to do the reverse lookup. This is from the ReversibleEnum interface.
• New static method getInstance() that just returns the first constant of the enum. This is kind of a hack to get a reference to an instant of the enum, so that the reverse(..) method could be called on it.

Now to do a reverse lookup on the MathEnum, all you need to do is:

Lets recap all the steps needed to be done to create a reversible enum:

1. Define an interface ReversibleEnum. — One time job.
2. Define a new class ReverseEnumMap. — One time job.
3. Implement the ReversibleEnum interface and define a member variable of type ReverseEnumMap in the enum that you want to be reversible. If you do not need the reverse functionality, you don’t need to do any of this.

So, effectively once you have the interface and map class created, it’s minimal effort to create a reversible enum. I wish this was a built-in functionality in the JDK enum implementation.

The applications usually need to cache information in memory. The most often used classes to do this in Java are HashMap and Hashtable. If you need to do any sophisticated caching, then you can use JBoss Cache, OSCache or EHCache. Even if you use an external caching system, you may still want to cache some information locally within an object just to have fast access. The problem with this approach is that, if you are not careful and do not control the size of this in-memory cache, then it may grow too big and affect the performance of your application.

A very simple solution to this problem is to set a maximum size for your in-memory cache and most preferably make it LRU (Least Recently Used). This way you will have a predictable memory utilization and only the items used recently will be kept in the cache.

Starting with JDK 1.4, a new (and very rarely used) collection class was added called LinkedHashMap. There are couple of benefits of using a LinkedHashMap:

• It is possible to preserve the order of items in the map. So, the order of iteration through the items is same as the order of insertion. A special constructor is provided for this purpose. This is very useful when you already have a sorted collection of data and you want to do some processing on it and return it as a Map. Using a TreeMap (the only other map that allows iteration in a given order) is too expensive for this scenario.
• It exposes a method removeEldestEntry(Map.Entry) that may be overridden to impose a policy for removing stale mappings automatically when new mappings are added to the map. This is what we are going to use to create a LRU cache.

Check out the following snippet for an example of simple LRU cache.

This code creates a simple LRU cache with maximum 50 entries. The magic happens during the creation of the LinkedHashMap where a boolean flag for access order is set to true and the method removeEldestEntry is overridden. Now, if you run the program, you can see that the cache size is fixed to 50 entries and the least recently used entries are removed from the map. It is possible to create a pruning thread to prune the map so you don’t always necessarily maintain the maximum number of entries.

PMD is an open source static analysis tool for the Java source code. It has a pretty good set of standard rules and you can add new rules by using XPath or Java code against the AST (Abstract Syntax Tree) it generates from the source code. The code analysis tools help you to keep your code clean and simple and avoid common pitfalls.

I recently found a bug while trying to analyze the code in the JDK 1.5 mode using PMD 3.8 version. The PMD seems to be ignoring all the violations for the class/method if you have a @SuppressWarnings annotation in it. According to the PMD documentation, only if you have @SuppressWarnings("") in your code, it’s supposed to ignore the violations for that class/method. But, the source code for PMD does not implement this logic. It just checks for the presence of @SuppressWarnings annotation and not any values passed to it. The end result is that all the violations are ignored. This can be very annoying as you may miss a significant number of violations.

To fix this issue, I made a couple of changes to the PMD source code. These are listed below with explanations:

• net.sourceforge.pmd.ast.ASTAnnotation - This is the class where the check for @SuppressWarnings is implemented. I made changes to the suppresses(Rule rule) method of this class to check if any values are passed to this annotation. If any values except "" are passed (e.g. @SuppressWarnings("unchecked")), then all the PMD violations are written to the report. Otherwise if "" is passed, all the PMD violations are suppressed.

  public boolean suppresses(Rule rule) { /*Check for "suppress all warnings" case @SuppressWarnings("") TypeDeclaration Annotation NormalAnnotation Name:SuppressWarnings */ if (!(jjtGetChild(0) instanceof ASTSingleMemberAnnotation) && !(jjtGetChild(0) instanceof ASTNormalAnnotation)) { return false; } final SimpleJavaNode n = (SimpleJavaNode) jjtGetChild(0); if(!(n.jjtGetChild(0) instanceof ASTName) || !((ASTName) n.jjtGetChild(0)).getImage().equals("SuppressWarnings")) { return false; } final List suppressionQualifierNodeList = n.findChildrenOfType( ASTLiteral.class ); final Iterator suppressionQualifierNodeIt = suppressionQualifierNodeList.iterator(); while(suppressionQualifierNodeIt.hasNext()) { final ASTLiteral literalNode = (ASTLiteral)suppressionQualifierNodeIt.next(); if(literalNode == null) { continue; } final String literalImage = literalNode.getImage(); if(literalImage != null && literalImage.trim().equals( "\"\"" )) { return true; } } return false; }

• net.sourceforge.pmd.renderers.XMLRenderer - You need to change this only if you generate your PMD reports in the XML format. The changes I made here are to improve the information presented for suppressed violations tag (<suppressedviolation>). So, if you decide to use the @SuppressWarnings("") annotation, at the end of reports their will be a section for suppressed violations. Another change I made is to enclose all the <suppressedviolation> tags inside a <file> tag for each file just like regular violations.

  public String render( Report report ) { StringBuffer buf = new StringBuffer( "< ?xml version=\"1.0\" encoding=\"UTF-8\"?>" + PMD.EOL + createVersionAttr() + createTimestampAttr() + createTimeElapsedAttr( report ) + ">" + PMD.EOL ); String filename = null; // rule violations for( Iterator i = report.iterator(); i.hasNext(); ) { IRuleViolation rv = (IRuleViolation)i.next(); if ( !rv.getFilename().equals( filename ) ) { // New File if ( filename != null ) {// Not first file ? buf.append( "" + PMD.EOL ); } filename = rv.getFilename(); buf.append( "<file name=\"" ); StringUtil.appendXmlEscaped( buf, filename ); buf.append( "\">" ).append( PMD.EOL ); } buf.append( "<violation line=\"" ).append( rv.getBeginLine() ).append( "\"" ); buf.append( " rule=\"" ); StringUtil.appendXmlEscaped( buf, rv.getRule().getName() ); buf.append( "\"" ); buf.append( " ruleset=\"" ); StringUtil.appendXmlEscaped( buf, rv.getRule().getRuleSetName() ); buf.append( "\"" ); maybeAdd( "package", rv.getPackageName(), buf ); maybeAdd( "class", rv.getClassName(), buf ); maybeAdd( "method", rv.getMethodName(), buf ); maybeAdd( "externalInfoUrl", rv.getRule().getExternalInfoUrl(), buf ); buf.append( " priority=\"" ); buf.append( rv.getRule().getPriority() ); buf.append( "\">" ); buf.append( PMD.EOL ); StringUtil.appendXmlEscaped( buf, rv.getDescription() ); buf.append( PMD.EOL ); buf.append( "</violation>" ); buf.append( PMD.EOL ); } if ( filename != null ) { // Not first file ? buf.append( "</file>" + PMD.EOL ); } // errors for( Iterator i = report.errors(); i.hasNext(); ) { Report.ProcessingError pe = (Report.ProcessingError)i.next(); buf.append( "<error " ).append( "filename=\"" ); StringUtil.appendXmlEscaped( buf, pe.getFile() ); buf.append( "\" msg=\"" ); StringUtil.appendXmlEscaped( buf, pe.getMsg() ); buf.append( "\"/>" ).append( PMD.EOL ); } // suppressed violations if ( showSuppressedViolations ) { int count = 0; for( Iterator i = report.getSuppressedRuleViolations().iterator(); i.hasNext(); ) { Report.SuppressedViolation suppressed = (Report.SuppressedViolation)i.next(); if ( !suppressed.getRuleViolation().getFilename().equals( filename ) || count == 0 ) { // New // File if ( filename != null && count > 0 ) {// Not first file ? buf.append( "" + PMD.EOL ); } filename = suppressed.getRuleViolation().getFilename(); buf.append( "<file name=\"" ); StringUtil.appendXmlEscaped( buf, filename ); buf.append( "\">" ).append( PMD.EOL ); } buf.append( "<suppressedviolation line=\"" ).append( suppressed.getRuleViolation().getBeginLine() ).append( "\"" ); buf.append( " rule=\"" ); StringUtil.appendXmlEscaped( buf, suppressed.getRuleViolation() .getRule() .getName() ); buf.append( "\"" ); buf.append( " ruleset=\"" ); StringUtil.appendXmlEscaped( buf, suppressed.getRuleViolation() .getRule() .getRuleSetName() ); buf.append( "\" suppressiontype=\"" ); StringUtil.appendXmlEscaped( buf, suppressed.suppressedByNOPMD() ? "nopmd" : "annotation" ); buf.append("\""); maybeAdd( "package", suppressed.getRuleViolation().getPackageName(), buf ); maybeAdd( "class", suppressed.getRuleViolation().getClassName(), buf ); maybeAdd( "method", suppressed.getRuleViolation().getMethodName(), buf ); maybeAdd( "externalInfoUrl", suppressed.getRuleViolation() .getRule() .getExternalInfoUrl(), buf ); buf.append( " priority=\"" ); buf.append( suppressed.getRuleViolation().getRule().getPriority() ); buf.append( "\" usermsg=\"" ); StringUtil.appendXmlEscaped( buf, suppressed.getUserMessage() ); buf.append( "\">" ); buf.append( PMD.EOL ); StringUtil.appendXmlEscaped( buf, suppressed.getRuleViolation() .getDescription() ); buf.append( PMD.EOL ); buf.append( "</suppressedviolation>" ); buf.append( PMD.EOL ); count++; } final String fileEndStr = "</file>" + PMD.EOL; if(!buf.substring( buf.length() - fileEndStr.length() ).equalsIgnoreCase( fileEndStr)) { buf.append( fileEndStr ); } } buf.append( "" ); return buf.toString(); }

• net.sourceforge.pmd.util.StringUtil - This was throwing NullPointerException for the suppressed violations while generating the report. I just added a null check here.

  private static void appendXmlEscaped(StringBuffer buf, String src, boolean supportUTF8) { char c; if(src == null) { return; } ....... .......

You can just replace the whole methods for ASTAnnotation and XMLRenderer classes and add the code snippet for StringUtil class. Once you do that you are ready to do the build. Run following in the bin directory of pmd-3.8 or the directory where you extracted the source code:

ant dist

That should create the pmd-3.8.jar in the lib directory. Now, you can use this jar whether you use the command line, ant or eclipse plugin to run PMD. If you have any questions, please leave me a comment.