Buggy Java Code: The Top 10 Most Common Mistakes That Java Developers Make

View all articles

Java is a programming language that was initially developed for interactive television, but over time it has become widespread over everywhere software can be used. Designed with the notion of object-oriented programming, abolishing the complexities of other languages such as C or C++, garbage collection, and an architecturally agnostic virtual machine, Java created a new way of programming. Moreover, it has a gentle learning curve and appears to successfully adhere to its own moto - “Write once, run everywhere”, which is almost always true; but Java problems are still present. I’ll be addressing ten Java problems that I think are the most common mistakes.

common java mistakes

Common Mistake #1: Neglecting Existing Libraries

It’s definitely a mistake for Java Developers to ignore the innumerable amount of libraries written in Java. Before reinventing the wheel, try to search for available libraries - many of them have been polished over the years of their existence and are free to use. These could be logging libraries, like logback and Log4j, or network related libraries, like Netty or Akka. Some of the libraries, such as Joda-Time, have become a de facto standard.

The following is a personal experience from one of my previous projects. The part of the code responsible for HTML escaping was written from scratch. It was working well for years, but eventually it encountered a user input which caused it to spin into an infinite loop. The user, finding the service to be unresponsive, attempted to retry with the same input. Eventually, all the CPUs on the server allocated for this application were being occupied by this infinite loop. If the author of this naive HTML escape tool had decided to use one of the well known libraries available for HTML escaping, such as HtmlEscapers from Google Guava, this probably wouldn’t have happened. At the very least, true for most popular libraries with a community behind it, the error would have been found and fixed earlier by the community for this library.

Common Mistake #2: Missing the ‘break’ Keyword in a Switch-Case Block

These Java issues can be very embarrassing, and sometimes remain undiscovered until run in production. Fallthrough behavior in switch statements is often useful; however, missing a “break” keyword when such behavior is not desired can lead to disastrous results. If you have forgotten to put a “break” in “case 0” in the code example below, the program will write “Zero” followed by “One”, since the control flow inside here will go through the entire “switch” statement until it reaches a “break”. For example:

public static void switchCasePrimer() {
    	int caseIndex = 0;
    	switch (caseIndex) {
        	case 0:
            	System.out.println("Zero");
        	case 1:
            	System.out.println("One");
            	break;
        	case 2:
            	System.out.println("Two");
            	break;
        	default:
            	System.out.println("Default");
    	}
}

In most cases, the cleaner solution would be to use polymorphism and move code with specific behaviors into separate classes. Java mistakes such as this one can be detected using static code analyzers, e.g. FindBugs and PMD.

Common Mistake #3: Forgetting to Free Resources

Every time a program opens a file or network connection, it is important for Java beginners to free the resource once you are done using it. Similar caution should be taken if any exception were to be thrown during operations on such resources. One could argue that the FileInputStream has a finalizer that invokes the close() method on a garbage collection event; however, since we can’t be sure when a garbage collection cycle will start, the input stream can consume computer resources for an indefinite period of time. In fact, there is a really useful and neat statement introduced in Java 7 particularly for this case, called try-with-resources:

private static void printFileJava7() throws IOException {
    try(FileInputStream input = new FileInputStream("file.txt")) {
        int data = input.read();
        while(data != -1){
            System.out.print((char) data);
            data = input.read();
        }
    }
}

This statement can be used with any object that implements the AutoClosable interface. It ensures that each resource is closed by the end of the statement.

Common Mistake #4: Memory Leaks

Java uses automatic memory management, and while it’s a relief to forget about allocating and freeing memory manually, it doesn’t mean that a beginning Java developer should not be aware of how memory is used in the application. Problems with memory allocations are still possible. As long as a program creates references to objects that are not needed anymore, it will not be freed. In a way, we can still call this memory leak. Memory leaks in Java can happen in various ways, but the most common reason is everlasting object references, because the garbage collector can’t remove objects from the heap while there are still references to them. One can create such a reference by defining class with a static field containing some collection of objects, and forgetting to set that static field to null after the collection is no longer needed. Static fields are considered GC roots and are never collected.

Another potential reason behind such memory leaks is a group of objects referencing each other, causing circular dependencies so that the garbage collector can’t decide whether these objects with cross-dependency references are needed or not. Another issue is leaks in non-heap memory when JNI is used.

