After some years of Java development, one should expect that all lessons are learned and good code is the norm. Yes, really, but this isn't the case. As I work in different projects and reading many books, I am still surprised how often you will find the same ways for doing things wrong.
Create Unnecessary Objects
One of the easiest errors you can do, is to create unnecessary objects. Yes, honestly. There is one famous class in the whole Java environment that can only have two real values. These values are provided as constant values and the only reasonable ones everybody should use as objects of this class. I am talking of the Boolean class. True and false are the only instance and are provided as constant values Boolean.TRUE and Boolean.FALSE. Unfortunately, this class also has a public constructor Boolean(boolean value). This constructor leads to really unnecessary code new Boolean(true) or new Boolean(false). If you see this in your code, it is time to speak with the author and introduce some code inspection tools like FindBugs or PMD.
Use of StringBuffer
In JDK 1.0 times, StringBuffer was a reasonable approach to boost performance for String concatenation. It still is an alternative, but StringBuilder is a better one. The worst thing you can do is to concatenate string literals (aka constant strings) with StringBuffer.append(). Because you are working with literals use the + operator. It gives the compiler the chance to optimize the concatenation during compile time and not to leverage the effect to the runtime.
Method Calls in Loop Conditions
It is really tempting to use the "size" methods of the collection classes (anything like size(), length(), rowCount(), etc.). But before you use them in a loop condition like for(int i = 0; i < array.size(); ++i), take a step back and think about it again. Do you just iterate over it to find something? Or do you modify it in the loop? If the content of the collection will not be modified you should use a construct like for(int i = 0, size = array.size(); i < size; ++i). This is much faster, especially when the method is synchronized.
Repeated Access of the Same Object Through a Method
Very often, in a loop you find code to access the same object through the same method more then once. This makes your code hard to read, less maintainable, and slower. Imagine this method call is synchronized, what an avoidable bottleneck. Of course, you check the method before writing the code, but this does not help for ever. The next version of the API might use a synchronized method. Anyway, to improve the readability, maintainability and performance of the loop, you can introduce a temporary variable, call the method and use the variable throughout the loop. Here is some code:
// Bad
for (int i = 0, size = array.size(); i < size; ++i) {
doSomething(array.get(i));
doSomethingElse(array.get(i));
}// Better
for (int i = 0, size = array.size(); i < size; ++i) {
Object obj = array.get(i);
doSomething(obj);
doSomethingElse(obj);
}
Sure, these two line loop isn't really challenging, but image loops with more lines and line lengths with more then 80 characters...
JDBC DriverManager vs DataSource
Another classic. Use of JDBC in a server environment is much different than in a simple demo environment. You should avoid the DriverManager API (for example, not allowed in Java EE) as this does not allow you to provide connection pools as with a DataSource and very often the wrong class loader is used.
Closing JDBC Resources
Although some JDBC drivers support implicit closing of JDBC resources it is still a recommended practice to close the JDBC resources in the right order at the right place in your code and explicitly. The right order is ResultSet, PreparedStatement, Statement, and Connection. The right place for issuing this code is the finally clause of your try-catch-finally block. Do not reinvent the wheel for this, use the Apache Commons DbUtils library.
Use the Correct ClassLoader
The usage of the ClassLoader in Java is very often the only way to get resources loaded. Using the correct ClassLoader is essential for a perfect running application. The best way to use the correct ClassLoader is by using the Context ClassLoader. You can access it through Thread.currentThread.getContextClassLoader()
Don't Repeat Yourself - DRY
You can write the worst code in every programming language. If you write your code, always ask yourself, whether you have seen this code before, if so, try to refactor both pieces to common code and find ways to put custom code in it. Repetition is boring and introduces errors and problems.
Common Wisdom
This is just the tip of the iceberg. You can get much more interesting points to check your code against from very good references:
Comments (18)
Good blog entry, well done. On the subject of creating unnecessary objects, it is good to: 1) minimize mutability of objects and keep the stateless and 2) if an object is stateless and not a JavaBean, make the object a singleton to keep from creating unnecessary instances of the object.
Posted by Terry Gardner | April 26, 2009 7:43 PM
Posted on April 26, 2009 19:43
Most of the problems you bring up aren't actually problems because the VM should identify these concerns and optimize them out on the fly.
For the one's that aren't, they probably will be soon or should be. Earlier in Java's life people tried to trick the VM and optimize things manually. Now those same optimizations actually slow execution down.
Posted by Charlie Hayes | April 27, 2009 12:23 AM
Posted on April 27, 2009 00:23
method calls in loops will be inlined. size calls will be cached so there shouldn't be a performance penalty for points 3 and 2.
StringBuffer is synchronized where as StringBuilder isn't. Most often string building happens in a local context which translates single threaded access. Until the JVM properly eludes locks, StringBuilder will be a better choice when threading isn't a concern
I see no evidence that a for loop over a collection is better or worse than using an iterator. Again, method calls will be inlined thus one shouldn't see virtual method lookup overheads after a suitable warmup period.
Posted by Kirk Pepperdine | April 27, 2009 7:30 AM
Posted on April 27, 2009 07:30
You are right. Basic knowledge like concatenate String literals or closing resources is not widespread.
But the Sun JDK sources are full of bad examples: they are easy to find. For sure you will take the time to change that during the summer.
Posted by evernat | April 27, 2009 7:33 AM
Posted on April 27, 2009 07:33
@Kirk: You're right. I prefer Iterator instead of using a loop, but when I see a loop the "size" function makes me sick. Even if the accessor functions are inlined, they are hard to read and hard to maintain.
Posted by Olaf Heimburger | April 27, 2009 8:34 AM
Posted on April 27, 2009 08:34
I think most of your points are moot, HotSpot fixes most of these "bottlenecks". Temporary objects, HotSpot recognizes these. Call to method such as size, HotSpot inlines the call. Don't even get me started on ++i ;-)
Posted by Tristan | April 27, 2009 9:33 AM
Posted on April 27, 2009 09:33
@Tristan: Can you guarantee that the target platform uses HotSpot? I can't and will not. My usage of ++i comes from my C/C++ heritage, just too used to it ;-)
Posted by Olaf Heimburger | April 27, 2009 9:40 AM
Posted on April 27, 2009 09:40
You are right.I agree with you!
Posted by java learning boy | April 27, 2009 1:55 PM
Posted on April 27, 2009 13:55
@Olaf, maybe some code would clarify things but get methods should be preferred over object.field notations IMHO.
@Charlie +1
@Tristan object lifecycle can still be a bottleneck even if the objects never survive a single GC. What is hideous about this is that it is generally exists as dark latency.
Posted by Kirk | April 28, 2009 6:39 AM
Posted on April 28, 2009 06:39
@Kirk: I would never use object.field notations and never mention them as good. Effective Java, 2nd: Item 48 is a good reference for using the temporary variables. I will update the blog entry with a code for methods in loops.
Posted by Olaf Heimburger | April 29, 2009 7:22 AM
Posted on April 29, 2009 07:22
Java totally sucks. Just...don't. no.
Posted by topper | April 30, 2009 8:58 PM
Posted on April 30, 2009 20:58
@topper: No matter which programming language you prefer, it does not prevent you from writing ugly, bad, and slow code.
Posted by Olaf Heimburger | May 4, 2009 5:01 AM
Posted on May 4, 2009 05:01
Great entry.
In Java 5 (and newer) you can use the StringBuilder for non syncronized blocks instantied StringBuffer. And remember that the compiler converts all string concatenations ("test " + true) to StringBuffer.
Other good pratice is use final in classes and objects.
Regards
Posted by Garcia | May 4, 2009 2:43 PM
Posted on May 4, 2009 14:43
Other frequent problems are:
- retaining duplicates of strings in memory:
http://kohlerm.blogspot.com/2008/05/memory-consumption-of-netbeans-versus.html
- Overuse of Strings in general
- overuse of finalize(): http://kohlerm.blogspot.com/2008/06/classical-finalizer-problem-in-netbeans.html
Posted by Markus Kohler | May 7, 2009 12:19 PM
Posted on May 7, 2009 12:19
@Markus: Very nice. I never said that this list is complete...
Posted by Olaf Heimburger | May 7, 2009 12:42 PM
Posted on May 7, 2009 12:42
I wonder what kinds of projects you all work in that performance really is an issue that you feel is more important than readable code.
I've been working for 6 years or so in a number of different Rich Client and sometimes in language tooling projects, and only once or twice I was in a situation where performance became an issue.
Usually the user happy if you sync in and out of the UI thread at the right moments.
Making loops look really ugly never was the solution to better performance (probably in the 1980s or so...), usually it was a different conceptual approach.
Posted by Matthias | May 26, 2009 5:13 PM
Posted on May 26, 2009 17:13
Readability is a subjective topic. Coming from an 20x80 development environment, I'm sick of lines that wrap around and/or repeat 80% of it. Every time you repeat yourself, you should consider using a temporary. This might improve your code because you avoid errors, repeated side effects, synchronization issues and so on. Working in environments the overall response time has to be less then 2 seconds, every sub-second counts. Speaking of old school programming style with loops, it is a habit one gets used to when found that it could be faster and safer then recursion. One of the major complaints on Ant is that it has no support for loops, sigh.
Anyway, thank you for reading this one.
Posted by Olaf Heimburger | May 26, 2009 6:09 PM
Posted on May 26, 2009 18:09
I think a blanket statement of using the context classloader to obtain resources is incorrect. I also don't think it affects performance but correctness.
Posted by Ron | June 4, 2009 4:19 PM
Posted on June 4, 2009 16:19