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 ReferencesMost 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 .equals and === 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.
Frankly, it often doesn't matter whether the Java developer uses == or .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 != versus !.equals when comparing enums.
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.
String ConstantsWhen 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 NameThis 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() MethodsAlthough 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 CommentsI 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 CommentsAlthough 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 CodeAlthough 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 CodeCommented 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.
To-Do StatementsIt has become so common to add "to-do" statements to code that modern Java IDEs provide special support and features for them. These features do help because they often put the todo markers in a list for reviewing, but "to-do" comments still remain red flags and can bring with them some of the same problems as dead code and commented out code. I have definitely used "to do" comments for short-term reminders, but I think it is best to include an "expiration date" and contact information with the "to do comment" as well as, if available, the people or events that need to transpire to allow the "to do" to be worked off. Having the expiration date, contact information, and events or people necessary to address the "to do" reduce the likelihood of have "to do" comments in the code because no one remembers exactly what they were for but no one dares remove them because they were something thought important to do at one point. When I see a "to do" statement in code, I cannot help but wonder if the code is somehow lacking in functionality.
Compiler Warnings and IDE/Tool Warnings/Hints/FindingsObvious examples of Java red flags are the warnings put out by the javac compiler and the findings and hints that IDEs and other code analysis tools provide. Each of these findings and warnings is potentially its own red flag. In fact, many of the red flags I've cited in this post are warned about or hinted about by the javac compiler or by tools and IDEs. Not only do these warnings, hints, and findings directly correspond to many Java code red flags, but their aggregated existence is a huge red flag indicating a potentially neglected code base in which serious warnings (even errors by some definitions) might be lost in the deluge of warnings and hints.
Compiler Warnings and IDE/Tool Warnings/Hints/Findings Turned OffI definitely don't feel that every issue identified in my Java code by FindBugs is necessarily a bug or a defect. In fact, in some cases, I even disable some of the findings because I don't agree with them and they clutter the FindBugs output. That being said, such deselection of findings should be done carefully to ensure that only true non-findings are ignored and things important to the development team are obvious. In a similar vein, I like to have many of my IDE's warnings turned on, but do have a few turned off. I also don't think it's a good idea to build Java applications with javac's warnings disabled. Disabling these warnings and findings may remove the red flags the presence of the warnings represent, but the action of turning them off is potentially an even larger red flag because these warnings and hints point out so many potential red flags that could be addressed.
Too Clever / Science Fair Projects / Over-Engineered / Premature OptimizationMy last category of red flags is the large category of overly complicated software that is often the result of one of the common developer dysfunctional behaviors. Code that is overly clever to the point of not being readable or focuses on flexibility or (premature) optimization when not needed and at the expense of readability often has other problems. Developers who over-engineer their code may be doing this to see if they can or to try something new or just to shake things up, but these are rarely behaviors are rarely conducive to good quality software. Once software becomes too complex and difficult to understand, it is not as likely to be maintained properly and is more likely to have incorrect changes made to it.
At common telltale sign of one example of this category of red flags is when reading code and wondering why in the world the developer did not implement this in a much more obvious and straightforward way. On one hand, you might be impressed that they were aware of and able to apply some advanced feature, but on the other hand you know that this probably was more complex than it should have been. There are many manifestations of this category of red flags, but I seem to commonly see them in a few areas. One area in particular is pushing too much functionality that works perfectly well in static Java code to more dynamic constructs via reflection, Spring or other dependency injection, dynamic proxies, observers, and so forth. All of these things are handy and useful when applied correctly, but I have also seen all of these things overused and frequently abused, making it difficult for developers to follow what's happening in the code.
ConclusionIn this post, I've looked at some of the common Java red flags I see in Java code and in Java development environments. These things are not necessarily wrong or negative when used appropriately, but can be seen as alerts to potentially harmful practices when not used appropriately. When I see these red flags, I don't rush to judgment until I've had a chance to delve slightly deeper to determine if there is trouble brewing underneath these red flags or if they are in a situation in which the tactic is justifiable. More often than not, these red flag are early indicators of impending problems. In some cases, they are the explanation or pointer to serious existing and perhaps previously unexplained problems.
