Ten Java coding antipatterns to avoid: Worst practices #10 through #6

You should avoid these worst practices—and fix them when you maintain or refactor existing code.

With experience, everyone gains ideas of good and bad practice, and that applies to both coding and code reviews. In her article “Five code review antipatterns,” fellow Java Champion Trisha Gee pointed out several worst practices for the code review process. I’d like to point out 10 antipatterns for the coding process itself; half are in this article, and the worst offenders are in the next article, to be published in Java Magazine soon.

To be clear, you should avoid these worst practices—and eliminate them when you maintain or refactor existing code. And, of course, resolve them if you see these issues during a code review.

Worst practice #10: Import messes

The list of imported classes and methods at the top of a class is intended to be a reference to the API that it is using. Imports ending in * convey little specific information and, even worse, unused imports are misleading. Imports in a quasi-random order take longer to read, which is a pain for maintenance.

A better way: Let your IDE maintain the imports. The Eclipse IDE has really good support for this: Its “organize imports” feature will, with one click, remove unused imports, add any missing imports, and sort the list into a consistent order, with java classes first, then javax, then third-party classes, and then static imports. You can get all that in IntelliJ IDEA, but you must tweak three or four settings to get there.

True confession: When I was a young and foolish tech lead on a large app project which shall remain nameless, I once set the messaging level for unused imports from Warning to Error in the Eclipse settings and committed this to the project repository. Of course, I did this worst practice only after lecturing and hectoring the development team didn’t work. This was part of my plan to keep imports organized across the entire project. Changing this setting wasn’t popular, but the few opposing developers came around after seeing how easy it was to fix (using Ctrl+Shift+O) and how this change made the long list of imports on that project much easier to read.

Worst practice #9: Inconsistent indentation

The indentation-champion language is surely Python, which uses indentation instead of braces or keywords to denote the body of a control flow or method. Thus, if Python code is indented incorrectly it won’t compile!

Fortunately, Java (like the other C-family languages) uses braces for block structure and ignores whitespace. That said, consistent indentation still matters. While indents are not required by the compiler, they are required for the human reader. Consider the following code:

if (condition)

    statement1;

    statement2;

statement3;

Upon a quick read, it appears as though statements 1 and 2 are controlled by the if. However, statement 2 is not, because this is Java, not Python.

Or consider the following code:

statement1;

   statement2;

 statement3;

What was the programmer thinking? The code looks like something spewed by a waterfall on a windy day. The statements have the same level of control flow, so they should all begin in the same column. Again, modern IDEs can repair this damage in no time flat with a feature such as “Fix Indentation.”

Select an entire file with Ctrl+A or Cmd+A, or select one method by selecting it with the mouse. Then choose the indentation repair from the Edit or Code menu. Problem solved!

Worst practice #8: JAR files without links

When Java first arrived, it appeared that it would solve one of Windows developers’ nightmares: the oft-cursed “DLL Hell,” where a mixture of different shared objects (such as .dll files in Windows or .so files on other platforms) contain version conflicts.

Unfortunately, the problem wasn’t solved. That’s part of what the Java Platform Module System (JPMS) was intended to address. Tools such as Maven and Gradle have been helping with this issue for years—but sometimes JAR files without links still appear.

The worst case I’ve run across is a project with a folder of files that were named something like the following:

util.jar

system.jar

financial.jar

report.jar

The files had dates about 10 years old. Each of the four projects had been updated by their maintainers during that time, but there was no record of what version of the library JAR files was depended upon by the main application—unless you considered “the JAR files that happen to be in the lib folder” to be a form of documentation.

Some of the JAR files were from third-party APIs (whose names have been changed to protect the guilty) that had multiple news-making security issues over the years, yet none of the developers on the team seemed concerned enough to move to versioned JAR files—maybe because they didn’t know if they were using the affected versions.

I admit that I may have created some projects like this many years ago—but I have taken the pledge to avoid them.

Today all my projects are managed by Maven or Gradle, each of which takes a specification of each dependency’s group (usually the organization), artifact (the JAR name), and a version number and will fetch the matching JAR file. That file will have the artifact name and version number in the filename. For example, a project might have the following in its Maven configuration file (pom.xml):

<dependency>

    <groupId>com.darwinsys</groupId>

    <artifactId>darwinsys-api</artifactId>

    <version>1.7.5</version>

</dependency>

This code in pom.xml directs Maven to download the darwinsys-api-1.7.5.jar file and store it (along with some metadata files) in a carefully constructed tree in my home directory (which is ~/.m2/repository/com/darwinsys/darwinsys-api/1.7.5). In this way, when two or more projects require the same JAR file, the JAR will be downloaded only once.

Here is a very selective look at the Maven local repository on one of my systems.

$ ls ~/.m2/repository