The primitive leak example could look like the following:

final ScheduledExecutorService scheduledExecutorService = Executors.newScheduledThreadPool(1);
final Deque<BigDecimal> numbers = new LinkedBlockingDeque<>();
final BigDecimal divisor = new BigDecimal(51);

scheduledExecutorService.scheduleAtFixedRate(() -> {
	BigDecimal number = numbers.peekLast();
   	if (number != null && number.remainder(divisor).byteValue() == 0) {
     	System.out.println("Number: " + number);
		System.out.println("Deque size: " + numbers.size());
	}
}, 10, 10, TimeUnit.MILLISECONDS);

	scheduledExecutorService.scheduleAtFixedRate(() -> {
		numbers.add(new BigDecimal(System.currentTimeMillis()));
	}, 10, 10, TimeUnit.MILLISECONDS);

try {
	scheduledExecutorService.awaitTermination(1, TimeUnit.DAYS);
} catch (InterruptedException e) {
	e.printStackTrace();
}

This example creates two scheduled tasks. The first task takes the last number from a deque called “numbers” and prints the number and deque size in case the number is divisible by 51. The second task puts numbers into the deque. Both tasks are scheduled at a fixed rate, and run every 10 ms. If the code is executed, you’ll see that the size of the deque is permanently increasing. This will eventually cause the deque to be filled with objects consuming all available heap memory. To prevent this while preserving the semantics of this program, we can use a different method for taking numbers from the deque: “pollLast”. Contrary to the method “peekLast”, “pollLast” returns the element and removes it from the deque while “peekLast” only returns the last element.

To learn more about memory leaks in Java, please refer to our article that demystified this problem.

Common Mistake #5: Excessive Garbage Allocation

Excessive Garbage Allocation

Excessive garbage allocation may happen when the program creates a lot of short-lived objects. The garbage collector works continuously, removing unneeded objects from memory, which impacts applications’ performance in a negative way. One simple example:

String oneMillionHello = "";
for (int i = 0; i < 1000000; i++) {
    oneMillionHello = oneMillionHello + "Hello!";
}
System.out.println(oneMillionHello.substring(0, 6));

In Java, strings are immutable. So, on each iteration a new string is created. To address this we should use a mutable StringBuilder:

StringBuilder oneMillionHelloSB = new StringBuilder();
    for (int i = 0; i < 1000000; i++) {
        oneMillionHelloSB.append("Hello!");
    }
System.out.println(oneMillionHelloSB.toString().substring(0, 6));

While the first version requires quite a bit of time to execute, the version that uses StringBuilder produces a result in a significantly less amount of time.

Common Mistake #6: Using Null References without Need

Avoiding excessive use of null is a good practice. For example, it’s preferable to return empty arrays or collections from methods instead of nulls, since it can help prevent NullPointerException.

Consider the following method that traverses a collection obtained from another method, as shown below:

List<String> accountIds = person.getAccountIds();
for (String accountId : accountIds) {
    processAccount(accountId);
}

If getAccountIds() returns null when a person has no account, then NullPointerException will be raised. To fix this, a null-check will be needed. However, if instead of a null it returns an empty list, then NullPointerException is no longer a problem. Moreover, the code is cleaner since we don’t need to null-check the variable accountIds.

To deal with other cases when one wants to avoid nulls, different strategies may be used. One of these strategies is to use Optional type that can either be an empty object or a wrap of some value:

Optional<String> optionalString = Optional.ofNullable(nullableString);
if(optionalString.isPresent()) {
    System.out.println(optionalString.get());
}

In fact, Java 8 provides a more concise solution:

Optional<String> optionalString = Optional.ofNullable(nullableString);
optionalString.ifPresent(System.out::println);

Optional type has been a part of Java since version 8, but it has been well known for a long time in the world of functional programming. Prior to this, it was available in Google Guava for earlier versions of Java.

Common Mistake #7: Ignoring Exceptions

It is often tempting to leave exceptions unhandled. However, the best practice for beginner and experienced Java developers alike is to handle them. Exceptions are thrown on purpose, so in most cases we need to address the issues causing these exceptions. Do not overlook these events. If necessary, you can either rethrow it, show an error dialog to the user, or add a message to the log. At the very least, it should be explained why the exception has been left unhandled in order to let other developers know the reason.

