Design

public class MaybeAUtility {
  public static void foo() {}
  public static void bar() {}
}

    

public boolean isBarEqualTo(int x) {
   
	if (bar == x) {		 // this bit of code...
		return true;
	} else {
		return false;
    }
}

public boolean isBarEqualTo(int x) {

   	return bar == x;	// can be replaced with this
}

    

//EqualityExpression/PrimaryExpression
 /PrimaryPrefix/Literal/BooleanLiteral

              

  
public class Bar {
  // can be simplified to
  // bar = isFoo();
  private boolean bar = (isFoo() == true);

  public isFoo() { return false;}
}
  
      

                  
//SwitchStatement[not(SwitchLabel[@Default='true'])]
                  
              

public void bar() {
    int x = 2;
    switch (x) {
      case 1: int j = 6;
      case 2: int j = 8;
      				// missing default: here
    }
}

    

public class Foo {
  public void bar(int x, int y, int z) {
    if (x>y) {
      if (y>z) {
        if (z==x) {
         // !! too deep
        }
      }
    }
  }
}

    

public class Foo {
  private void foo(String bar) {
    bar = "something else";
  }
}

    

 
public class Foo {
  public void bar(int x) {
    switch (x) {
      case 1: {
        // lots of statements
        break;
      } case 2: {
        // lots of statements
        break;
      }
    }
  }
}
 
      

  
public class SeniorClass {
  public SeniorClass(){
      toString(); //may throw NullPointerException if overridden
  }
  public String toString(){
    return "IAmSeniorClass";
  }
}
public class JuniorClass extends SeniorClass {
  private String name;
  public JuniorClass(){
    super(); //Automatic call leads to NullPointerException
    name = "JuniorClass";
  }
  public String toString(){
    return name.toUpperCase();
  }
}
  
      

  
public class Outer {
 void method(){
  Inner ic = new Inner();//Causes generation of accessor class
 }
 public class Inner {
  private Inner(){}
 }
}
  
      

                    
//FieldDeclaration
 [@Final='true' and @Static='false']
 [not (../../../../ClassOrInterfaceDeclaration[@Interface='true'])]
   /VariableDeclarator/VariableInitializer/Expression
    /PrimaryExpression[not(PrimarySuffix)]/PrimaryPrefix/Literal
                    
                

  
public class Foo {
  public final int BAR = 42; // this could be static and save some space
}
  
      

public class Bar {
  public void foo() {
    Connection c = pool.getConnection();
    try {
      // do stuff
    } catch (SQLException ex) {
     // handle exception
    } finally {
      // oops, should close the connection using 'close'!
      // c.close();
    }
  }
}

    

//Initializer[@Static='false']

                 

   
public class MyClass {
 // this block gets run before any call to a constructor
  {
   System.out.println("I am about to construct myself");
  }
}
   
       

//SwitchStatement
 [not(SwitchLabel[position() = last()][@Default='true'])]
 [SwitchLabel[@Default='true']]

                 

   
public class Foo {
  void bar(int a) {
   switch (a) {
    case 1:  // do something
       break;
    default:  // the default case should be last, by convention
       break;
    case 2:
       break;
   }
  }
}   
       

 
//SwitchStatement//BlockStatement/Statement/LabeledStatement
 
                 

   
public class Foo {
  void bar(int a) {
   switch (a) {
     case 1:
       // do something
       break;
     mylabel: // this is legal, but confusing!
       break;
     default:
       break;
    }
  }
}
   
       

                  
//PrimaryExpression
[PrimaryPrefix/Name[ends-with(@Image, 'toArray')]]
[
PrimarySuffix/Arguments/ArgumentList/Expression
 /PrimaryExpression/PrimaryPrefix/AllocationExpression
 /ArrayDimsAndInits/Expression/PrimaryExpression/PrimaryPrefix/Literal[@Image='0']
]

                  
              

  
List foos = getFoos();
  
    // inefficient, the array will be discarded
Foo[] fooArray = foos.toArray(new Foo[0]);
    
    // much better; this one sizes the destination array, 
    // avoiding of a new one via reflection
