In several years of developing, reading, reviewing, and maintaining hundreds of thousands of lines of Java code, I have become accustomed to seeing certain "red flags" in Java code that often (but perhaps not always) imply problems with the code. I'm not talking about practices that are always wrong, but rather am talking about practices that might, in limited circumstances, be appropriate but generally are a sign of something wrong. These "red flags" may at times be innocent, but often warn of an inevitable "heap of hurt" that is inevitably coming. Here I summarize some of these and briefly discuss situations in which they might be okay as well as describing why they typically are not okay.
Many of these "red flags" are significant enough to warrant warnings from code analysis tools such as FindBugs. The popular Java IDEs flag many of these as well. However, I have seen developers miss these more literal flaggings from these tools and IDEs either because they had the options turned off or because they had, out of habit or because not understanding the risk associated with the flag, ignored the warning.
Use of == (rather than .equals) with References
Most Java developers learn to compare primitives with
== and to compare reference types with
.equals. This is generally an easy rule to remember and typically serves Java developers well. There are times when using
== to compare standard Java type references (String, Integer, Long, etc.) will work fine, but counting on the values to be cached so that this works is not a good idea. There are also cases where one might want to check for identity equality rather than content equality and then
== is appropriate for comparing references. I prefer Groovy's approach here where
== works like
=== explicitly and obviously communicates the developer's desire to more strictly compare identities. The same argument applies to the use of
!= for comparing two references to be a red flag because this will always return true if the two objects being compared do not share the same identify (memory address) even if they share the same content.
Use of .equals (rather than ==) with Enums
Frankly, it often doesn't matter whether the Java developer uses
.equals with enums. However, I do prefer use of == with enums. The most significant reason for this preference is that use of
== with enums eliminates making the possible mistake of comparing an enum to some unrelated object (to which it will never be equal). The Object.equals(Object) method necessarily must accept any Object, but this means the compiler cannot enforce that the passed-in object is actually of the type being compared. I generally prefer static compile time detection of problems over dynamic runtime detection of problems and use of
== with enums satisfies this preference. The same arguments, of course, apply to use of
!.equals when comparing enums.
Magic Numbers and Literal Strings
I know it's "Computer Science 101," but I still have seen use of "magic numbers" and literal strings in Java code far too often. These cry out as "red flags" for future maintainability and give me serious doubt about the correctness of the current application. Representing these as constants (or better yet as enums when appropriate) in a single location improves future maintainability and also gives me a higher degree of confidence that all code using those values are using the same values. In addition, centralized defined constants and enums make it easy to use an IDE's "Find Usages" feature to find all of the "clients" of those constants.
When I see a finite set of related String constants, I often think an enum would serve better. This is especially true for a set of String constants with a high degree of cohesiveness that allows an enum to represent nicely the concept those Strings constitute. The enum provides compile-time static type safety and potential performance advantages over the String constants. In terms of program correctness, it is the compile-time safety that interests me most.
Use of Java's "Goto"
I almost did not include this item on use of branching to labeled code in this post because the truth is that most of the few instances I have seen this used in production code are justifiable. In other words, this item is listed in this post more because of the potential for its abuse and use in the wrong ways than in what I've actually witnessed. In most cases I've seen, the Java developer applying Java's "goto" is doing so to avoid far messier and more difficult to read code. The fact that this is seldom used may be partially to credit for its being used properly. If it was used often, it's likely that most of those uses would be in bad taste.
Depending on Scope for Appropriate References of Variables of the Same Name
This item falls under the category of never really appropriate in my opinion, but definitely workable and even done intentionally by some Java developers some of the time. The best example of this is when a Java developer points a variable passed into a method to another reference during the method's execution. That variable that was pointing to the method parameter temporarily points to whatever alternate it is assigned until the method ends, at which point it falls out of scope. Placing the final keyword in front of the parameter's definition in the method signature will lead to a compiler error for this case and is one of the reasons I like final in front of all my method parameters. To me it's clearer and more readable to simply declare a new variable local to that method because it is only be used locally to that method anyway. More importantly, as the reader of the code, I have no way of knowing if the developer intentionally wanted that parameter's name to be used locally only for a different value or if they had introduced a bug thinking that reassigning that parameter to a new reference would actually change it on the calling side. When I see these, I either work with the original developer or glean from unit tests and production use what the intent is and make it more clear (or fix it if the invoking client's value is intended to be changed).
Mismatched equals(Object) and hashCode() Methods
Although I believe a toString() method should be written for just about every Java class that is written, I don't feel the same way about equals(Object) and hashCode() overrides. I think these should only be written when the class is intended to be used in situations requiring these methods because their existence should imply a certain degree of extra thoroughness in their design and development. In particular, the equals and hashCode methods need to meet their intent and advertised (in Object's API documentation) contracts and need to be aligned with each other. Most IDEs and analysis tools will identify when one method exists without the other. However, I like to make sure that the same attributes use for equals are used for hashCode and I prefer them to be considered in the same order in both methods.
Lack of Javadoc Comments
I like all contract methods (especially public methods) to be commented with Javadoc comments. I have also found it useful to have attributes commented with what they are intended to store. I have heard the excuse for no Javadoc comments when the code is "self-documenting," but I can almost always tell the person making that claim one or more examples of how a simple Javadoc comment can relay the same information as it would take to spend much longer than that deciphering the code. Even longer method names can typically not be long enough to specify all the expected input conditions and output expectations of a given method. I think many Java developers, like me, prefer to read the Javadoc comments when using the JDK rather than reading JDK code. Why then should our own internal code be any different? I do read source code when the comments are insufficient or behavior appears different than what is advertised or when I have reason to believe the comments might be old or shoddy.
I also like to see Javadoc comment for class attributes. Some people prefer to comment the public get/set methods with attribute information, but I don't like that approach as well because it assumes that get/set methods will always be available (an assumption I don't like to make).
Implementation Details in Methods' Javadoc Comments
Although I feel that no Javadoc comments is a red flag, having the wrong type of Javadoc comments is also a red flag. Javadoc comments should not explain the implementation details, but should rather focus on method expectations of clients (parameters) and client expectations of the method (return type and possibly thrown exceptions). In a truly object-oriented system, the implementation should be modifiable underneath the public interface and so it seems inappropriate to place these implementation details in the interface documentation. This is a "red flag" because presence of implementation details in the Javadoc makes me suspicious of the timeliness of the comments. In other words, these are the types of comments that often get outdated and flat-out wrong quickly as code evolves.
Comments in the Source Code
Although lack of comments on the interfaces (typically in Javadoc) is a red flag, the existence of comments within the body of methods and other source code is also a red flag indicating potential issues. If code needs to be commented to understand what it's doing, it is likely that the code is more complicated than it needs to be ("comments are the deodorant for stinky code" is often as true as it is funny). Like many of these "red flags" in this post, there are exceptions when I have seen in-code comments that are informative and in which I cannot think of a better way to present the code to remove the need for those comments. However, I believe in-code (not interface descriptive Javadoc comments) should be relatively rare and should focus on "why" something was done rather than "how" it was done. The code should speak for itself on the details of "how," but typically cannot be written to implicitly explain the "why" (because of customer/management direction, design decision, formal interface requirements, formal algorithm requirements, etc.).
Implementation Inheritance (extends)
I have seen many cases where use of
extends (implementation inheritance) is done well and is appropriate for the situation. I've seen many more cases where the opposite is true and implementation inheritance causes more trouble than good. Allen Holub has written that extends is evil and the Gang of Four Design Patterns book has a huge focus on reasons why composition is often preferable to implementation inheritance. The longer that I have developed Java code and, more importantly, the longer I have maintained Java code, the more convinced I have become of the virtues of composition and the vices of implementation inheritance. As I stated, I have seen implementation inheritance used to good effect, but more often than not classes are "shoehorned" into fitting into an inheritance hierarchy and child classes get riddled with UnsupportedOperationExceptions or "noop" implementations because a method they inherit doesn't really apply. Throw in some of the issues with inheritance outlined in Effective Java (such as equals and hashCode methods implementations for inheriting concrete classes) and it can become a real mess. Implementation inheritance is not always bad, but it is often used badly and so is a red flag.
It's never a good thing to have unused code sitting around. It doesn't take too long for people to forget how it was used or why it was used. Not too long after that, developers start to wonder if it was left around for a reason. Dead code not only increases code that must be read and potentially maintained, but in world of coding via IDE completion, dead methods can be easily and accidentally invoked based solely on their name and argument list. Dead code is also often symptomatic of a neglected code base.
Commented Out Code
Commented out code may not be as bad as executable dead code because at least it cannot be accidentally invoked and it is more obvious that it is not being used, but it still is a red flag because it indicates potential code base neglect. Like dead executable code, the more time that passes between the time the code is commented out, the more difficult it is to know why the code was ever there, why it was commented out, and why it wasn't simply removed when no longer needed. Developers may be afraid to remove it because it obviously must have been important to leave in before but no one can remember why.