Is this the real
Quackster? Congratulations on seeing the need for DAOs! Been skimming through some of the code and this thread and this looks quite promising. Browsing your code at your repo in the browser should give you a very clear idea of
why you don't want to use tabs for indentation. (You're also still using Eclipse, like come on its 2016 for crying out loud) After looking through some of your DAOs, I would believe what The General was trying to point out earlier was that if something wrong happens between opening the SQL connection (eg. aquiring a PreparedStatement) and releasing it through Storage.releaseObject. There are a number of things wrong with this approach, if you look at the function getItem(long itemId) on
You must be registered to see links
:
Code:
@Override
public Item getItem(long itemId) {
try {
PreparedStatement statement = dao.getStorage().prepare("SELECT * FROM items WHERE id = ? LIMIT 1");
statement.setLong(1, itemId);
ResultSet row = statement.executeQuery();
Item item = new Item();
if (!row.next()) {
return null;
}
this.fill(item, row);
Storage.releaseObject(row);
return item;
} catch (Exception e) {
Log.exception(e);
}
return null;
}
If an exception is thrown at statement.executeQuery, the connection to the database would remain open. I can see that you have some finally blocks here and there but this is still a dangerous practice as you have to remember to write the finally block. You also get code duplication for this across all the places where you handle the closing of the connection. This is what
You must be registered to see links
solves. By making the PreparedStatement implement the AutoClosable interface, you avoid zombie connections, code duplication and having to remember these retarded things that can duck you up really bad.
And don't do SELECT *, don't. You rely on a specific order from the database (but in this case you specify them later on so thats fine), but then if something is missing you get an exception on a far later stage than if it is in the query itself (An exception would be thrown when querying the database instead of in the fill command).
I really dislike the set functions where you specify the index of an argument (setLong in your case) as it is not maintainable when you get a few more parameters. Using parameter names is far easier to read and maintain. I'm also questioning why you have a LIMIT 1 on a query where it would be reasonable that the
id field is the primary key and it would therefore always return either one or zero rows.
Your code also demonstrates a good example of error hiding where if an exception is thrown in the try-catch block, the caller does not know whether the database call failed or if the item does not exist.
The code I quoted has some really strong dependencies that makes it hard to test, but this is up to you.
I'm not gonna say this is code is good, because it wasn't (Yet it was actually consistently formatted which I find rare in the Java world for some strange reason [???]). But I still think this is a really good project compared to the other I've seen so far and it got a great potential. Good luck! (And are you the real
Quackster????)