The Best Groovy Inspections You’re Not Using
January 18th, 2012Many, many tools perform static analysis on code. There are all sorts of automated ways to look through your code and tell you if there are likely errors or not. FindBugs, PMD, and CheckStyle are some of the big names from the Java world. On the Groovy side we have two real options: IntelliJ IDEA and CodeNarc.
This post highlights my favorite static analysis rules for Groovy in IDEA that are not enabled by default. Groovy in IDEA 11 has well over 100 rules, but less than half of them are activated when you install the product. To turn these rules on you’ll need to go into Settings (Ctrl+Alt+S) and enable them under Inspections.
So here are my favorites that I think you should turn on:
Threading
Threading comes first because these inspections have saved my butt in the past. None of them are enabled by default, so do yourself a favor and enable a few. These ones are all must haves:
* Access to static field locked on instance data – Reading and writing a static field from multiple threads is not thread-safe. It needs to be done within a synchronized block. Do you know what happens when you synchronize on instance data, like this:
class MyClass {
static resource
final lock = new Object()
def initialize() {
synchronized(lock) {
resource = new Resource()
}
}
}The object ‘lock’ is an instance field, not a static field. So this assignment within the synchronized block is not thread-safe. It is possible that the static field is accessed from multiple threads, which can lead to unspecified side effects.
* Synchronization on non-final field – If you are accessing a variable within a synchronized block then your synchronization token must be final. Consider this code:
class MyClass {
def resource
def lock = new Object()
def initialize() {
synchronized(lock) {
resource = new Resource()
}
}
}The field ‘lock’ is not final, so different instances of MyClass may have different values for lock. The result is that different threads may be locking on different objects even when operating on the same object. Make your locks final.
* Synchronization on variable initialized with literal – Do you understand the problem with synchronizing on a primitive literal? Consider this code:
class MyClass {
def lock = 'my lock'
def initialize() {
synchronized(lock) {
// do something
}
}
}Strings are interned by the JVM, so the string ‘my lock’ may also be used by another class. Numbers are even worse because they can be assigned from a cache. Some other object in the system may be initialized with the same literal, and could create an accidental (or malicious) dead-lock situations.
* Unsynchronized method overrides synchronized method – In some limited circumstances you may actually want to override a synchronized method with an unsynchronized one. But probably not. More than likely you don’t even know and you are accidentally creating a subtle bug. If the parent is synchronized then the child should be also. Or better yet, make the parent final.
Probably bugs
* Named arguments of constructor call – This inspection warns you when something like the following happens when using named arguments, which is almost certainly a bug:
class Order {
int orderId
}
new Order(orderid: 5)See the problem? The field is named ‘orderId’ with a big ‘I’ and the constructor call uses ‘orderid’ with a small ‘i’. RuntimeException if you run this code. This is a type of bug that CodeNarc cannot find because the type information of the classes is so limited. IDEA keeps track of types much better and can do nice things like this.
* Access to unresolved expression – There has been a lot of talk over the years about adding static compilation to Groovy. Some people want to be warned by the compiler or in the IDE if their Groovy code is referencing an object that does not exist. For example, they’d like this to produce a warning or error:
def c = { parm ->
println it
}See the problem? The closure parameter is named ‘parm’ but the closure references the parameter by the standard name ‘it’. The access to unresolved expression inspection turns this into a warning or error in the IDE. IDEA is quite smart about it too and knows all about delegate/super and the other Groovy dynamic variables. If you like this sort of thing then be sure to read up on Grumpy mode in Groovy 2, coming soon.
Control Flow
There are many features in Groovy that simplify standard and verbose Java code. Two of these are the Elvis operator and the Null-Safe Dereference. IDEA contains inspections to help you migrate to the new way of writing code with these features.
* Conditional expression can be elvis – This inspection migrates you from code like this:
def x = value != null ? value : 2
And suggests you rewrite it into the equivalent Groovy:
def x = value ?: 2
Is it always safe to make this transformation? (Think about it for a few seconds). GroovyTruth means it is not. Technically speaking, the two code snippets are not equivalent: value != null may be true while value is false, such as when value is an empty String. Overall, I like the inspection but you need to be careful with it.
* Conditional expression can be conditional call – This inspection promotes the use of the Null-Safe dereference. This following code produces a violation:
def x = value != null ? value.toString() : null
… and you will be prompted to transform it ito:
def x = value?.toString()
These two code blocks really are the same and this can be safely accepted as the correct way to write the code.
* If statement (or conditional) with identical branches - I periodically refactor code and accidentally leave an if or conditional statement with identical branches. These are of course redundant and can be cleaned up by removing the if expression. If you have good unit tests and use automated refactorings then this sort of things happens from time to time. The cleanup reminder is nice.
Error Handling
* Unused catch parameter - My favorite Error Handling inspection is ‘Unused catch parameter’. Consider this code:
try {
doSomething()
} catch (e) {
LOG.error('unexpected error occurred')
}What happens to the stack trace in this scenario? It gets eaten up, never to appear in a log. You shouldn’t have any unused catch parameters. If you really don’t care about the exception then name it ‘ignore’ or ‘ignored’ to suppress the warning.
Validity issues
* Duplicate switch case – Groovy has Switch on Steroids, meaning you can put almost any object in the case statement, including regular expressions:
switch (input) {
case ~/[0-9]/ : println 'numeric!'; break
case ~/[0-9]/ : println 'a number'; break
}See the problem? The same regex appears twice. The second case will never be executed, even if it would match. As long as your case statements have literals in them then IDEA can verify that there are no duplicates.
Naming Conventions
Lastly, there are a whole bunch of naming convention inspections that you can activate and configure. If you’re not using CodeNarc (why not?) then you should at least activate some of these. Standards, on a whole, are good to adhere to.
IDEA Inspections of CodeNarc?
So which should you use, IDEA or CodeNarc? These two tools complement each other. There is good reason to use both CodeNarc and IDEA together. CodeNarc does have more inspections than IDEA, but the IDE integration in IDEA is better. Pressing Alt+Enter typically rewrites the offending code, and the speed of the actions are much better in IDEA. Also, the static analysis rules do not overlap between the products because the CodeNarc developers (that’s Chris Mair and myself) already use IDEA and made a conscious effort not to duplicate anything.
And if you do want to use CodeNarc and IDEA, then you might try out the CodeNarc IDEA plugin.
If you like this then you might check out some of my other IDEA related posts: The 10 Best Inspections You’re Not Using (in Java), or the IDEA archive on my blog and the Canoo blog.
And remember if you need Groovy and Grails help, then give us a call or drop me an email at hamlet.darcy@canoo.com
Posted by Hamlet
