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. *
Please sign in to leave a comment.
...I provided a long list of intentions...
...I developed Intention Power-Pack...
Great !
Could you please post the list of intentions you developed, as well as some links to where we can find your plugin.
Many thanks.
Dan/
See http://www.intellij.org/twiki/bin/view/Main/IntentionPowerPack
Best regards,
Jorgen
Intention Power-Pack (version 0.3.1) is available at
http://www.intellij.org/twiki/bin/view/Main/IntentionPowerPack
Intentions provided (in no particular order):
Convert 'and' to 'or'
Convert 'or' to 'and'
Flip 'and'
Flip 'or
Flip 'equals'
Convert 'equals' to '=='
Convert '==' to 'equals'
Invert Comparison
Flip Comparison
Convert integer literal to decimal
Convert integer literal to octal
Convert integer literal to hexidecimal
Convert between 'assertTrue' with equality comparisons and 'assertEquals'
Convert between 'assertEquals' and 'assertTrue'
Convert between 'assertEquals' and 'assertFalse'
Convert between 'assertEquals' and 'assertNull'
Convert between 'assertTrue' and 'assertFalse'
Convert between multiply and right-shift
Convert between multiply-assign and right-shift
Convert between divide and left-shift
Convert between divide-assign and left-shift
Replace ?: return with if-else
Replace ?: assignment with if-else
Replace ?: declaration with if-else
Replace if-else-return with ?:
Replace if-else-assignment with ?:
Flip ?:
Replace switch statement with if-then-else statement
Replace if-then-else statement with switch statement (Work In Progress)
Wrap body of 'while' statement with {}
Wrap body of 'for' statement with {}
Wrap branches of 'if' statement with {}
Replace simple assigment with operator assignment
Simplify trivial conditional expressions
Simplify trivial if-then-else statements
Change fully qualified name to unqualified name
Merge nested if statements
Detail Exceptions
Requires Aurora build 818 or higher.
If you've got anymore suggestions, the best thing to do is add a Tracker request. If I feel like adding it, I will.
That would be great (OpenAPI and your commitment).
The ipp is great!
To your list I came up with 2 more:
Hide bugs
1) assignment to self
OO Best practices
2) equals should start with identity test
3) equals w/o hashCode and vice versa
"Dave Griffith" <dave.griffith@cnn.com> wrote in message
news:15317366.1055680863201.JavaMail.itn@is.intellij.net...
>
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.
>
>
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.
>
>
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.
>
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.
>
>
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
with an asterisk (*) are slated to have automatic repair links, like many of
the core IDEA inspections do.)
>
maximum lengths).
maximum lengths).
maximum lengths).
maximum lengths).
and maximum lengths).
and maximum lengths).
minimum and maximum lengths).
and maximum lengths).
maximum lengths).
are declared in
contains them
'is'.
>
ignored.
at most once.
at most once.
executed once.
constructors.
>
clause.
RuntimeException or Throwable.
Exception or Throwable.
declarations.
'Throwable', use a subclass instead.
>
object initialization).
>
branches.
not adjacent to the variable name. *
*
non-octal integer literals.
"CloneNotSupported".*
>
(public,protected,private,static,abstract,final,transient,volatile,native,st
rictfp).*
declared as interfaces. *
>
redundant. *
*
conditional operator. *
>
>
>
writeObject.
serialVersionUID field.
>
context.
context.
context.
context.
internationalized context.
>
>
>
replaced by other logging constructs.
logging constructs.
>
types (e.g. new Integer(x).intValue())*
calls to StringBuffer.append().
operation.
concatenation. *
>
>1) assignment to self
Don't know what this means
>2) equals should start with identity test
Good.
>3) equals w/o hashCode and vice versa
This one is already in the core IDEA product.
--Dave Griffith
Amazing list!
Do you have an example?
More restrictive: Do not call non-final methods in constructors.
>Potentially confusing or bug-prone constructs
+ Where doubles are used without a period, append a ".0", e.g instead
of
double x = 5;
it should read
double x = 5.0;
Strange. I prefer the complete opposite: reduce the nesting as much as
possible ("premature returns").
Strange, other guys (e.g. Bloch) say, constants should not be declared
in interfaces.
>Serialization
+ following: avoid Serialization at all :)
Tom
Dave Griffith wrote:
Wow! This would be a very useful tool and I really hope they do open
the inspection API (at least under the same conditions as the PSI).
The following is legal Java, but almost certainly not what you wanted, due to a typo.
Hmm, I had thought of something about implicit conversions between basic types, but wasn't sure how valuable it would be. This is a reasonable start.
>Strange, other guys (e.g. Bloch) say, constants should >not be declared in interfaces.
You're right. I had meant to have checks for both sides or that particular religious dispute, and had forgotten it for the (heretic) 'constants in abstract classes' view. I'll add it back to my list.
>> Where doubles are used without a period, append a ".0"
Sure, "1" will be implicitely converted to "1.0", but when you see a
"1" as an method parameter, you don't know, whether the method accepts
int or doubles; with "1.0" it becomes obviously.
Tom
>
>
Never mind it is already provided. It means things like x = x;
However I have also these
setUp and cleanUp should always call super()
equals and hashCode might need to call super implementation when class
inherit from class that has an equals/hashCode.
Multiple returns complicate refactoring dramatically. What I find is that if I restrict myself to a single return, then I can easily refactor a complex method into several simple ones.
Further, if you have excessive nesting, then perhaps that is a sign that the method needs some refactoring.
I do, however, use multiple returns at the very begining to short-circuit the execution of a method (run-time assertion checking...basically).
Mike
>Multiple returns complicate refactoring dramatically.
That's exactly why the check is on my list. The "don't use continue statements" and "don't use break statements other than at the end of a switch statement branch" are there for similar reasons.
"Dave Griffith" <dave.griffith@cnn.com> wrote in message
news:15317366.1055680863201.JavaMail.itn@is.intellij.net...
That's a long list.
Especially after this plugin is written, it would be nice if the inspection
dialog box could be changed to make it easier to select and deselect
multiple inspections.
"Thomas Singer" <thomas.singer@NOregnisSPAM.de> wrote in message
news:ejsqevgarc4vt7aqms2g9a7bviqq21flle@4ax.com...
>
I would say, that your approach is C-style, while one return is
Pascal-style. I wonder are there other style differencies between different
languages?
+billion
I've seen many a useful inspection on your list. There is one nitpick I have though. Why aren't you planning an automatic repair link for:
Use "literal".equals(var) rather than var.equals("literal").
It shouldn't be that hard, should it?
Just an oversight, thanks for catching it.
-Dave "Not sure I should be accepting feature requests before JetBrains decides to make this possible" Griffith
I second everything that Michael has said.
It is funny that before refactoring I always used multiple returns as a way
to make the code more readable.
Now like Michael, I let nesting tell me when something needs to be
extracted.
And this is all in the same language...
Seriously, I did not remember that Pascal function had only one return.
Thanks for a little trip down the memory lane... (so many old war stories...
;)
Jacques
"Michael Jouravlev" <mikus@mail.ru> wrote in message
news:bckvcd$8j1$1@is.intellij.net...
>
>
different
>
>
I've seen this used a couple times but I've never read why one form is better/more preferred than the other? The latter form appears (to me) more visually clear of what is going on but there is there some bytecode advantage to the other method?
My understanding is that it's to avoid a null pointer exception, but I think if you're using the string at all you should be totally aware of whether it's null or not, and so getting a nullpointerexception is as good a means as any for telling you to fix your code. I could be misunderstanding the reasoning behind the recommendation though; I forget where I heard the NPE thing.
Using "literal".equals(var) works better in situations where var may be null. If you used var.equals("literal") you would need to makes sure var is not null first.
Michael Kirby wrote:
>>> Methods should have only one return point.
>>
>>Strange. I prefer the complete opposite: reduce the
>>nesting as much as possible ("premature returns").
> Multiple returns complicate refactoring dramatically.
.. by machine, but they - can - simplify reading by humans a great lot :
eg.
I write a lot of code like this :
... if (invalid_parameter) return -1 ; .. if (noAction) return untouchedParameter ; // real code starts here ... return result ; ]]>
I'd rather see "Extract Method" improved than change this habit of mine.
Alain
Keith Lea wrote:
> My understanding is that it's to avoid a null pointer exception,
> but I think if you're using the string at all you should be totally
> aware of whether it's null or not, and so getting a
> nullpointerexception is as good a means as any for telling you to
> fix your code.
When it makes sense, I sometimes replace
if ( a.equals ("abc") )
...
if ( a.equals ("defg") )
...
by
if ( null == a )
throw new NullPointerException ('..");
if ( "abc" .equals (a) ) ...
if ( "defg" .equals (a) ) ...
Simplicity of understanding/reading, over simplicity of writing. I tend
to read my code much more than I write it.
But I guess it's just a matter of taste... till M. Fowler says it's a
good practice, then it'll become a recommended practice :)
Alain
+1
Writing nested code to be able to refactor does not solve the
problem, it just works around it.
Tom
On Tue, 17 Jun 2003 09:05:07 -0100, Alain Ravet wrote:
Personally, I'd rather throw an InvalidParameterException for the first
one.
--
...turn to the light - don't be frightened by the shadows it creates,
...turn to the light - turning away could be a terrible mistake
...dream theater - the great debate
On Mon, 16 Jun 2003 18:55:57 +0000, Wouter Zelle wrote:
Inspection -and- intention would be good.
--
...turn to the light - don't be frightened by the shadows it creates,
...turn to the light - turning away could be a terrible mistake
...dream theater - the great debate
I really love idea of making inspection tool extendable. Unfortunately
inspection API is not (at the moment) the thing that could be opened as it
is due to its complexity and ease of improper use. On other hand I can
easily open a subset which is capable of processing problems on a per method
basis like those from "Local Code Analysis" group do.
Well, the question actually is: is this subset somewhat satisfactory to
start from?
--
Best regards,
Maxim Shafirov
JetBrains, Inc / IntelliJ Software
http://www.intellij.com
"Develop with pleasure!"
It would be great!
The vast bulk of the inspections I listed are even simpler than those under "Local Code Analysis". Most require no data flow analysis at all, and are purely local to an expression or statement ("All Collections should be constructed with an initial capacity"). A few more do some local control-flow analysis, but nothing exciting ("Switch statement should not use 'fallthrough'"). You guys are already handling most of the really tough inspections, it's just the easy ones you skimped on.
Overall, I'd guess that a purely method-local API would cover about 80% of the inspections I listed. If you could add access at the class level (sufficient, say, for the existing "equals() and hashCode() not paired" inspection), that rises to about 95%. For the handful left that do require global checks ("Class initializations have circular dependencies", possibly superclass member hiding), I'll happily submit Tracker requests and let you guys handle 'em in your own time.
Thanks for the response!
--Dave Griffith
There's already an intention to flip the .equals() in Intention Power-Pack. If you mean a "yellow-line warning", I agree, but even I'm not hubristic enough to ask for that API to be opened.
See http://www.intellij.net/tracker/idea/viewSCR?publicId=11882
A couple items related to exceptions.
===Empty catch clause===
try {
statement;
}
catch (Exception e) {}
===Missing try{}catch{} blocks around program entry points. ===
It is good practice to add try{}catch{} blocks around all program entry points,e.g main(), subclasses of Runnable.run(), AWT/Swing callbacks, RMI calls, applet start/init, servlet init(), doGet(), doPost(), etc.
This is final backstop to catch runtime errors (NPE,ArrayIndexBoundsE,etc) in the software,
and to try to either (1) take some corrective action
(2) properly log the erorr and/or notify the user. A servlet can display a special page "Internal Error: Please contact ...". A swing client can popup an error dialog "Internal Error:" similary to what INTELLIJ IDEA does.
==== Using Exception instead of the specific Exception===
It could indicate sloppy programming if someone always says try{}catch(Exception e) {} instead of using the specific Exception class. If they want to catch runtime exceptions, they should have multiple catch clauses, one for each specific exception, and then the Exception clause as a catchall for Runtime exceptions.
-Alex