aopalliance biz bouncycastle cglib com dev eclipse edu info io

jakarta javax jaxen jline log4j ...

$ ls ~/.m2/repository/com/darwinsys/darwinsys-api

1.5.14

1.5.15

1.7.5

maven-metadata-central.xml

maven-metadata-central.xml.sha1

resolver-status.properties

$ ls ~/.m2/repository/com/darwinsys/darwinsys-api/1.7.5

_remote.repositories

darwinsys-api-1.7.5.jar

darwinsys-api-1.7.5.jar.sha1

darwinsys-api-1.7.5.pom

darwinsys-api-1.7.5.pom.sha1

$

By looking at the pom.xml file, not only is it clear which version of the API is used in that particular project, it’s also clear (at least if you know what the default is and that there is no other repository listed in the pom.xml file) that the JAR file came from the centralized Maven repository, Maven Central.

What’s more, the JAR file itself has its version number embedded in its filename.

Maven uses the Secure Hash Algorithm (sha) files to ensure that the JAR file hasn’t been tampered with. If you run the build tool in debug mode, you will see an extremely verbose output that includes the full path of each JAR file that is on the classpath. Plus, Maven has capabilities such as mvn dependency:tree to show all the sub- and sub-sub-dependencies in a tree format.

Keeping JAR dependencies under control is part of making software development a discipline. Make it so!

Worst practice #7: Meaningless names

Now is a good time to quote Ian’s First Rule of Coding:

You should never type more than a few characters of any name except when you’re creating it.

Given that most developers (except for two or three vi diehards) use a full-featured IDE these days, and since all major IDEs have really good code completion features, there’s no reason to type out long names.

But neither is there any reason to avoid giving meaningful names to methods, fields, classes, variables, and other elements.

Variable names such as i, j, and k are, in my book, allowed only when you’re using the old-style for loop to index an array or count something. Also allowed are names such as s for a locally used String, in the header of a few-lines-long method, or when you’re writing a lambda that is short and self-contained.

For everything else, pick a useful name.

This becomes particularly important where the var keyword is used to avoid having to give type declarations. Why? The variable name may be the only clue the reader has as to what you mean the variable to be used for. Consider the following example:

for (int i = 0; i < functionData.length; i++) {

        functionData[i] = someFunction(i);

}

customerNames.forEach(s->s.substring(1)); // "s" OK here

int bodyFontSize = 11;

I’m not only talking about variables: Method names should also be meaningful. In writing JUnit tests, you’ll find that names like test1() and test2() and so on are not only useless: They mislead, because such naming implies an ordering that isn’t there.

JUnit does not make any claim to run methods in the order in which you wrote them. Methods are, in fact, run in the order given by the reflection API, which is documented to return members that “are not sorted and are not in any particular order.”

Here is an example of this antipattern.

@Test

    public void test1() {   // Bad

        // test here...

    }

And here is a better way to write it.

@Test

    public void testPositiveResultsCorrect() { // Better

        // test here...

    }

Remember: You are one of the people most likely to need to read this code months or years after you wrote it, so be kind to yourself!

Worst practice #6: Reinventing the flat tire

This antipattern’s title is from a paper I worked on many years ago. “Reinventing the wheel” is a common English-language idiom for designing and creating something that already exists. My then-colleague Geoff Collyer and I took the expression one step further, in a C coding style paper Geoff and I co-wrote long ago in a galaxy far away. “Reinventing the flat tire” meant that a programmer not only wrote code whose functionality was readily available in a standard or common library, but that the new code did a worse job than the public API.

Here’s an example of this antipattern.

String[] candidates = getStrings();

String searchingFor = "The Lost Boys";

int found = -1;

for (int i = 0; i < candidates.length; i++) { // flat tire

    if (candidates[i].equals(searchingFor)) {

        found = i;

    }

}

And here is a better way.

Arrays.sort(candidates); // start of "better" approach

found = Arrays.binarySearch(candidates, searchingFor);

You might think the second approach would run more slowly, because a binary search requires the input be sorted. That’s true. But notice that the programmer of the antipattern forgot to break out of the loop when finding the match, so that code’s efficiency is terrible anyway.

Reinventing public APIs is nothing new and is often a sign of incomplete knowledge of the API. Of course, it’s easy enough to make that error when languages have such a vast standard library as Java has.

Here’s an example of reinventing an API you might or might not know; this has been in the platform since Java 1.7.

var x = getValue();     // legacy way

if (x == null) {

    x = getSomeDefaultValue();

}

System.out.println(x);

Here’s the better, shorter way.

var y = Objects.requireNonNullElse(getValue(), getSomeDefaultValue());

System.out.println(y);

The first example’s programmer could have used the standard Objects.requireNonNullElse() library routine, which has a variety of overloads that will help reduce coding for some common operations.

Source: oracle.com