Foo[] fooArray = foos.toArray(new Foo[foos.size()]);
  
      

                  
//EqualityExpression[@Image='==']
 /PrimaryExpression/PrimaryPrefix
 /Name[@Image='Double.NaN' or @Image='Float.NaN']
                  
              

  
boolean x = (y == Double.NaN);
  
      

    
//PrimaryExpression
  [
    PrimaryPrefix[Name[ends-with(@Image, 'equals')]]
      [following-sibling::node()/Arguments/ArgumentList[count(Expression)=1]
          /Expression/PrimaryExpression/PrimaryPrefix/Literal/NullLiteral]

    or

    PrimarySuffix[ends-with(@Image, 'equals')]
      [following-sibling::node()/Arguments/ArgumentList[count(Expression)=1]
          /Expression/PrimaryExpression/PrimaryPrefix/Literal/NullLiteral]

  ]
    
                

       
String x = "foo";

if (x.equals(null)) { // bad form
   	doSomething();
	}
	
if (x == null) { 	// preferred
   	doSomething();
	}
    
        

          
boolean bar(int x, int y) {
  return (x != y) ? diff : same;
 }
          
        

                
//PrimarySuffix
 [@Image='getClass']
 [parent::PrimaryExpression
  [PrimaryPrefix/AllocationExpression]
  [count(PrimarySuffix) = 2]
 ]
     
            

    
  // replace this
Class c = new String().getClass();

  // with this:
Class c = String.class;

    
        

      
public class Foo {
 public void bar() {
  int x = 2;
  x = x;
 }
}
      
      

//AllocationExpression
 [ClassOrInterfaceType[@Image='SimpleDateFormat']]
 [Arguments[@ArgumentCount=1]]

                    

        
public class Foo {
  // Should specify Locale.US (or whatever)
  private SimpleDateFormat sdf = new SimpleDateFormat("pattern");
}
        
        

  
public class Foo {
  private int x; // could be final
  public Foo() {
      x = 7;
  }
  public void foo() {
     int a = x + 2;
  }
}
  
      

                
//PrimaryExpression
[PrimaryPrefix/Name
 [ends-with(@Image, 'toLowerCase') or ends-with(@Image,
'toUpperCase')]
 ]
[PrimarySuffix[position() = 1]/Arguments[@ArgumentCount=0]]
     
            

    
class Foo {
 // BAD
 if (x.toLowerCase().equals("list"))...
 /*
 This will not match "LIST" when in Turkish locale
 The above could be
 if (x.toLowerCase(Locale.US).equals("list")) ...
 or simply
 if (x.equalsIgnoreCase("list")) ...
 */
 // GOOD
 String z = a.toLowerCase(Locale.EN);
}
    
        

//ClassOrInterfaceDeclaration[@Final='true']
/ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration
/FieldDeclaration[@Protected='true']
 
                 