selfie = person.shootASelfie();
try {
    selfie.show();
} catch (NullPointerException e) {
    // Maybe, invisible man. Who cares, anyway?
}

A clearer way of highlighting an exceptions’ insignificance is to encode this message into the exceptions’ variable name, like this:

try { selfie.delete(); } catch (NullPointerException unimportant) {  }

Common Mistake #8: Concurrent Modification Exception

This exception occurs when a collection is modified while iterating over it using methods other than those provided by the iterator object. For example, we have a list of hats and we want to remove all those that have ear flaps:

List<IHat> hats = new ArrayList<>();
hats.add(new Ushanka()); // that one has ear flaps
hats.add(new Fedora());
hats.add(new Sombrero());
for (IHat hat : hats) {
    if (hat.hasEarFlaps()) {
        hats.remove(hat);
    }
}

Concurrent Modification Exception

If we run this code, “ConcurrentModificationException” will be raised since the code modifies the collection while iterating it. The same exception may occur if one of the multiple threads working with the same list is trying to modify the collection while others iterate over it. Concurrent modification of collections in multiple threads is a natural thing, but should be treated with usual tools from the concurrent programming toolbox such as synchronization locks, special collections adopted for concurrent modification, etc. There are subtle differences to how this Java issue can be resolved in single threaded cases and multithreaded cases. Below is a brief discussion of some ways this can be handled in a single threaded scenario:

Collect objects and remove them in another loop

Collecting hats with ear flaps in a list to remove them later from within another loop is an obvious solution, but requires an additional collection for storing the hats to be removed:

List<IHat> hatsToRemove = new LinkedList<>();
for (IHat hat : hats) {
    if (hat.hasEarFlaps()) {
        hatsToRemove.add(hat);
    }
}
for (IHat hat : hatsToRemove) {
    hats.remove(hat);
}

Use Iterator.remove method

This approach is more concise, and it doesn’t need an additional collection to be created:

Iterator<IHat> hatIterator = hats.iterator();
while (hatIterator.hasNext()) {
    IHat hat = hatIterator.next();
    if (hat.hasEarFlaps()) {
        hatIterator.remove();
    }
}

Use ListIterator’s methods

Using the list iterator is appropriate when the modified collection implements List interface. Iterators that implement ListIterator interface support not only removal operations, but also add and set operations. ListIterator implements the Iterator interface so the example would look almost the same as the Iterator remove method. The only difference is the type of hat iterator, and the way we obtain that iterator with the “listIterator()” method. The snippet below shows how to replace each hat with ear flaps with sombreros using “ListIterator.remove” and “ListIterator.add” methods:

IHat sombrero = new Sombrero();
ListIterator<IHat> hatIterator = hats.listIterator();
while (hatIterator.hasNext()) {
    IHat hat = hatIterator.next();
    if (hat.hasEarFlaps()) {
        hatIterator.remove();
        hatIterator.add(sombrero);
    }
}

With ListIterator, the remove and add method calls can be replaced with a single call to set:

IHat sombrero = new Sombrero();
ListIterator<IHat> hatIterator = hats.listIterator();
while (hatIterator.hasNext()) {
    IHat hat = hatIterator.next();
    if (hat.hasEarFlaps()) {
        hatIterator.set(sombrero); // set instead of remove and add
    }
}

Use stream methods introduced in Java 8 With Java 8, programmers have the ability to transform a collection into a stream and filter that stream according to some criteria. Here is an example of how stream api could help us filter hats and avoid “ConcurrentModificationException”.

hats = hats.stream().filter((hat -> !hat.hasEarFlaps()))
        .collect(Collectors.toCollection(ArrayList::new));

The “Collectors.toCollection” method will create a new ArrayList with filtered hats. This can be a problem if the filtering condition were to be satisfied by a large number of items, resulting in a large ArrayList; thus, it should be use with care. Use List.removeIf method presented in Java 8 Another solution available in Java 8, and clearly the most concise, is the use of the “removeIf” method:

hats.removeIf(IHat::hasEarFlaps);

