Typical Mistakes in Java Code
This page contains most typical mistakes I see in the Java code of people working with me. Static analysis (we’re using qulice can’t catch all of the mistakes for obvious reasons, and that’s why I decided to list them all here.
Let me know if you want to see something else added here, and I’ll be happy to oblige.
All of the listed mistakes are related to object-oriented programming in general and to Java in particular.
Class Names
Read this short “What is an Object?” article. Your class should be an abstraction of a real life entity with no “validators”, “controllers”, “managers”, etc. If your class name ends with an “-er” — it’s a bad design.
And, of course, utility classes are anti-patterns, like StringUtils
, FileUtils
, and IOUtils
from Apache. The above are perfect examples of terrible designs. Read this follow up post: OOP Alternative to Utility Classes.
Of course, never add suffixes or prefixes to distinguish between interfaces and classes. For example, all of these names are terribly wrong: IRecord
, IfaceEmployee
, or RecordInterface
. Usually, interface name is the name of a real-life entity, while class name should explain its implementation details. If there is nothing specific to say about an implementation, name it Default,
Simple
, or something similar. For example:
class SimpleUser implements User {}; class DefaultRecord implements Record {}; class Suffixed implements Name {}; class Validated implements Content {};
Method Names
Methods can either return something or return void
. If a method returns something, then its name should explain what it returns, for example (don’t use the get
prefix ever):
boolean isValid(String name); String content(); int ageOf(File file);
If it returns void,
then its name should explain what it does. For example:
void save(File file); void process(Work work); void append(File file, String line);
There is only one exception to the rule just mentioned — test methods for JUnit. They are explained below.
Test Method Names
Method names in JUnit tests should be created as English sentences without spaces. It’s easier to explain by example:
/** * HttpRequest can return its content in Unicode. * @throws Exception If test fails */ public void returnsItsContentInUnicode() throws Exception { }
It’s important to start the first sentence of your JavaDoc with the name of the class you’re testing followed by can
. So, your first sentence should always be similar to “somebody can do something”.
The method name will state exactly the same, but without the subject. If I add a subject at the beginning of the method name, I should get a complete English sentence, as in above example: “HttpRequest returns its content in unicode”.
Pay attention that the test method doesn’t start with can
.Only JavaDoc comments start with ‘can.’ Additionally, method names shouldn’t start with a verb.
It’s a good practice to always declare test methods as throwing Exception
.
Variable Names
Avoid composite names of variables, like timeOfDay
, firstItem
, or httpRequest
. I mean with both — class variables and in-method ones. A variable name should be long enough to avoid ambiguity in its scope of visibility, but not too long if possible. A name should be a noun in singular or plural form, or an appropriate abbreviation. For example:
List<String> names; void sendThroughProxy(File file, Protocol proto); private File content; public HttpRequest request;
Sometimes, you may have collisions between constructor parameters and in-class properties if the constructor saves incoming data in an instantiated object. In this case, I recommend to create abbreviations by removing vowels (see how USPS abbreviates street names).
Another example:
public class Message { private String recipient; public Message(String rcpt) { this.recipient = rcpt; } }
In many cases, the best hint for a name of a variable can ascertained by reading its class name. Just write it with a small letter, and you should be good:
File file; User user; Branch branch;
However, never do the same for primitive types, like Integer number
or String string
.
You can also use an adjective, when there are multiple variables with different characteristics. For instance:
String contact(String left, String right);
Constructors
Without exceptions, there should be only one constructor that stores data in object variables. All other constructors should call this one with different arguments. For example:
public class Server { private String address; public Server(String uri) { this.address = uri; } public Server(URI uri) { this(uri.toString()); } }
One-time Variables
Avoid one-time variables at all costs. By “one-time” I mean variables that are used only once. Like in this example:
String name = "data.txt"; return new File(name);
This above variable is used only once and the code should be refactored to:
return new File("data.txt");
Sometimes, in very rare cases — mostly because of better formatting — one-time variables may be used. Nevertheless, try to avoid such situations at all costs.
Exceptions
Needless to say, you should never swallow exceptions, but rather let them bubble up as high as possible. Private methods should always let checked exceptions go out.
Never use exceptions for flow control. For example this code is wrong:
int size; try { size = this.fileSize(); } catch (IOException ex) { size = 0; }
Seriously, what if that IOException
says “disk is full”? Will you still assume that the size of the file is zero and move on?
Indentation
For indentation, the main rule is that a bracket should either end a line or be closed on the same line (reverse rule applies to a closing bracket). For example, the following is not correct because the first bracket is not closed on the same line and there are symbols after it. The second bracket is also in trouble because there are symbols in front of it and it is not opened on the same line:
final File file = new File(directory, "file.txt");
Correct indentation should look like:
StringUtils.join( Arrays.asList( "first line", "second line", StringUtils.join( Arrays.asList("a", "b") ) ), "separator" );
The second important rule of indentation says that you should put as much as possible on one line – within the limit of 80 characters. The example above is not valid since it can be compacted:
StringUtils.join( Arrays.asList( "first line", "second line", StringUtils.join(Arrays.asList("a", "b")) ), "separator" );
Redundant Constants
Class constants should be used when you want to share information between class methods, and this information is a characteristic (!) of your class. Don’t use constants as a replacement of string or numeric literals — very bad practice that leads to code pollution. Constants (as with any object in OOP) should have a meaning in a real world. What meaning do these constants have in the real world:
class Document { private static final String D_LETTER = "D"; // bad practice private static final String EXTENSION = ".doc"; // good practice }
Another typical mistake is to use constants in unit tests to avoid duplicate string/numeric literals in test methods. Don’t do this! Every test method should work with its own set of input values.
Use new texts and numbers in every new test method. They are independent. So, why do they have to share the same input constants?
Test Data Coupling
This is an example of data coupling in a test method:
User user = new User("Jeff"); // maybe some other code here MatcherAssert.assertThat(user.name(), Matchers.equalTo("Jeff"));
On the last line, we couple "Jeff"
with the same string literal from the first line. If, a few months later, someone wants to change the value on the third line, he/she has to spend extra time finding where else "Jeff"
is used in the same method.
To avoid this data coupling, you should introduce a variable.
Related Posts
You may also find these posts interesting:
- Why NULL is Bad?
- Objects Should Be Immutable
- OOP Alternative to Utility Classes
- Avoid String Concatenation
- Simple Java SSH Client
Reference: | Typical Mistakes in Java Code from our JCG partner Yegor Bugayenko at the About Programming blog. |
So to hell with using beans (get-prefixes) and using names that are meaningful, right? In the healthcare industry, companies are striving for clarity. Part of that endeavor is to remove abbreviations from everything (documents, websites, etc). I think that programming is far beyond the need for abbreviations, in most cases. They are a relic of 80 character vim/emacs windows and hurt more than help. I especially think that abbreviations in method parameters are just plain awful design. The abbreviation rcpt does not clearly state what it is in a quick glance. Not many developers are going, nor will they want,… Read more »
I hate it if getters/setters are missing the get/set/is/has prefix. Additionally i prefer object members to be prefixed with a ‘m’ to be able to distinguish them from method params (that are prefixed with a ‘p’) and local variables – especially when working in a simple text editor…
Read this follow up article, about getters and setters: http://www.yegor256.com/2014/09/16/getters-and-setters-are-evil.html
Well, I do like some of your proposals, but don’t agree on others. On which is this based, besides your opinion?
Well, mostly on the “Object Thinking” book by David West. Highly recommend to read it. Read my recent article about Getters and Setters, it mentions some anti-OOP misconceptions we have: http://www.yegor256.com/2014/09/16/getters-and-setters-are-evil.html
Yannick, every particular rule has its own roots and reasons. Which one are you most interested in?
MY coding conventions are the BEST coding conventions, and if you don’t do things MY way, OBVIOUSLY you’ve made a MISTAKE.
It’s good start for a dialog :) Drop me a message at me@yegor256.com and we’ll discuss in details. I will be happy to change my opinion if you convince me.
It’s about the attitude more than the positions. Claiming people are WRONG on subjects of style and preference is, for many people, arrogant and annoying. Even when people agree with you, it’s offputting and we don’t want to associate with you, and it’s not a good start for a dialog, since people who disagree with you will dismiss you as arrogant and annoying, and why would we want to have a dialog with someone like that?
Yegor,
I’d like to thank you for writing up this article,
Please do not allow negative people to discourage you from sharing your thoughts and knowledge.
I’m working with multiple programmers every day, leading them and managing. So, I’m used to negativity. In time, the most negative opponents turn into the most active followers :) And thanks for cheering up :)
I totally disagree with all of your proposals.
Francois
Oracle Certified Java Architect & Oracle Certified Java Developer
I totally disagree with all of your proposals.
Dean
Oracle certified Java Architect
Oracle certified Java Developer
A typical example showing that the following point is not credible : You said : Usually, interface name is the name of a real-life entity, while class name should explain its implementation details. Example : User as an interface, SimpleUser as implementation. Now, read other chapters of your article : none of them respect this rule. I really disagree with you when you want to get rid of prefixes. I find them as important as the context word. A get returns a value, a set sets a value. Nothing is an action. Something which is “able” says exactly what it… Read more »
The article doesn’t say that, but a proper object in an ideal OOP world doesn’t have “setters”. It only has behavior. An object is NOT a container of data. It is a living creature, like you and me. I can’t SET anything into you. I can give you a glass of water, but it doesn’t mean that “I set a glass of water into you”, right?
Check these two articles: http://www.yegor256.com/2014/05/13/why-null-is-bad.html and http://www.yegor256.com/2014/06/09/objects-should-be-immutable.html
Hi,
I cannot open most of the related articles – I get “Nothing found for ….” messaeg in teh title.
May you provide valid links for:
Why NULL is Bad?
Objects Should Be Immutable
OOP Alternative to Utility Classes
Avoid String Concatenation
Best regards,
NullPointerException
Why NULL is Bad: http://www.yegor256.com/2014/05/13/why-null-is-bad.html
Objects Should be Immutable: http://www.yegor256.com/2014/06/09/objects-should-be-immutable.html
OOP Alternative to Utility Classes: http://www.yegor256.com/2014/05/05/oop-alternative-to-utility-classes.html
Avoid String Concatenation: http://www.yegor256.com/2014/06/19/avoid-string-concatenation.html
All of them are from my blog: http://www.yegor256.com :)
Sorry dude, but these are some of the worst advices I’ve ever seen. And you call yourself CTO, seriously? You probably never had to work in large environments with high load and never had to debug code in real life situations. What are the rationals behind those advices? You cannot just claim that something is bad practice without explaining why. This article should be deleted. “Your class should be an abstraction of a real life entity with no validators, controllers, managers” -> validator and controller are very specific nouns, disallowing them is a total overstretch. Ever heard about bean validation… Read more »
Pointing out advantages of a personal coding style-guide is okay. But marking every alternate way as a “typical mistake in java code” is outright stupid from someone calling himself an “architect”.
I strongly believe that a job of an architect is to say what is right and what is wrong. If one way is right, the other way is wrong. You may disagree with my point of view, but I’m not going to accept what I think is wrong just for sake of being nice. Sounds logical?
No, sounds arrogant, actually. Logic is not cold, just as the sword is not a piece of steel in the hands of someone that actually uses it properly ;) . What you are doing is not describing a logical path, but a node of a tree as “the correct one”. Obviously, you do not want to debate what you have written, which defeats the purpose of the article itself. But more on that later. There CAN certainly be two or even more completely correct ways of doing things. Moreover, some of the “mistakes” you list are in fact not: –… Read more »
Check this article about getters and setters: http://www.yegor256.com/2014/09/16/getters-and-setters-are-evil.html
Check this one about indentation, static analysis and quality in general: http://www.yegor256.com/2014/08/13/strict-code-quality-control.html
And I would really love to discuss further, but the format of the site is not really convenient for it. I’ll try to answer all your questions in follow up posts in my blog and will share the links :)
Great to know you reply to comments. Thanks for that. Whatever you call them, methods that give you access to an object’s state are getters/setters. You may use different semantics to describe them in text/speech, but the final result is the same. Your dog example in the blog article is cute. You renamed the methods, but their behaviour is the same. Reference back to my question about test method names reasoning. Why not take a ball and throw it into the environment, and let the dog decide whether it wants to catch it? Maybe make a human object to throw… Read more »
Check this article about getters/setters and semantic: http://www.yegor256.com/2014/09/16/getters-and-setters-are-evil.html. I believe (and the article explains why) that it’s not about semantic. It’s about “anthropomorphizing” of objects :) Simply put, do we treat them as living creatures or as dead bags of data items. I believe that OOP is about living objects. Check this article about NULL references: http://www.yegor256.com/2014/05/13/why-null-is-bad.html. It explains why NULL reference is not in spirit of OOP. Again, the same metaphor, if an object is a living organism, than what NULL is? Visual representation of the code demonstrates a mindset of its author, I believe. It’s like a discipline.… Read more »