Design RulesThe Design Ruleset contains a collection of rules that find questionable designs. UseSingletonRuleIf you have a class that has nothing but static methods, consider making it a Singleton Here's an example of code that would trigger this rule: public class MaybeASingleton { public static void foo() { // etc } public static void bar() { // etc } } LooseCouplingRuleAvoid using implementation types (i.e., HashSet); use the interface (i.e, Set) instead Here's an example of code that would trigger this rule: import java.util.*; public class Bar { // should be "private List list" private ArrayList list = new ArrayList(); // should be "public Set getFoo()" public HashSet getFoo() { return new HashSet(); } } SimplifyBooleanReturnsRuleAvoid unnecessary if..then..else statements when returning a boolean Here's an example of code that would trigger this rule: public class Foo { private int bar =2; public boolean isBarEqualsTo(int x) { // this bit of code if (bar == x) { return true; } else { return false; } // can be replaced with a simple // return bar == x; } } SimplifyBooleanExpressionsAvoid unnecessary comparisons in boolean expressions - this makes simple code seem complicated. This rule is defined by the following XPath expression: //Expression/ConditionalAndExpression/InstanceOfExpression[position()>1] /UnaryExpression/PrimaryExpression/PrimaryPrefix/Literal/BooleanLiteral Here's an example of code that would trigger this rule: public class Bar { // can be simplified to // bar = isFoo(); private boolean bar = (isFoo() == true); public isFoo() { return false;} } SwitchStmtsShouldHaveDefaultSwitch statements should have a default label. This rule is defined by the following XPath expression: //SwitchStatement[not(SwitchLabel[count(*) = 0])] Here's an example of code that would trigger this rule: public class Foo { public void bar() { int x = 2; switch (x) { case 2: int j = 8; } } } AvoidDeeplyNestedIfStmtsDeeply nested if..then statements are hard to read. Here's an example of code that would trigger this rule: public class Foo { public void bar() { int x=2; int y=3; int z=4; if (x>y) { if (y>z) { if (z==x) { // this is officially out of control now } } } } } AvoidReassigningParametersRuleReassigning values to parameters is a questionable practice. Use a temporary local variable instead. Here's an example of code that would trigger this rule: public class Foo { private void foo(String bar) { bar = "something else"; } } SwitchDensityA high ratio of statements to labels in a switch statement implies that the switch statement is doing too much work. Consider moving the statements either into new methods, or creating subclasses based on the switch variable. Here's an example of code that would trigger this rule: public class Foo { private int x; public void bar() { switch (x) { case 1: { System.out.println("I am a fish."); System.out.println("I am a fish."); System.out.println("I am a fish."); System.out.println("I am a fish."); break; } case 2: { System.out.println("I am a cow."); System.out.println("I am a cow."); System.out.println("I am a cow."); System.out.println("I am a cow."); break; } } } } ConstructorCallsOverridableMethodRuleCalling overridable methods during construction poses a risk of invoking methods on an incompletely constructed object. This situation can be difficult to discern. It may leave the sub-class unable to construct its superclass or forced to replicate the construction process completely within itself, losing the ability to call super(). If the default constructor contains a call to an overridable method, the subclass may be completely uninstantiable. Here's an example of code that would trigger this rule: 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(); } } AccessorClassGenerationRuleInstantiation by way of private constructors from outside of the constructor's class often causes the generation of an accessor. A factory method, or non-privitization of the constructor can eliminate this situation. The generated class file is actually an interface. It gives the accessing class the ability to invoke a new hidden package scope constructor that takes the interface as a supplementary parameter. This turns a private constructor effectively into one with package scope, though not visible to the naked eye. Here's an example of code that would trigger this rule: public class OuterClass { void method(){ InnerClass ic = new InnerClass();//Causes generation of accessor } public class InnerClass { private InnerClass(){ } } } public class OuterClass { void method(){ InnerClass ic = new InnerClass();//OK, due to public constructor } public class InnerClass { public InnerClass(){ } } } public class OuterClass { void method(){ InnerClass ic = InnerClass.getInnerClass();//OK } public static class InnerClass { private InnerClass(){ } private static InnerClass getInnerClass(){ return new InnerClass(); } } } public class OuterClass { private OuterClass(){ } public class InnerClass { void method(){ OuterClass oc = new OuterClass();//Causes generation of accessor } } } FinalFieldCouldBeStaticIf a final field is assigned to a compile-time constant, it could be made static, thus saving overhead in each object This rule is defined by the following XPath expression: //FieldDeclaration [not (ancestor::InterfaceDeclaration)] [@Final='true' and @Static='false'] /VariableDeclarator/VariableInitializer/Expression /ConditionalAndExpression/InstanceOfExpression /UnaryExpression/PrimaryExpression/PrimaryPrefix/Literal Here's an example of code that would trigger this rule: public class Foo { public final int BAR = 42; // this could be static and save some space } BooleanInstantiationAvoid instantiating Boolean objects, instead use Boolean.valueOf(). This rule is defined by the following XPath expression: //AllocationExpression[not (ArrayDimsAndInits) and (Name/@Image='Boolean' or Name/@Image='java.lang.Boolean')] Here's an example of code that would trigger this rule: public class Foo { private Boolean bar = new Boolean("true"); // just do a Boolean bar = Boolean.TRUE or Boolean.valueOf(true); } CloseConnectionRuleEnsures that Connection objects are always closed after use Here's an example of code that would trigger this rule: 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(); } } ProperCloneImplementationRuleObject clone() should be implemented with super.clone() This rule is defined by the following XPath expression: //ClassDeclaration//MethodDeclarator [@Image = 'clone'] [count(FormalParameters/*) = 0] [count(../Block//*[ (self::AllocationExpression) and (./Name/@Image = ancestor:: UnmodifiedClassDeclaration[position()=last()]/@Image) ])> 0 ] Here's an example of code that would trigger this rule: class Foo{ public Object clone(){ return new Foo(); // This is bad } } NonStaticInitializerA nonstatic initializer block will be called any time a constructor is invoked (just prior to invoking the constructor). While this is a valid language construct, it is rarely used and is confusing. This rule is defined by the following XPath expression: //ClassBodyDeclaration/Initializer[not(@Static='true')] Here's an example of code that would trigger this rule: public class MyClass { // this block gets run before any call to a constructor { System.out.println("I am about to construct myself"); } } DefaultLabelNotLastInSwitchStmtThe default label in a switch statement should be the last label, by convention. Most programmers will expect the default label (if present) to be the last one. This rule is defined by the following XPath expression: //SwitchStatement [not(SwitchLabel[position() = last()][count(*) = 0])] [SwitchLabel[count(*) = 0] Here's an example of code that would trigger this rule: switch (a) { case 1: // do something break; default: // the default case should be last, by convention break; case 2: break; } NonCaseLabelInSwitchStatementA non-case label (e.g. a named break/continue label) was present in a switch statement. This legal, but confusing. It is easy to mix up the case labels and the non-case labels. This rule is defined by the following XPath expression: //SwitchStatement//BlockStatement/Statement/LabeledStatement Here's an example of code that would trigger this rule: public class Foo { void bar(int a) { switch (a) { case 1: // do something break; mylabel: // this is legal, but confusing! break; default: break; } } } |