An Immodest Proposal


The IDEA inspection panel is an amazing tool for finding and fixing implementation errors early in the coding phase, before the code has even been run. I've found bugs using it that could literally have taken me hours to find otherwise, or indeed might not have gone unnoticed until production. It's also great for enforcing team coding standards, and as a prod to get developers to improve the preciseness and conformance their own code. Like all the rest of the IDEA product, you guys have done a great job.

However.

A code auditing tool is only as good as the number of code weaknesses and bug-prone constructs it finds. The inspection panel finds quite a few, including some very difficult ones, but there are literally scores of known dubious and buggy constructs which it can't find. No new inspection has been added since two months before the Ariadna release, and there is no indication that this is likely to change. I understand that you are continuously working on improving IDEA, and are currently working on many large, difficult and high-value-added features(AspectJ, Generics, GUI Builder). I appreciate that, but want more.

The obvious solution, of course, is plug-ins.

About a month ago, in response to a posting about a possible new intention, I provided a long list of intentions I would commit to developing if the intention API were opened up. When Aurora build 816 released with an open intention API, I developed Intention Power-Pack, which to date covers all but one of my promised intentions. The response has been very favorable, even earning me intention feature requests from JetBrains employees.

Attached is a list of code inspections that I have redacted from various commercial and open-source static code analysis tools. If IntelliJ/JetBrains will open the inspection API as they have the intention API, I will commit to implementing all of the listed code checks within two months.

Let's go kill some bugs.

(Note: I am not advocating any of these checks as gospel, or saying that code that doesn't follow these rules is of low quality. Indeed, there are many that I wouldn't use myself, and some are actually contradictory. All that presence in this list indicates is that some people have advocated this rule as a way of achieving higher quality code, and that I feel capable of implementing a check
for violations if JetBrains opens up the inspection API. Rules marked with an asterisk (*) are slated to have automatic repair links, like many of the core IDEA inspections do.)

Naming conventions
Package names should follow specified regex (including minimum and maximum lengths).
Class names should follow specified regex (including minimum and maximum lengths).
Interface names should follow specified regex (including minimum and maximum lengths).
Exception names should follow specified regex (including minimum and maximum lengths).
Instance method names should follow specified regex (including minimum and maximum lengths).
Static method names should follow specified regex (including minimum and maximum lengths).
Instance variable names should follow specified regex (including minimum and maximum lengths).
Static variable names should follow specified regex (including minimum and maximum lengths).
Constant names should follow specified regex (including minimum and maximum lengths).
Method names should not differ only by capitalization.
Methods should be named differently from the class they are declared in
Methods should be named differently from the parent of the class they are declared in
Class names should not be prefixed with the name of the package that contains them
Only property accessor methods should have names begining with 'get'.
Only boolean property accessor methods should have names begining with 'is'.
Only property mutator methods should have names begining with 'set'.

Possible coding errors
Switch statements should not contain text labels.
Switch statement should not use "fallthru".
For statement should not have an empty body.
While statement should not have an empty body.
If statement should not have an empty body.
Assignment expression should not be the condition of an if statement.
Floating point types should not be compared using == or !=.
equals() should be defined to take Object as an argument.
compareTo() should be defined to take Object as an argument.
Class initializations should not contain circular dependencies.
Method should not be named hashcode(), instead of hashCode().
Method should not be named compareto(), instead of compareTo().
Return value of java.io.InputStream.read() methods should not be ignored.
java.lang.Object should be compared using .equals(), rather than ==. *
java.lang.String should be compared using .equals(), rather than ==. *
For statement should be not be used if the body will only be executed at most once.
While statement should be not be used if the body will only be executed at most once.
Do-While statement should be not be used if the body will only be executed once.
Fields should not be read in constructors before they are initialized.
Non-final, non-static, non-private methods should not be called from constructors.
Use "literal".equals(var) rather than var.equals("literal").
Use getClass() in every equals() method.
Use instanceof in every equals() method.

Constructs that mask possible bugs
Switch statement should have "default" branches.
Catch block parameters should be as specific as possible.*
Catch blocks should not be empty.
Throws clauses should be as specific as possible.*
RuntimeException and it's subclasses should not be used in a catch clause.
Throwable should not be used in a catch clause.
User declared exceptions should be subclasses of Exception, not RuntimeException or Throwable.
User declared exceptions should be subclasses of RuntimeException, not Exception or Throwable.
Do not return from inside a finally block.
Do not throw exceptions from inside a finally block.
Avoid assignment of a local variable to 'null', outside of declarations.
Avoid throwing an object of class 'Exception', 'RuntimeException' or 'Throwable', use a subclass instead.

