Java Anti-Patterns

This page collects some bad code that may not look so obiously bad for beginners. Beginners often struggle with the language syntax. They also have little knowledge about the standard JDK class library and how to make the best use of it. In fact we collected all examples from everyday junior code. And we modified the original code to give it example character and such that it highlights the problems. String concatenation String s = ""; for (Person p : persons) { s += ", " + p.getName(); } s = s.substring(2); //remove first comma This is a real performance killer: O(p.length²). The repeated concatenation of strings in a loop causes excess garbage and array copying. Moreover it is ugly that the resulting string has to be fixed for an extra comma. StringBuilder sb = new StringBuilder(persons.size() * 16); // well estimated buffer for (Person p : persons) { if (sb.length() > 0) sb.append(", "); sb.append(p.getName); } Lost StringBuffer performance StringBuffer sb = new StringBuffer(); sb.append("Name: "); sb.append(name + '\n'); sb.append("!"); ... String s = sb.toString(); Despite good intentions the above code is not perfect. The most obvious mistake is the string concatenation in line 3. In line 4 a appending char would be faster than appending a String. An also major omission is the missing length initialization of the buffer which may incur unnecessary resizing (array copying). In JDK 1.5 a StringBuilder instead of StringBuffer should have been used: because it is only a local variable the synchronization is overkill. StringBuilder sb = new StringBuilder(100); sb.append("Name: "); sb.append(name); sb.append('\n'); sb.append('!'); String s = sb.toString(); Testing for string equality if (name.compareTo("John") == 0) ... if (name == "John") ... if (name.equals("John")) ... if ("".equals(name)) ... None of the above comparisons is wrong - but neither are they really good. The compareTo method is overkill and too verbose. The == operator tests for object identity which is probably not what you want. The equals method is the way to go, but reversing the constant and variable would give you extra safety if name is null plus an increase in speed because the equals method is always called from the same object if used in a loop. When testing for empty strings it's faster to check if their length is 0. Because the equals method may first calculate a hash value. if ("John".equals(name)) ... if (name.length() == 0) ... Converting numbers to Strings "" + set.size() new Integer(set.size()).toString() The return type of the Set.size() method is int. A conversion to String is wanted. These two examples in fact do the conversion. But the first incurs the penalty of a concatenation operation. And the second creates an intermediate Integer wrapper. The correct way of doing it is String.valueOf(set.size()) Not taking adavantage of immutable objects zero = new Integer(0); return Boolean.valueOf("true"); Integer as well as Boolean are immutable. Thus it doesn't make sense to create several objects that represent the same value. Those classes have built-in caches for frequently used instances. In the case of Boolean there are even only two possible instances. The programmer can take advantage of this: zero = Integer.valueOf(0); return Boolean.TRUE; XML parsers are for sissies int start = xml.indexOf(""); int end = xml.indexOf(""); String name = xml.substring(start, end); This naïve XML parsing only works with the most simple XML documents. It will however fail if a) the name element is not unique in the document, b) the content of name is not only character data b) the character data of name contains escaped characters c) the document uses XML namespaces. XML is way too complex for string operations. There is a reason why XML parsers like Xerces are a over one megabyte jar files! The equivalent with JDOM is: SAXBuilder builder = new SAXBuilder(false); Document doc = doc = builder.build(new StringReader(xml)); String name = doc.getRootElement().getChild("name").getText(); The XML encoding trap String xml = FileUtils.readTextFile("my.xml"); It is a very bad idea to read an XML file and store it in a String. An XML specifies its encoding in the XML header. But when reading a file you have to know the encoding beforehand! Also storing an XML file in a String wastes memory. All XML parsers accept an InputStream as a parsing source and they figure out the encoding themselves correctly. Undefined encoding Reader r = new FileReader(file); Writer w = new FileWriter(file); Reader r = new InputStreamReader(inputStream); Writer w = new OutputStreamWriter(outputStream); String s = new String(byteArray); // byteArray is a byte[] byte[] a = string.getBytes(); Each line of the above converts between byte and char using the default platform encoding. The code behaves differently depending on the platform it runs on. This is harmful if the data flows from one platform to another. It is considered bad practice to rely on the default platform encoding at all. Conversions should always be performed with a defined encoding. Reader r = new InputStreamReader(new FileInputStream(file), "ISO-8859-1"); Writer w = new OutputStreamWriter(new FileOutputStream(file), "ISO-8859-1"); Reader r = new InputStreamReader(inputStream, "UTF-8"); Writer w = new OutputStreamWriter(outputStream, "UTF-8"); String s = new String(byteArray, "ASCII"); byte[] a = string.getBytes("ASCII"); Unbuffered streams InputStream in = new FileInputStream(file); int b; while ((b = in.read()) != -1) { ... } The above code reads a file byte by byte. Every read() call on the stream will cause a native (JNI) call to the filesystem of the underlying operating system. JNI calls are expensive. The number of native calls can be reduced dramatically by wrapping the stream into a BufferedInputStream. Reading 1 MB of data from /dev/zero with the above code took about 1 second on my laptop. With the fixed code below it was down to 60 milliseconds! That's a 94% saving. This also applies for output streams of course. And it is true not only for the file system but also for sockets. InputStream in = new BufferedInputStream(new FileInputStream(file)); Infinite heap byte[] pdf = toPdf(file); Here a method creates a PDF file from some input and returns the binary PDF data as a byte array. This code assumes that the generated file is small enough to fit into the available heap memory. If this code can not make this 100% sure then it is vulnerable to an out of memory condition. Especially if this code is run server-side which usually means many parallel threads. Bulk data must never be handled with byte arrays. Streams should be used and the data should be spooled to disk or a database. File pdf = toPdf(file); Catch all: I don't know the right runtime exception Query q = ... Person p; try { p = (Person) q.getSingleResult(); } catch(Exception e) { p = null; } This is an example of a J2EE EJB3 query. The getSingleResult throws runtime exceptions when a) the result is not unique, b) there is no result c) when the query could not be executed due to database failure or so. The code above just catches any exception. A typical catch-all block. Using null as a result may be the right thing for case b) but not for case a) or c). In general one should not catch more exceptions than necessary. The correct exception handling is Query q = ... Person p; try { p = (Person) q.getSingleResult(); } catch(NoResultException e) { p = null; } Exceptions are annoying try { doStuff(); } catch(Exception e) { log.fatal("Could not do stuff"); } doMoreStuff(); There are two problems with this tiny piece of code. First, if this is really a fatal condition then the method should abort and notify the caller of the fatal condition with an appropriate exception. Hardly ever you can just continue after a fatal condition. Second, this code is very hard to debug because the reason of the failure is lost. Exception objects carry detailed information about where the error occurred and what caused it. If you catch highlevel exceptions then at least log the message and stack trace. try { doStuff(); } catch(Exception e) { throw new MyRuntimeException("Could not do stuff because: "+ e.getMessage, e); } Catching to log try { ... } catch(ExceptionA e) { log.error(e.getMessage(), e); throw e; } catch(ExceptionB e) { log.error(e.getMessage(), e); throw e; } This code only catches exception to write out a log statement and then rethrows the same exception. This is stupid. Let the caller decide if the message is important to log and remove the whole try/catch clause. Incomplete exception handling try { is = new FileInputStream(inFile); os = new FileOutputStream(outFile); } finally { try { is.close(); os.close(); } catch(IOException e) { /* we can't do anything */ } } If streams are not closed, the underlying operating system can't free native resources. This programmer wanted to be careful about closing both streams. So he put the close in a finally clause. But if is.close() throws an IOException then os.close is not even executed. Both close statements must be wrapped in their own try/catch clause. Moreover, if creating the input stream throws an exception (because the file was not found) then os is null and os.close() will throw a NullPointerException. To make this less verbose I have stripped some newlines. try { is = new FileInputStream(inFile); os = new FileOutputStream(outFile); } finally { try { if (is != null) is.close(); } catch(IOException e) {/* we can't do anything */} try { if (os != null) os.close(); } catch(IOException e) {/* we can't do anything */} } The exception that never happens try { ... do risky stuff ... } catch(SomeException e) { // never happens } ... do some more ... Here the developer executes some code in a try/catch block. He doesn't want to rethrow the exception that one of the called methods declares to his annoyance. As the developer is clever he knows that in his particular situation the exception will never be thrown, so he just inserts an empty catch block. He even puts a nice comment in the empty catch block - but they are famous last words... The problem with this is: how can he be sure? What if the implementation of the called method changes? What if the exception is still thrown in some special case but he just didn't think of it? The code after the try/catch may do the wrong thing in that situation. The exception will go completely unnoticed. The code can be made much more reliable by throwing a runtime exception in the case. This works like an assertion and adheres to the "crash early" principle. The developer will notice if his assumption was wrong. The code after the try/catch will not be excecuted if the exception occured against all honest hope and expectation. If the exception really never occurs - fine, nothing changed. try { ... do risky stuff ... } catch(SomeException e) { // never happens hopefully throw new IllegalStateException(e.getMessage(), e); // crash early, passing all information } ... do some more ... Overkill initialization public class B { private int count = 0; private String name = null; private boolean important = false; } This programmer used to code in C. So naturally he wants to make sure every variable is properly initialized. Here however it is not necessary. The Java language specification guarantees that member variables are initialized with certain values automatically: 0, null, false. By declaring them explicitly the programmer causes a class initializer to be executed before the constructor. This is unnecessary overkill and should be avoided. public class B { private int count; private String name; private boolean important; } Too much static private static Log LOG = LogFactory.getLog(MyClass.class); Many developers store Log instances in static member variables. This is somehow correct from a design point of view. But it is totally unnecessary in this case. The LocFactory already keeps static references to the Log objects, so the getLog call is essentially a lookup in a hash table and not expensive. But having every class in your project equiped with a static variable is very bad as it prevents class garbage collection. Only in rare cases like serializable classes are static variables the way to go. It also prevents you from using getClass() to find the current class name. Instead you must hardcode the class name so you can't share the Log instance in subclasses but must declare it private. protected Log log = LogFactory.getLog(getClass());