public final class Bar {
  private int x;
  protected int y;  // bar cannot be subclassed, so is y really private or package visible?
  Bar() {}
}
 
         

   
public class StaticField {
   static int x;
   public FinalFields(int y) {
    x = y; // unsafe
   }
}
   
       

    
//ClassOrInterfaceDeclaration[@Nested='false']
[
  (
    count(./ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration/ConstructorDeclaration)>0
    and
    count(./ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration/ConstructorDeclaration) = count(./ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration/ConstructorDeclaration[@Private='true'])
  )
  and
  count(.//MethodDeclaration[@Static='true'])=0
  and
  count(.//FieldDeclaration[@Private='false'][@Static='true'])=0
  and
  count(.//ClassOrInterfaceDeclaration[@Nested='true']
           [@Public='true']
           [@Static='true']
           [count(./ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration/ConstructorDeclaration[@Public='true']) > 0]
           [count(./ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration/MethodDeclaration
                    [@Public='true']
                    [./ResultType/Type/ReferenceType/ClassOrInterfaceType
                        [@Image = //ClassOrInterfaceDeclaration[@Nested='false']/@Image]
                    ]
            ) > 0]
        ) = 0
]
    
              

// This class is unusable, since it cannot be
// instantiated (private constructor),
// and no static method can be called.

public class Foo {
  private Foo() {}
  void foo() {}
}


      

    
//MethodDeclaration[@Synchronized='true']
    
              

public class Foo {
  // Try to avoid this:
  synchronized void foo() {
  }
  // Prefer this:
  void bar() {
    synchronized(this) {
    }
  }

  // Try to avoid this for static methods:
  static synchronized void fooStatic() {
  }
  
  // Prefer this:
  static void barStatic() {
    synchronized(Foo.class) {
    }
  }
}

      

    
//SwitchStatement
[(count(.//BreakStatement)
 + count(BlockStatement//Statement/ReturnStatement)
 + count(BlockStatement//Statement/ThrowStatement)
 + count(BlockStatement//Statement/IfStatement[@Else='true' and Statement[2][ReturnStatement|ThrowStatement]]/Statement[1][ReturnStatement|ThrowStatement])
 + count(SwitchLabel[name(following-sibling::node()) = 'SwitchLabel'])
 + count(SwitchLabel[count(following-sibling::node()) = 0])
  < count (SwitchLabel))]
    
              

public void bar(int status) {
    switch(status) {
      case CANCELLED:
        doCancelled();
        // break; hm, should this be commented out?
      case NEW:
        doNew();
        // is this really a fall-through?
      case REMOVED:
        doRemoved();
        // what happens if you add another case after this one?
      case OTHER: // empty case - this is interpreted as an intentional fall-through
      case ERROR:
        doErrorHandling();
        break;
    }
}

      

    
//StatementExpression/PrimaryExpression
[count(PrimarySuffix/Arguments/ArgumentList) = 0]
[
PrimaryPrefix[./Name[@Image='notify' or ends-with(@Image,'.notify')]
or ../PrimarySuffix/@Image='notify'
or (./AllocationExpression and ../PrimarySuffix[@Image='notify'])
]
]
    
              

  void bar() {
    x.notify();
    // If many threads are monitoring x, only one (and you won't know which) will be notified.
    // use instead:
    x.notifyAll();
  }

      

    
//CatchStatement/FormalParameter
 /following-sibling::Block//InstanceOfExpression/PrimaryExpression/PrimaryPrefix
  /Name[
   @Image = ./ancestor::Block/preceding-sibling::FormalParameter
    /VariableDeclaratorId/@Image
  ]
    
              

try { // Avoid this
 // do something
} catch (Exception ee) {
 if (ee instanceof IOException) {
  cleanup();
 }
}
try {  // Prefer this:
 // do something
} catch (IOException ee) {
 cleanup();
}

      

//ClassOrInterfaceDeclaration
 [@Abstract='true'
  and count( .//MethodDeclaration[@Abstract='true'] )=0 ]
  [count(ImplementsList)=0]
  [count(.//ExtendsList)=0]
              
              

public abstract class Foo {
  void int method1() { ... }
  void int method2() { ... }
  // consider using abstract methods or removing
  // the abstract modifier and adding protected constructors
}

      

                      
//Expression
 [ConditionalOrExpression
 [EqualityExpression[@Image='==']
  //NullLiteral
  and
  UnaryExpressionNotPlusMinus
   [@Image='!']//InstanceOfExpression[PrimaryExpression
     //Name/@Image = ancestor::ConditionalOrExpression/EqualityExpression
      /PrimaryExpression/PrimaryPrefix/Name/@Image]
  and
  (count(UnaryExpressionNotPlusMinus) + 1 = count(*))
 ]
or
ConditionalAndExpression
 [EqualityExpression[@Image='!=']//NullLiteral
 and
InstanceOfExpression
 [PrimaryExpression[count(PrimarySuffix[@ArrayDereference='true'])=0]
  //Name[not(contains(@Image,'.'))]/@Image = ancestor::ConditionalAndExpression
   /EqualityExpression/PrimaryExpression/PrimaryPrefix/Name/@Image]
 and
(count(InstanceOfExpression) + 1 = count(*))
 ]
]
 
                  

      
class Foo {
  void bar(Object x) {
    if (x != null && x instanceof Bar) {
      // just drop the "x != null" check
    }
  }
}      
           

class Foo {
  boolean bar(String a, String b) {
    return a == b;
  }
}


  

              
//PrimaryExpression[
        PrimaryPrefix[Name
                [
	(ends-with(@Image, '.equals'))
                ]
        ]
        [
                   (../PrimarySuffix/Arguments/ArgumentList/Expression/PrimaryExpression/PrimaryPrefix/Literal[@StringLiteral='true'])
	and
	( count(../PrimarySuffix/Arguments/ArgumentList/Expression) = 1 )
        ]
]
[not(ancestor::Expression/ConditionalAndExpression//EqualityExpression[@Image='!=']//NullLiteral)]
[not(ancestor::Expression/ConditionalOrExpression//EqualityExpression[@Image='==']//NullLiteral)]

          
          

class Foo {
  boolean bar(String x) {
    return x.equals("2"); // should be "2".equals(x)
  }
}


  

              
//PrimaryExpression[
        PrimaryPrefix[Name
                [
    (ends-with(@Image, '.equalsIgnoreCase'))
                ]
        ]
        [
                   (../PrimarySuffix/Arguments/ArgumentList/Expression/PrimaryExpression/PrimaryPrefix/Literal)
    and
    ( count(../PrimarySuffix/Arguments/ArgumentList/Expression) = 1 )
        ]
]
[not(ancestor::Expression/ConditionalAndExpression//EqualityExpression[@Image='!=']//NullLiteral)]
[not(ancestor::Expression/ConditionalOrExpression//EqualityExpression[@Image='==']//NullLiteral)]

          
          

class Foo {
  boolean bar(String x) {
    return x.equalsIgnoreCase("2"); // should be "2".equalsIgnoreCase(x)
  }
}


  

  
public class Foo {
   public int foo() {
     int x = doSomething();
     return x;  // instead, just 'return doSomething();'
   }
}
  
      

private static Foo foo = null;

//multiple simultaneous callers may see partially initialized objects
public static Foo getFoo() {
    if (foo==null)
        foo = new Foo();
    return foo;
}
        

    
//MethodDeclaration/Block[count(BlockStatement) = 0 and @containsComment = 'false']
 
             

  
public void doSomething() {
}
 
      

    
//ConstructorDeclaration[@Private='false'][count(BlockStatement) = 0 and ($ignoreExplicitConstructorInvocation = 'true' or not(ExplicitConstructorInvocation)) and @containsComment = 'false']
 
             

  
public Foo() {
  // This constructor is intentionally empty. Nothing special is needed here.
}
 
      

    
//ClassOrInterfaceDeclaration[@Interface="true"]
    [
     count(.//MethodDeclaration)=0
     and
     count(.//FieldDeclaration)>0
    ]
    
        

    
public interface ConstantsInterface {
   public static final int CONSTANT1=0;
   public static final String CONSTANT2="1";
}
    
      

    
public class Foo {
    private static final SimpleDateFormat sdf = new SimpleDateFormat();
    void bar() {
        sdf.format(); // poor, no thread-safety
    }
    synchronized void foo() {
        sdf.format(); // preferred
    }
}
    
      

    
public class Foo {
    void good() {
        try{
            Integer.parseInt("a");
        } catch (Exception e) {
            throw new Exception(e); // first possibility to create exception chain
        }
        try {
            Integer.parseInt("a");
        } catch (Exception e) {
            throw (IllegalStateException)new IllegalStateException().initCause(e); // second possibility to create exception chain.
        }
    }
    void bad() {
        try{
            Integer.parseInt("a");
        } catch (Exception e) {
            throw new Exception(e.getMessage());
        }
    }
}
    
      

    
public class Foo {
	void good() {
       	List foo = getList();
		if (foo.isEmpty()) {
			// blah
		}
   	}

    void bad() {
   	    List foo = getList();
			if (foo.size() == 0) {
				// blah
			}
    	}
}
    
      

TypeDeclaration[count(../TypeDeclaration) = 1]/ClassOrInterfaceDeclaration
[@Final = 'false']
[count(./ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration/ConstructorDeclaration[@Private = 'true']) >= 1 ]
[count(./ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration/ConstructorDeclaration[(@Public = 'true') or (@Protected = 'true') or (@PackagePrivate = 'true')]) = 0 ]
[not(.//ClassOrInterfaceDeclaration)]
             

public class Foo {  //Should be final
    private Foo() { }
}
     

                
                    //ClassOrInterfaceDeclaration[@Abstract = 'true']
                        /ClassOrInterfaceBody
                        /ClassOrInterfaceBodyDeclaration
                        /MethodDeclaration[@Abstract = 'false' and @Native = 'false']
                        [
                            ( boolean(./Block[count(./BlockStatement) =  1]/BlockStatement/Statement/ReturnStatement/Expression/PrimaryExpression/PrimaryPrefix/Literal/NullLiteral) = 'true' )
                            or
                            ( boolean(./Block[count(./BlockStatement) =  1]/BlockStatement/Statement/ReturnStatement/Expression/PrimaryExpression/PrimaryPrefix/Literal[@Image = '0']) = 'true' )
                            or
                            ( boolean(./Block[count(./BlockStatement) =  1]/BlockStatement/Statement/ReturnStatement/Expression/PrimaryExpression/PrimaryPrefix/Literal[string-length(@Image) = 2]) = 'true' )
                            or
                            (./Block[count(./BlockStatement) =  1]/BlockStatement/Statement/EmptyStatement)
                            or
                            ( count (./Block/*) = 0 )
                        ]
                
             

        	
public abstract class ShouldBeAbstract {
    public Object couldBeAbstract() {
        // Should be abstract method ?
        return null;
    }

    public void couldBeAbstract() {
    }
}
	     	
    	

public class Foo {
    private int x;  // no reason to exist at the Foo instance level
    public void foo(int y) {
     x = y + 5;
     return x;
    }
}
   

                    
                        //MethodDeclaration
                        [
                        (./ResultType/Type[@Array='true'])
                        and
                        (./Block/BlockStatement/Statement/ReturnStatement/Expression/PrimaryExpression/PrimaryPrefix/Literal/NullLiteral)
                        ]
                    
                

public class Example {
    // Not a good idea...
    public int[] badBehavior() {
                   // ...
        return null;
    }

    // Good behavior
    public String[] bonnePratique() {
      //...
     return new String[0];
    }
}
            

                    
//ClassOrInterfaceDeclaration[
	(@Abstract = 'true')
	and
	(count(//MethodDeclaration) + count(//ConstructorDeclaration) = 0)
]
                    
                

            
public class abstract Example {
	String field;
	int otherField;
}
            
        

                    
//SwitchStatement[
    (count(.//SwitchLabel) < $minimumNumberCaseForASwitch)
]
                    
                

            
// With a minimumNumberCaseForASwitch of 3
public class Foo {
    public void bar() {
        switch (condition) {
            case ONE:
                instruction;
                break;
            default:
                break; // not enough for a 'switch' stmt, a simple 'if' stmt would have been more appropriate
        }
    }
}
            
        

          
//UnaryExpressionNotPlusMinus[@Image='!']/PrimaryExpression/PrimaryPrefix/Expression[EqualityExpression or RelationalExpression]
          
          

    
public boolean bar(int a, int b) {

	if (!(a == b)) // use !=
         return false;

	if (!(a < b)) // use >=
         return false;

	return true;
}
    
    

//FormalParameters/FormalParameter
    [position()=last()]
    [@Array='true']
    [@Varargs='false']
    [not (./Type/ReferenceType[@Array='true'][PrimitiveType[@Image='byte']])]
    [not (./Type/ReferenceType[ClassOrInterfaceType[@Image='Byte']])]
    [not (./Type/PrimitiveType[@Image='byte'])]
            

public class Foo {
   public void foo(String s, Object[] args) {
      // Do something here...
   }

   public void bar(String s, Object... args) {
      // Ahh, varargs tastes much better...
   }
}
        

      
public class HelloWorldBean {

  // Field declared before methods / inner classes - OK
  private String _thing;

  public String getMessage() {
    return "Hello World!";
  }

  // Field declared after methods / inner classes - avoid this
  private String _fieldInWrongLocation;
}
      
    

//ClassOrInterfaceDeclaration[@Final='true' and not(ExtendsList)]
/ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration
/MethodDeclaration[@Protected='true'][MethodDeclarator/@Image != 'finalize']
 
                 

public final class Foo {
  private int bar() {}
  protected int baz() {} // Foo cannot be subclassed, and doesn't extend anything, so is baz() really private or package visible? 
}