That’s it. Under the hood, it uses “Iterator.remove” to accomplish the behavior.

Use specialized collections

If at the very beginning we decided to use “CopyOnWriteArrayList” instead of “ArrayList”, then there would have been no problem at all, since “CopyOnWriteArrayList” provides modification methods (such as set, add, and remove) that don’t change the backing array of the collection, but rather create a new modified version of it. This allows iteration over the original version of the collection and modifications on it at the same time, without the risk of “ConcurrentModificationException”. The drawback of that collection is obvious - generation of a new collection with each modification.

There are other collections tuned for different cases, e.g. “CopyOnWriteSet” and “ConcurrentHashMap”.

Another possible mistake with concurrent collection modifications is to create a stream from a collection, and during the stream iteration, modify the backing collection. The general rule for streams is to avoid modification of the underlying collection during stream querying. The following example will show an incorrect way of handling a stream:

List<IHat> filteredHats = hats.stream().peek(hat -> {
    if (hat.hasEarFlaps()) {
        hats.remove(hat);
    }
}).collect(Collectors.toCollection(ArrayList::new));

The method peek gathers all the elements and performs the provided action on each one of them. Here, the action is attempting to remove elements from the underlying list, which is erroneous. To avoid this, try some of the methods described above.

Common Mistake #9: Breaking Contracts

Sometimes, code that is provided by the standard library or by a third-party vendor relies on rules that should be obeyed in order to make things work. For example, it could be hashCode and equals contract that when followed, makes working guaranteed for a set of collections from the Java collection framework, and for other classes that use hashCode and equals methods. Disobeying contracts isn’t the kind of error that always leads to exceptions or breaks code compilation; it’s more tricky, because sometimes it changes application behavior without any sign of danger. Erroneous code could slip into production release and cause a whole bunch of undesired effects. This can include bad UI behavior, wrong data reports, poor application performance, data loss, and more. Fortunately, these disastrous bugs don’t happen very often. I already mentioned the hashCode and equals contract. It is used in collections that rely on hashing and comparing objects, like HashMap and HashSet. Simply put, the contract contains two rules:

  • If two objects are equal, then their hash codes should be equal.
  • If two objects have the same hash code, then they may or may not be equal.

Breaking the contract’s first rule leads to problems while attempting to retrieve objects from a hashmap. The second rule signifies that objects with the same hash code aren’t necessarily equal. Let us examine the effects of breaking the first rule:

public static class Boat {
    private String name;

    Boat(String name) {
        this.name = name;
    }

    @Override
    public boolean equals(Object o) {
        if (this == o) return true;
        if (o == null || getClass() != o.getClass()) return false;

        Boat boat = (Boat) o;

        return !(name != null ? !name.equals(boat.name) : boat.name != null);
    }

    @Override
    public int hashCode() {
        return (int) (Math.random() * 5000);
    }
}

As you can see, class Boat has overridden equals and hashCode methods. However, it has broken the contract, because hashCode returns random values for the same object every time it’s called. The following code will most likely not find a boat named “Enterprise” in the hashset, despite the fact that we added that kind of boat earlier:

public static void main(String[] args) {
    Set<Boat> boats = new HashSet<>();
    boats.add(new Boat("Enterprise"));

    System.out.printf("We have a boat named 'Enterprise' : %b\n", boats.contains(new Boat("Enterprise")));
}

Another example of contract involves the finalize method. Here is a quote from the official java documentation describing its function:

”The general contract of finalize is that it is invoked if and when the JavaTM virtual machine has determined that there is no longer any means by which this object can be accessed by any thread (that has not yet died), except as a result of an action taken by the finalization of some other object or class which is ready to be finalized. The finalize method may take any action, including making this object available again to other threads; the usual purpose of finalize, however, is to perform cleanup actions before the object is irrevocably discarded. For example, the finalize method for an object that represents an input/output connection might perform explicit I/O transactions to break the connection before the object is permanently discarded.“

One could decide to use the finalize method for freeing resources like file handlers, but that would be a bad idea. This is because there’s no time guarantees on when finalize will be invoked, since it’s invoked during the garbage collection, and GC’s time is indeterminable.

Common Mistake #10: Using Raw Type Instead of a Parameterized One