OO Best practices
All non-constant fields should be private.
All non-constant field should be private or protected.
Instanceof expressions should only refer to interfaces, not classes.
Static member should not have the same name as member of parent class.
Private member should not have the same name as member of parent class.
Method parameter should not have the same name as a member variable.
Local variable should not have the same name as a member variable.
Declaration can be weakened to use superclass.
Declaration can be weakened to use interface.
Do not call abstract methods in constructors of abstract classes.
Explicitly initialize all instance variables in each constructor (or on object initialization).
Explicitly initialize all static variables on class initialization.

Potentially confusing or bug-prone constructs
Numeric literals should be explicitly declared (no 'magic numbers').
Method parameters should not be assigned to.
For loop control variables should not be assigned to.
Results of assignment expression should not be used.
Results of increment or decrement expression should not be used.
Labelled statements should not be used.
Continue statements should not be used.
Break statements should not be other than to end switch statement branches.
The ternary condition operator should not be used.
The ternary condition operator should not be nested.
Switch statements should not be nested.
Array variables should be declared with the [] adjacent to the type, not adjacent to the variable name. *
Only one type of variable should be declared in any given declaration. *
Only one variable should be declared in any given declaration. *
No more than three negations ( or !=) should be used in a method.
Static members should not be accessed via an object instance. *
Methods should have only one return point.
Octal integer literals should not be used in array literal with non-octal integer literals.
Long literals should end in 'L', not 'l'. *
Non-final "clone()" methods should be declared to throw "CloneNotSupported".*

Style Violations
For statement should have a block statement as it's body. *
While statement should have a block statement as it's body. *
If statement should have a block statement for each branch. *
Constants should be placed on the left hand side of ==. *
Constants should be placed on the right hand side of ==. *
Modifiers should be used in canonical order (public,protected,private,static,abstract,final,transient,volatile,native,strictfp).*
Classes with only static final fields and abstract methods should be declared as interfaces. *

Unnecessary or overly verbose code
Equality comparison to boolean literals should be simplified. *
Declaring interface methods as 'public' or 'abstract' is redundant. *
Declaring interface fields as 'public', 'static' or 'final' is redundant. *
If statements returning or assigning boolean literal may be simplified. *
If statements returning or assigning a single value may be replaced by conditional operator. *

Threading
Double-checked locking should not be used.
Object.notifyall() should be used instead of Object.notify().
Thread.run() should probably be Thread.start().
Methods should not synchronize on non-final fields.
Object.wait() should only be called in a loop.

Finalization
finalize() should be declared protected. *
finalize() should call super.finalize().
finalize() should not be called explicitly.

Serialization
Serializable or Externalizable classes should implement readObject and writeObject.
Serializable or Externalizable classes should contain a serialVersionUID field.
Explicitly initialize every instance variable in readObject().
The serialVersionUID field should be static.

Internationalization
String literals should not be used in an internationalized context.
java.util.Date.toString() should not be used in an internationalized context.
java.sql.Time.toString() should not be used in an internationalized context.
String.compareTo() should not be used in an internationalized context.
String.equals() should not be used in an internationalized context.
String.equalsIgnoreCase() should not be used in an internationalized context.
String concatenation should not be used in an internationalized context.
Numeric .toString() should not be used in an internationalized context.
StringTokenizer should not be used in an internationalized context.
Character literals should not be used in comparisons in an internationalized context.

Portability
Calls to Runtime.exec() are non-portable.
Calls to System.getenv() are non-portable.
Calls to System.exit() are non-portable.
Native methods are non-portable.
Line separators should not be hardcoded.
File separators should not be hardcoded.

JUnit
JUnit test cases should not include a constructor.
JUnit test cases should implement setUp(), not setup().
JUnit test cases should implement tearDown(), not teardown().
All JUnit assertions should include a message.
JUnit suite() methods should be static.

Code Maturity
Calls to System.out.println() or System.err.println() should be replaced by other logging constructs.
Calls to Exception.printStackTrace() should be replaced by other logging constructs.

Performance
All Collections should be constructed with an initial capacity.
All StringBuffers should be constructed with an initial capacity.
The constructor String(String s) should not be used. *
The constructor Boolean(boolean b) should not be used. *
Array copying should be done via System.arraycopy, not looping. *
Calls to System.gc() should not be made in production code.
Do not create unnecessary temporaries when converting between basic types (e.g. new Integer(x).intValue())*
String concatenation in loops should probably be replace by explicit calls to StringBuffer.append().
Calling String.toString() is unnecessary. *
Non-constant Strings should be replaced by StringBuffers.
Multiplication by a power of 2 should be replaced by a right shift operation.
Division by a power of 2 should be replaced by a left shift operation.
Use single quotes instead of double quotes for single character string concatenation. *

31 comments
Comment actions Permalink

>===Empty catch clause===

Already on the list

>===Missing try{}catch{} blocks around program entry points. ===

Nice, although "program entry points" is going to be a tough concept to fully automate.

>==== Using Exception instead of the specific Exception===

Already on the list (as "Catch clauses should be as specific as possible).

0

Please sign in to leave a comment.