On occasion, you see code that another person has written—or code that you just wrote—and smack your head in surprise, disbelief, and dismay.
I’ll reiterate what I wrote within the earlier article’s introduction: It’s best to keep away from these worst practices—and eradicate them whenever you preserve or refactor current code. And, after all, resolve them should you see these points throughout a code evaluation.
Worst observe #5: Duplicating code
Many builders are taught early on that copy-and-paste is a nasty thought. Actually copying code from elsewhere in an software is unhealthy as a result of it creates a upkeep nightmare: Discovering a bug or altering the performance requires that you just discover all of the copies and repair all of them. Copies are additionally unhealthy as a result of they make a program needlessly bigger.
(In my protection, typically a developer does copy a code instance for authentic causes that wouldn’t apply when constructing an actual software.)
Whereas writing this text, I ran CPD towards my repository, and it discovered a number of points. Listed below are two.
$ cpd
Discovered a 14 line (184 tokens) duplication within the following information:
Beginning at line 19 of /residence/ian/git/javasrc/desktop/src/most important/java/gui/FlowLayoutSimple.java
Beginning at line 37 of /residence/ian/git/javasrc/desktop/src/most important/java/gui/FlowLayoutSimple.java
getContentPane().add(quitButton = new JButton(“Cease”));
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 primary one is attention-grabbing. It’s clearly an enhancing error; whenever you use the vi editor, a quantity adopted by an insert causes the insertion of that variety of copies of the insert. Nevertheless, numbers adopted by the letter G (for go) are used to leap to a line by quantity.
My guess is that I typed a quantity to leap to a line, forgot the G, and typed a line to be inserted at that location, inflicting the road to be erroneously inserted many occasions. Surprisingly, this error has been in my public repository since 2003, and no person has ever reported it to me.
The second difficulty recognized an 18-line (184 tokens) duplication within the following information:
Beginning at line 28 of /residence/ian/git/javasrc/most important/src/most important/java/regex/LogRegEx.java
Beginning at line 25 of /residence/ian/git/javasrc/most important/src/most important/java/regex/LogRegExp.java
System.out.println(“Enter line:” + logEntryLine);
Matcher matcher = p.matcher(logEntryLine);
if (!matcher.matches() ||
LogParseInfo.MIN_FIELDS > matcher.groupCount()) {
System.err.println(“Unhealthy log entry (or drawback with regex):”);
System.err.println(logEntryLine);
return;
}
System.out.println(“IP Handle: ” + 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 Despatched: ” + matcher.group(7));
if (!matcher.group(8).equals(“-“))
System.out.println(“Referer: ” + matcher.group(8));
System.out.println(“Consumer-Agent: ” + matcher.group(9));
});
The identical program demonstrated using common expressions to parse the frequent Apache Log File format, and it appears as if I one way or the other unintentionally created the identical file with two totally different names, maybe whereas merging information into this repository from one other.
Right here I’m, rightfully busted by a instrument that I typically advocate. I shall have to make use of CPD extra typically.
Worst observe #4: Out-of-date Javadoc
/**
* Carry out the aFunction operate.
* @parameter x The X coordinate to begin
* @parameter y The Y coordinate to begin
* @Parameter len The variety of factors to course of
*/
public double aFunction(double x, double y, double endX, double endY) {
Your Javadoc could be extra helpful whether it is generated in codecs comparable to HTML, for reference. Maven and Gradle and different construct instruments have plugins that make it straightforward to generate Javadoc internet pages as a part of your total construct course of. In Maven it could take 10 or 15 strains of plugin configuration to tame Javadoc, however as soon as that’s written, that configuration not often modifications.
It’s possible you’ll need to embrace the next configuration ingredient when getting began:
<failOnError>false</failOnError>
By the best way, outdated and sporadically maintained Javadoc will in any other case fail the construct utterly. This provides you time to scrub up the documentation incrementally and get it right into a situation you’ll be proud to indicate to different builders.
Worst observe #3: Unvalidated person enter
By no means belief person enter.
Nicely, they really wrote, “Check enter for plausibility and validity,” however I like my formulation higher.
When studying code during which somebody has written JDBC calls, it isn’t unusual to seek out this antipattern within the first assertion.
rs = jdbcConnection.createStatement().executeQuery( // unhealthy
“choose * from prospects the place title=”” + nameField.getText() + “”;”);
PreparedStatement assertion = jdbcConnection.prepareStatement(
“choose * from prospects the place title = ?1”); // higher
assertion.setString(1, nameField.getText());
rs = assertion.executeQuery();
The worth of nameField.getText() is sort of definitely coming straight from the person; that’s, it’s person enter that ought to by no means be trusted. Nevertheless, this knowledge is being fed instantly into the database.
John Smith’; drop desk prospects; —
It will likely be as if you had entered the next SQL:
“choose * from prospects the place title=”John Smith”; drop desk prospects; –‘;”
Many, if not most, JDBC drivers enable a couple of assertion on a line, with a semicolon (;) between every. Now what if the database architect was as careless because the developer? By not limiting the database account utilized by the app server from having drop privileges, it’s recreation over.
The — on the finish is the twist of the knife as a result of it’s going to cease the leftover delimiter characters from even inflicting a syntax error within the log file, obfuscating the place the vandalism occurred.
Worst observe #2: Not testing the not-unit-testable
I dread strolling into an old-school mission that lacks unit checks.
Many older functions have been written in a single class, typically termed a ball of wax or all-in-one class or perhaps a monster class. (There are even less-polite names.) It’s tough to write down unit checks for monster courses as a result of unit checks, by definition, are designed to check one small unit of code. A monster class has no small items of code to check! Not solely are there no checks, however the code can be not written to be testable.
In case you are tasked with sustaining such an software, begin carving the monster into smaller items that may be examined. How huge or small ought to the code courses be? That’s a subject for infinite debate, as there isn’t a magic measurement and no precise variety of strains of code for courses or for strategies.
The only-responsibility precept (SRP) says that every class ought to have one major duty: performing some calculations, processing an order, or displaying the outcomes. In different phrases, in case your software does these three issues, you want not less than three courses. Equally, SRP says {that a} technique ought to do one factor, and one factor solely.
When you extract code out of the monolith, write unit checks—and ensure they move.
Worst observe #1: Empty and undocumented catch blocks
What does the next code do?
Connection jdbcConnection = getConnection();
var sqlQuery = “choose id,title from pupil the place title like ‘Robert%'”;
ResultSet rs = null;
attempt {
attempt {rs = jdbcConnection.createStatement().executeQuery(sqlQuery);
} catch (SQLException e) {}
whereas (rs.subsequent()) {
int id = rs.getInt(1);
String title = rs.getString(2);
logger.log(id + ” is ” + title);
}
} catch (SQLException e) {
logger.log(“SQL Error”, e);
}
The end result depends upon whether or not the primary SQL operation succeeds. If the operation fails, the exception is swallowed—and no person is the wiser. A number of strains later, the code will get in hassle once more, as a result of ignoring the error will not be technique.
This instance is distilled down from precise code in a library (whose title I’ve no want to keep in mind) that our crew utilized in a mission I labored on years in the past. Nevertheless, in the actual library, the illicit exception swallowing and the failing code have been just a few hundred strains aside, and the entire mess was down about 20 ranges within the library name stack. It took hours and hours to seek out this mess.
The underside line is that exceptions ought to by no means be caught and ignored. Both catch the exception or don’t. Should you do catch an exception, do one thing with it. On the very least, log the exception. If an exception is critical, both rethrow it or get out of the entire part of code that’s in hassle.
Many fashionable frameworks (comparable to Spring and Jakarta) that take care of JDBC will catch checked SQL exceptions and rethrow them as unchecked exceptions. This lets you course of them as near the human person as doable, as an alternative 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 could be permissible to disregard the exception should you remark it.
attempt {
Thread.sleep(5 * MSEC_PER_SEC);
} catch (InterruptedException ex) {
// Cannot Occur – single threaded
}
Bonus worst observe: Ignoring warnings
Handle warnings out of your IDE as they seem. Compiler warnings are a combined bag: Typically they point out critical bugs, however many occasions they’re irrelevant, and a few expertise is required to hone your warning judgment.
Against this, related warnings must be mounted instantly, as a result of should you let warnings construct up, odds are that your crew will get within the behavior of ignoring them. After which, sometime whenever you least anticipate it, an actual bug will slip by means of into manufacturing, and within the postmortem, anyone will discover that the IDE had been warning about it for weeks.
Worse, as soon as a mission will get above some threshold of warnings, it’s too late. You’ll most likely by no means repair them.
Code high quality issues. Please preserve your code clear. The developer job you save could also be your individual.
Supply: oracle.com