Raw types, according to Java specifications, are types that are either not parametrized, or non-static members of class R that are not inherited from the superclass or superinterface of R. There were no alternatives to raw types until generic types were introduced in Java. It supports generic programming since version 1.5, and generics were undoubtedly a significant improvement. However, due to backward compatibility reasons, a pitfall has been left that could potentially break the type system. Let’s look at the following example:

List listOfNumbers = new ArrayList();
listOfNumbers.add(10);
listOfNumbers.add("Twenty");
listOfNumbers.forEach(n -> System.out.println((int) n * 2));

Here we have a list of numbers defined as a raw ArrayList. Since its type isn’t specified with type parameter, we can add any object into it. But in the last line we cast elements to int, double it, and print the doubled number to standard output. This code will compile without errors, but once running it will raise a runtime exception because we attempted to cast a string to an integer. Obviously, the type system is unable to help us write safe code if we hide necessary information from it. To fix the problem we need to specify the type of objects we’re going to store in the collection:

List<Integer> listOfNumbers = new ArrayList<>();

listOfNumbers.add(10);
listOfNumbers.add("Twenty");

listOfNumbers.forEach(n -> System.out.println((int) n * 2));

The only difference from the original is the line defining the collection:

List<Integer> listOfNumbers = new ArrayList<>();

The fixed code wouldn’t compile because we are trying to add a string into a collection that is expected to store integers only. The compiler will show an error and point at the line where we are trying to add the string “Twenty” to the list. It’s always a good idea to parametrize generic types. That way, the compiler is able to make all possible type checks, and the chances of runtime exceptions caused by type system inconsistencies are minimized.

Conclusion

Java as a platform simplifies many things in software development, relying both on sophisticated JVM and the language itself. However, its features, like removing manual memory management or decent OOP tools, don’t eliminate all the problems and issues a regular Java developer faces. As always, knowledge, practice and Java tutorials like this are the best means to avoid and address application errors - so know your libraries, read java, read JVM documentation, and write programs. Don’t forget about static code analyzers either, as they could point to the actual bugs and highlight potential bugs.

About the author

Mikhail Selivanov, Russia
member since December 19, 2012
Mikhail has extensive experience working as a back-end programmer and has completed numerous successful projects. He has been responsible for every part of the development process, including the implementation of business logic, performance tuning, writing deployment scripts, and more. [click to continue...]
Hiring? Meet the Top 10 Freelance Java Developers for Hire in September 2016

Comments

