Every so often, you see code that someone else has written—or code that you wrote—and smack your head in wonder, disbelief, and dismay.
I’ll reiterate what I wrote in the previous article’s introduction: 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 #5: Duplicating code
Many developers are taught early on that copy-and-paste is a bad idea. Literally copying code from elsewhere in an application is bad because it creates a maintenance nightmare: Finding a bug or changing the functionality requires that you find all the copies and fix them all. Copies are also bad because they make a program needlessly larger.
Many IDEs have “extract method” or “introduce method” refactoring functions that take existing code and turn it into a new Java method. If you create a method instead of copying and pasting, your code will be shorter, clearer, and cleaner, as well as easier to debug and maintain.
CPD, the copy-and-paste detector from the PMD Open Source Project, is a useful tool for finding where copy-and-paste has been applied. It uses a clever algorithm to find duplicated tokens, and by default it looks for a run of 100 or more tokens, most of which must be identical to be declared a copy. A token is an element such as a keyword, literal, operator, separator, or identifier.
CPD is distributed as part of
PMD, which is an extensible cross-language static code analyzer.
One of my
open source GitHub repositories contains all the code examples from my
Java Cookbook plus many other code samples. Unfortunately, some of the examples not used in the book do not get the regular maintenance they deserve.
(In my defense, sometimes a developer does copy a code example for legitimate reasons that wouldn’t apply when building a real application.)
While writing this article, I ran CPD against my repository, and it found several issues. Here are two.
$ cpd
Found a 14 line (184 tokens) duplication in the following files:
Starting at line 19 of /home/ian/git/javasrc/desktop/src/main/java/gui/FlowLayoutSimple.java
Starting at line 37 of /home/ian/git/javasrc/desktop/src/main/java/gui/FlowLayoutSimple.java
getContentPane().add(quitButton = new JButton("Stop"));
getContentPane().add(quitButton = new JButton("Exit"));
getContentPane().add(quitButton = new JButton("Exit"));
getContentPane().add(quitButton = new JButton("Exit"));
getContentPane().add(quitButton = new JButton("Exit"));
getContentPane().add(quitButton = new JButton("Exit"));
getContentPane().add(quitButton = new JButton("Exit"));
getContentPane().add(quitButton = new JButton("Exit"));
getContentPane().add(quitButton = new JButton("Exit"));
getContentPane().add(quitButton = new JButton("Exit"));
getContentPane().add(quitButton = new JButton("Exit"));
getContentPane().add(quitButton = new JButton("Exit"));
getContentPane().add(quitButton = new JButton("Exit"));
getContentPane().add(quitButton = new JButton("Exit"));
The first one is interesting. It is obviously an editing error; when you use the vi editor, a number followed by an insert causes the insertion of that number of copies of the insert. However, numbers followed by the letter G (for go) are used to jump to a line by number.
My guess is that I typed a number to jump to a line, forgot the G, and typed a line to be inserted at that location, causing the line to be erroneously inserted many times. Strangely, this mistake has been in my public repository since 2003, and nobody has ever reported it to me.
The second issue identified an 18-line (184 tokens) duplication in the following files:
Starting at line 28 of /home/ian/git/javasrc/main/src/main/java/regex/LogRegEx.java
Starting at line 25 of /home/ian/git/javasrc/main/src/main/java/regex/LogRegExp.java
System.out.println("Input line:" + logEntryLine);
Matcher matcher = p.matcher(logEntryLine);
if (!matcher.matches() ||
LogParseInfo.MIN_FIELDS > matcher.groupCount()) {
System.err.println("Bad log entry (or problem with regex):");
System.err.println(logEntryLine);
return;
}
System.out.println("IP Address: " + matcher.group(1));
System.out.println("UserName: " + matcher.group(3));
System.out.println("Date/Time: " + matcher.group(4));
System.out.println("Request: " + matcher.group(5));
System.out.println("Response: " + matcher.group(6));
System.out.println("Bytes Sent: " + matcher.group(7));
if (!matcher.group(8).equals("-"))
System.out.println("Referer: " + matcher.group(8));
System.out.println("User-Agent: " + matcher.group(9));
});
The same program demonstrated the use of regular expressions to parse the common Apache Log File format, and it seems as if I somehow accidentally created the same file with two different names, perhaps while merging files into this repository from another.
Here I am, rightfully busted by a tool that I often recommend. I shall have to use CPD more often.
Worst practice #4: Out-of-date Javadoc
Javadoc is your friend—but having friends takes work. To be able to read documentation and apply it usefully, it must be up to date. Therefore, when you change the arguments to a function, for example, you need to change the Javadoc accordingly. Don’t be the developer responsible for the following:
/**
* Perform the aFunction function.
* @parameter x The X coordinate to start
* @parameter y The Y coordinate to start
* @Parameter len The number of points to process
*/
public double aFunction(double x, double y, double endX, double endY) {
Your Javadoc can be more useful if it is generated in formats such as HTML, for reference. Maven and Gradle and other build tools have plugins that make it easy to generate Javadoc web pages as part of your overall build process. In Maven it may take 10 or 15 lines of plugin configuration to tame Javadoc, but once that’s written, that configuration rarely changes.
You may want to include the following configuration element when getting started:
<failOnError>false</failOnError>
By the way, old and sporadically maintained Javadoc will otherwise fail the build completely. This gives you time to clean up the documentation incrementally and get it into a condition you’ll be proud to show to other developers.
Worst practice #3: Unvalidated user input
In 1979, Brian Kernighan and P.J. Plauger wrote a book called
The Elements of Programming Style. Although it was written with some older programming languages for the examples, Kernighan and Plauger’s book contains much developer advice that is truly timeless. One of my favorite idioms is
Never trust user input.
Well, they actually wrote, “Test input for plausibility and validity,” but I like my formulation better.
When reading code in which someone has written JDBC calls, it is not uncommon to find this antipattern in the first statement.
rs = jdbcConnection.createStatement().executeQuery( // bad
"select * from customers where name = '" + nameField.getText() + "';");
PreparedStatement statement = jdbcConnection.prepareStatement(
"select * from customers where name = ?1"); // better
statement.setString(1, nameField.getText());
rs = statement.executeQuery();
The value of nameField.getText() is almost certainly coming straight from the user; that is, it is user input that should never be trusted. However, this data is being fed directly into the database.
John Smith'; drop table customers; --
It will be as though you had entered the following SQL:
"select * from customers where name = 'John Smith'; drop table customers; --';"
Many, if not most, JDBC drivers allow more than one statement on a line, with a semicolon (;) between each. Now what if the database architect was as careless as the developer? By not restricting the database account used by the app server from having drop privileges, it’s game over.
The -- at the end is the twist of the knife because it will stop the leftover delimiter characters from even causing a syntax error in the log file, obfuscating where the vandalism occurred.
Java’s
PreparedStatement interface obviates this problem: This interface will treat the entire string (whether it’s normal or malicious) as characters to match in the where clause, and if the input is bad, it will fail safely.
SQL injection attacks such as this happen probably every day on small sites, so much so that they have been in the Open Web Application Security Project’s notorious
OWASP Top 10 list since its inception.
Worst practice #2: Not testing the not-unit-testable
I dread walking into an old-school project that lacks unit tests.
Many older applications were written in a single class, sometimes termed a ball of wax or all-in-one class or even a monster class. (There are even less-polite names.) It is difficult to write unit tests for monster classes because unit tests, by definition, are designed to test one small unit of code. A monster class has no small units of code to test! Not only are there no tests, but the code is also not written to be testable.
If you are tasked with maintaining such an application, start carving the monster into smaller pieces that can be tested. How big or small should the code classes be? That’s a topic for endless debate, as there is no magic size and no exact number of lines of code for classes or for methods.
The single-responsibility principle (SRP) says that each class should have one primary responsibility: performing some calculations, processing an order, or displaying the results. In other words, if your application does those three things, you need at least three classes. Similarly, SRP says that a method should do one thing, and one thing only.
While you extract code out of the monolith, write unit tests—and make sure they pass.
Of course, if you’re starting a project from scratch, you can have the benefit of writing the tests as you write the code. Writing the tests first—that is, following the
test-driven development (TDD) methodology—allows the IDE to generate the outline of the class and methods being tested, guaranteeing that they are in sync from the beginning.
Worst practice #1: Empty and undocumented catch blocks
What does the following code do?
Connection jdbcConnection = getConnection();
var sqlQuery = "select id,name from student where name like 'Robert%'";
ResultSet rs = null;
try {
try {rs = jdbcConnection.createStatement().executeQuery(sqlQuery);
} catch (SQLException e) {}
while (rs.next()) {
int id = rs.getInt(1);
String name = rs.getString(2);
logger.log(id + " is " + name);
}
} catch (SQLException e) {
logger.log("SQL Error", e);
}
The result depends on whether the first SQL operation succeeds. If the operation fails, the exception is swallowed—and nobody is the wiser. A few lines later, the code will get in trouble again, because ignoring the error is not a good strategy.
This example is distilled down from actual code in a library (whose name I have no wish to remember) that our team used in a project I worked on years ago. However, in the real library, the illicit exception swallowing and the failing code were a few hundred lines apart, and the whole mess was down about 20 levels in the library call stack. It took hours and hours to find this mess.
The bottom line is that exceptions should never be caught and ignored. Either catch the exception or don’t. If you do catch an exception, do something with it. At the very least, log the exception. If an exception is serious, either rethrow it or get out of the whole section of code that is in trouble.
Many modern frameworks (such as Spring and Jakarta) that deal with JDBC will catch checked SQL exceptions and rethrow them as unchecked exceptions. This allows you to process them as close to the human user as possible, instead of requiring rows and rows of try-catch statements or throws clauses.
The one exception to this rule of not ignoring exceptions is Thread.sleep, which has a checked InterruptedException. In single-threaded code, it may be permissible to ignore the exception if you comment it.
try {
Thread.sleep(5 * MSEC_PER_SEC);
} catch (InterruptedException ex) {
// Can't Happen - single threaded
}
Bonus worst practice: Ignoring warnings
Address warnings from your IDE as they appear. Compiler warnings are a mixed bag: Sometimes they indicate serious bugs, but many times they are irrelevant, and some experience is needed to hone your warning judgment.
By contrast, relevant warnings need to be fixed immediately, because if you let warnings build up, odds are that your team will get in the habit of ignoring them. And then, someday when you least expect it, a real bug will slip through into production, and in the postmortem, somebody will notice that the IDE had been warning about it for weeks.
Worse, once a project gets above some threshold of warnings, it is too late. You will probably never fix them.
Code quality matters. Please keep your code clean. The developer job you save may be your own.
Source: oracle.com
Posts
