In the post Common Red Flags in Java Development I looked at some practices that are not necessarily wrong or incorrect in and of themselves, but can be indicative of potentially greater problems. These “red flags” are similar to the concept of “code smells” and some of the particular “red flags” I cite in this post have been called “code smells.” As I stated in the initial post, several of these “red flags” are considered significant enough that static code analysis tools and Java IDEs will flag them.
“Logging” Messages Directly to stdout or stderr
Logging frameworks have been available for a long time in Java and today we have a wide variety of logging frameworks (some of which build on each other) including traditional Log4j 1.2, log4j 2, java.util.logging (Java Logging API), Apache Commons Logging, and SLF4J. Given this, it surprises me when I see System.out and System.err references in Java code.
There are multiple reasons that the existence of Java code directly writing to standard output or standard error is of concern. One reason for concern is that this might mean immature code that was intended to later be changed to logging but never got that finishing attention. Another disadvantage of referencing standard output and standard error is that the “logged” messages will likely not appear in the log files with the rest of the logs written by logging frameworks. A third problem is that there are numerous nice features provided by a logging framework that are not provided by simple writing to standard output and standard error. These include the ability to easily control the level of messages which are logged, the ability to control whether to take the performance hit to generate large output strings given the specified level of logging, the ability to easily associate caught exceptions with a logged error message, and the ability to easily redirect the output to different destinations and with different formats. Although all of this can be manually done when working directly with output and error streams, it requires customized work rather than being available “out of the box.”
There are manifestations in Java code that write to standard output and standard error other than direct access using System.out and System.err (though they typically implicitly System.out and System.err). For example, Throwable.printStackTrace() [more commonly used in handling of Exceptions], as its Javadoc states, “Prints this throwable and its backtrace to the standard error stream.”
Use of StringBuffer Rather than StringBuilder
This is admittedly a very minor thing, but it can indicate outdated Java code (StringBuffer introduced in JDK 1.0 and StringBuilder introduced in J2SE 5) or Java code where the developer did not understand the the differences between StringBuffer and StringBuilder. In most cases, the performance difference between the two is not significant to the application at hand, but because StringBuilder is preferable in most cases where I’ve seen StringBuffer used, one may as well enjoy the typically slight performance benefit of using StringBuilder. I’m having a difficult time recalling a single instance in which I have seen StringBuffer used in which StringBuilder could not have been used instead. A related red flag is mixing String concatenation with a StringBuilder in its constructor or overloaded append methods.
Too Many Parameters in Methods and Constructors
I’m always concerned about a method or constructor not being used correctly by its clients when the method or constructor has too many parameters, especially if several of the parameters are of the same type. If a method accepts three Strings and three booleans, for example, it is easy for the client to mix up the particular values it passes in. The compiler cannot help much in this case and the only way to detect the source of the problem (or even if a problem exists at all) is at runtime (via unit tests or other tests or, sadly, during regular execution of the software). Too many parameters can be a “red flag” for improper design as well. I am not going to look at this “red flag” any deeper in this post because I have already covered this “red flag,” multiple ways to resolve it, and issues it presents in a series of eight blogs posts.
- Too Many Parameters in Java Methods, Part 1: Custom Types
- Too Many Parameters in Java Methods, Part 2: Parameters Object
- Too Many Parameters in Java Methods, Part 3: Builder Pattern
- Too Many Parameters in Java Methods, Part 4: Overloading
- Too Many Parameters in Java Methods, Part 5: Method Naming
- Too Many Parameters in Java Methods, Part 6: Method Returns
- Too Many Parameters in Java Methods, Part 7: Mutable State
- Too Many Parameters in Java Methods, Part 8: Tooling
Excessive Explicit Casting
Explicit casting is probably one of the best examples of a red flag situation in which the casting itself may not affect any functionality or logic from working correctly, but is a tip-off that things are not as well as they could be. Casting can imply poor design choices (such as not using polymorphism correctly, using inheritance when inappropriate, or forcing things to go together that were never designed to go together). Explicit casting is certainly appropriate or required in many situations (such as when obtaining a Spring Framework context bean), but explicit casting can also be used as a crutch to get things working that have not been designed as carefully as they could have been. Casting can also be indicative of APIs that are too broad or interfaces used in APIs that are too broad (highlighted in the next item).
Use of Too Broad of an Interface or Class
I have often seen the Collection interface used as a method parameter or return type when Set or List or even more specific interface was more appropriate. For example, a method that returns a Collection but expects the client code to know that the returned Collection is ordered, should return a List or more specific interface or implementation of List. Likewise, if a sorted Set is expected by a method, it should advertise the method as expecting a SortedSet or similar interface or implementation class. When the interface or class returned or expected as a parameter is too broad for the given contract, somebody is forced to “know” that’s the case and to cast to the appropriate level to get the functionality that they are dependent on.
Using the appropriate level or interface or class goes beyond helping avoid unnecessary explicit casting. The appropriate type level advertises and enforces the method contract better than mere documentation can. However, it goes further than that. In some cases, significant runtime exceptions can occur when the advertised interface is too broad to capture assumptions in the method’s contract. For example, a generic interface might optionally support a method but the actual implementation of that interface throws an UnsupportedOperationException when called because it does not implement that optional method. Between UnsupportedOperationExceptions and ClassCastExceptions, use of overly broad interfaces or classes can lead to potentially serious runtime issues.
This is not to say that interfaces or broad classes should be avoided. Rather, it is to say that the appropriate degree of abstraction should be used in return types and parameter types so that the expected behavior for both sides of the invocation is appropriately advertised and enforced.
Use of List.addAll()
Use of one of the overloaded List.addAll() methods makes me nervous and is a bright red flag when I see it in code. That doesn’t mean it’s always wrong to use it, but it does seem like I’ve seen a lot of bloated memory issues due to misuse of this. Because Lists will add “duplicate” objects as much as the developer likes, errant code can exponentially fill these Lists up with redundant objects. Negative impacts from this range from potentially impeded performance to running out of memory. When I see use of List.addAll(), I carefully review the code and unit test it extra carefully to ensure that its memory consumption doesn’t get out of control. As described in the previous “red flag,” any use of Collection.addAll() must be analyzed similarly to List.addAll() until it is known for certain that the Collection is really not a List.
Non-Java Dialects
Perhaps the best example of all for me of a “red flag” is the frequent use of idioms and code conventions that are contrary or significantly different to “generally accepted Java coding standards.” Nothing about using names, case, or other style issues directly impact the correctness or performance of the code. However, these discrepancies remain a “red flag” warning of potential actual problems with logic or performance because use of these significantly non-standard idioms and conventions imply that the developer may be new to Java and hence may have made mistakes common to those new to Java. A good article on the importance of writing Java code “without an accent” is Speaking the Java language without an accent. In that article, author Elliotte Rusty Harold writes about how such code is more difficult to read and maintain.
In relatively rare cases, this can move from a style issue to an impacting issue. This occurs when one writes Java code in a manner that makes most sense in a different language (such as C or C++) but does not make as much sense as alternative approaches in Java.
Conclusion
As was the case in my first post on red flags in Java code, the “red flags” discussed in this post are generally things that are not necessarily incorrect when used in appropriate and select circumstances, but often do indicate that things are not as well as they could be in the greater application.