Lê Anh Quân
Great article, very detailed and informative. About item #7, I just want to ask your opinion about Java's checked exceptions, are they really necessary? Personally I think all exceptions should be unchecked ( Runtime exceptions ). Try-catch is good, but forcing developers to escalated code blocks is a really bad idea, and overall, it does more harm than good. How do you think?
Preda
Yes they are necessary because you want to capture possible code breaks ideally at compile time, and react to them, by capturing the exception and doing something that makes sense in that event. Rather than handling all exceptions at runtime, it's better to catch them earlier.
Lê Anh Quân
It's great if all checked exceptions are "possible code break", but many times they aren't. Think about writing a method to calculate days until Valentine using parse with "2/14" and SimpleDateFormat. You will have to handle ParseException even though you know it will never happen. Or catching IOException every time you do something with a ByteArrayOutputstream... It's painful to have your code poluted with unnecessary try catch
Preda
While that might be true. Proceeding with precaution now always pays dividends later, and I think the language designers had this in mind. However you can always use ruby, and don't have to do this :)
Lê Anh Quân
Dear Preda, I think you are terribly wrong: 1. About precaution: it is good, but please don't force. I bet your program will be much more robust if NullPointerException is a checked one, but you'll soon find out it's a horrible idea. 2. Language designers are not gods, they can be wrong. They didn't have generics or lambdas in mind in the first place. In this case of checked exception, they are wrong again. 3. Ruby? Really? What would I do if later I find out Ruby doesn't have static type? Change to dot net?
Chuck Batson
FindBugs (http://findbugs.sourceforge.net) is an invaluable tool and identifies many common mistakes. I would add that Common Mistake #11 is not using FindBugs. :-)
Mikhail Selivanov
Thanks, I'm glad that you liked the article. Regarding exceptions, I think in many cases it's a good idea to encode operation error into the result value. Forcing developers to handle errors is a nice feature for designing API, but it shouldn't be overused.
Josip Pokrajcic
Regarding mistake #3 you said that the program will write “Zero” followed by “One”. It should wrote "Zero" followed by "Default" if I'm not mistaken.
stingersdestiny
I think Number 7 needs further explanation. Its one thing to catch an exception and not do anything and completely another whether one should catch NPE (or other runtimeexcepions). In my opinion its extremely rare to justiy catching an NPE. It is bad code. Your code should not be returning null and at the very least should be verifying before its usage. Programmers should let NPE be thrown and then investigate it instead of catching it
Peter Storch
Be carefull with #1: In principle you are right about not inventing the wheel. But I've seen projects having dependencies to 3 XML, 4 Logging and 5 JSON Libraries. And often enough introducing one library adds a dependency to 10 others.
Carlos De Luna Saenz
I would like to add a couple more: 11th: Use OF Java like a Structured language instead a OOP language (it's weird and awfull when you get stock reviewing "spaguetti code", for example... and 12th: Lack of use of Design Patterns: Design patterns were made to make life easier and most of the "well known" frameworks use them and allows you to use them (such Spring MVC or Hibernate for DAO pattern)... then do it. Congratulations for an excelent article.
Mikhail Selivanov
I'm agree about NPE and there is a #6 which is about how to avoid it by not using null references.
Mikhail Selivanov
It's not very clever to use 5 libraries that do the same thing, just stick to one of them. However, there is another issue with third-party libraries. Each time you add a dependency to you project, there is a possibility that it will pull a half of the repository of it's own dependencies.
Mikhail Selivanov
Thank you for the kind words. Agree, knowledge of OOP and design patterns is an important thing, not only for Java programmers.
ncmathsadist
Disagree. Uncaught exceptions can cause unexpected crashes in programs for end users. This is a big-time no-no. You do not repay your customers with death. Runtimes exceptions should be reserved for programmer goofs. Even then, if user abuse causes a run time exception (think NumberFormatException from user entry in a dialog box), that exception should be caught and handled gracefully. Obviously, you should never duck fileIO and socket exceptions. For the most part, Java's exception rules make sense and prevent production software from crashing.
Lê Anh Quân
It is true... in theory. Somehow due to my 10+ years Java experience, the checked exceptions are the most frustrating thing: - It slows me down, force me to handle exceptions when I'm not ready to (have to focus on the logic the code mainly about) thus make me either handle them wrongly or throw a wrapping RTE. - "throws" exceptions are often not an option - due to API contracts. - Very hard to centralize exception handling with checked exceptions. Solutions often come with heavy weighted framework which then produce more troubles than it solves. - The in-line try-catch blocks are most horrible nightmare. I am OKAY with try-catch if catching exception is the only thing the method is about, but few in-line try-catch in a method... simply destroy the code and the software. Anyway, maybe you are right, maybe I'm too bent toward the elegant looking code like functional style and became too much emotional in this matter, but Java doesn't need to mean ugly looking code. Right?
lucas rafagnin
Thanks guy!
tfa
Are there still developers who think checked exceptions were are good idea? Come on!
Madonah
Helped me a lot. Java is cool, hope you write also about C++ and C#. Thank you.
Anand Kumar
visit for lot more java interview questions and programs - http://javadiscover.blogspot.com
Ricardo Santos
Also about item #7. When logging an exception you should include details about the context of the exception and also include the exception in order not to lose the stacktrace. On my past years I've seen many of log.error(ex.getMessage()) instead of log.error("Error reading file '" + path + "'", ex).
comments powered by Disqus
Subscribe
The #1 Blog for Engineers
Get the latest content first.
No spam. Just great engineering and design posts.
The #1 Blog for Engineers
Get the latest content first.
Thank you for subscribing!
You can edit your subscription preferences here.
Trending articles
Relevant technologies
About the author
Mikhail Selivanov
Java Developer
Mikhail has extensive experience working as a back-end programmer and has completed numerous successful projects. He has been responsible for every part of the development process, including the implementation of business logic, performance tuning, writing deployment scripts, and more.