About a month ago I went with my friend David to Silicon Valley CodeCamp. David met a few friends so we all set together and had lunch during which we discussed a few coding issues but we were also excited about the raffle which was to take place right after lunch. I told them that I’m not interested in the XBOX (which was considered as the best prize) and that what I would really LOVE is to get a license for intelliJ. First time I “met” intelliJ was
Back to our lunch at Silicon Valley CodeCamp. People were hoping to win the XBOX and I told them that I don’t care about the XBOX and that I wish they would throw in intelliJ licenses (one license worth $500). We finished our lunches and went to the main court where the raffle took place, and to my astonishment, the guy with the mic announced the list of prizes and among them, one intelliJ license. I was stunned. David laughed and said: “I’m going to win this one for you”.
And believe it or not – he did!
A few days later, I’m sitting at my desk, installing the newest version of intelliJ, and it took me only a few seconds to see what I was missing so much…
Jetbrains, the company that made intelliJ had a booth there in SVCC – so I got myself a shirt that says: “I see dead code”. And now I really do: among many other cool features, intelliJ finds unused parameters and colors them in grey. This simple feature is priceless (and I’m not familiar with any equivalent feature in Netbeans/Eclipse).
The shirt I got at SVCC
So, at that same first day when I started using intelliJ again, I went over some code, looking for something, and then I run into the following method (two points to the readers that find what’s wrong with this method):
private String getContractName(Connection conn, int contractOrderId) {
String methodName = "getContractName";
PreparedStatement st = null;
ResultSet rs = null;
String contractName = null;
try {
//get contract name
String sqlQuery = "SELECT CONTRACT_NAME FROM __CONTRACTS WHERE contract_order_id=?";
st.setInt(1, contractOrderId);
rs = st.executeQuery();
if (rs.next()) {
contractName = rs.getString("CONTRACT_NAME");
}
}
catch (Exception e) {
Utils.writeLog(className + ":" + methodName +"- Warning: " + className + ":" + methodName + "-" + e.getMessage(), DEBUG);
}
finally {
Utils.closeConnections(null, st, rs, null);
}
return contractName;
}
Without intelliJ it’s not easy to see it, but if you figured it out – way to go!
Between the following two lines:
String sqlQuery = "SELECT CONTRACT_NAME FROM __CONTRACTS WHERE contract_order_id=?";
st.setInt(1, contractOrderId);
there’s one “little” line missing:
st = conn.prepareStatement(sqlQuery);
This could be easily detected with intelliJ since “Connection conn” passed in the method’s signature was marked grey. Actually, there isn’t only one wrong thing with this method, now that we understand the problem, we can also see that every time this method is being called, an exception is being thrown on:
st.setInt(1, contractOrderId);
and that this exception is caught six lines below – but the only thing that is being done is a “debug” printing to log (not even an error!) and then the method ALWAYS returns null and continues.
How many bad practices do we see here – can you count ?
1. we don’t “fail-fast” (the method should have either thrown an error or at least print ERROR to log instead of DEBUG).
2. we throw and catch an exception (which is costly!) without really handling it.
3. we return a default value which is meaningless
4. I’m 100% sure that no unit-tests, functionality tests, regression, end-to-end or any kind of tests were written and used here – otherwise this bug would have been easily caught.
So here’s a call to all you decision makers, think about the hours of debugging, fixing, reviewing the code-fix, QAing the fix and deploying – that could be saved, not to mention potential damage that could be done to your company and your clients. Always consider purchasing the best tools you can for your engineers. It might look like an “expensive investment” but it’ll pay off, trust me (and if you can’t trust me, you should believe Joel. Check-out item #9 on his list).
Just Another Programmer 