Storing .h files in SQLite using SQLJet

Question

I’m trying to learn as much as I can on my own by reading lots of examples, documentations, and asking here. I would like to improve my style to write efficient code and adhere to Java standards.

In this small sample of code I would like to get feedback on a few things:

• Exception throwing
• Opening/closing database connection
• Any other comments in general style

There are two classes, Database and Main.

My Database class:

public class Database {
 private String dbName = "";
 private SqlJetDb db = null;
 public Database(String dbName) {
 this.dbName = dbName;
 }
 public void CreateDatabase() throws SqlJetException {...}
 public void OpenDatabaseConnection() throws SqlJetException {...}
 public void CloseDatabaseConnection() throws SqlJetException {...}
 private void InsertRecord(String file) throws SqlJetException {...}
 public void GetDirectoryContent(String dir) {
 File directory = new File(dir);
 if (directory.isDirectory()) {
 String[] content = directory.list();
 for (String s : content) {
 GetDirectoryContent(dir + "\\" + s);
 }
 } else {
 String file = directory.toString();
 String extension = file.substring(file.lastIndexOf("."));
 if (extension.equals(".h")) {
 try {
 InsertRecord(file);
 } catch (SqlJetException e) {
 e.printStackTrace();
 }
 }
 }
 }
}

Call in main:

Database db = new Database("test.db");
 try {
 db.CreateDatabase();
 db.OpenDatabaseConnection();
 db.GetDirectoryContent("C:\\test"); 
 } catch (SqlJetException e) {
 e.printStackTrace();
 } finally {
 try {
 db.CloseDatabaseConnection();
 } catch (SqlJetException e) {
 e.printStackTrace();
 } 
 }
}

• kinaesthesia: You code is Ok but do you use java 6 or java 7 ?
• A.H.: Besides the logik: Clean code also involves adherence to the coding guidelines. So please name your methods according to the Java standards.
• Marko Topolnik: Your exception handling could be improved. Remove the try-catch from the GetDirectoryContent method and let that exception bubble up into your Main class. Take this as a general guideline—catch as late as possible. But, even before you start entering such details, you must first study the Java Naming Conventions and adhere to them religiously.
• Fivell: There is one problem in your code. You should read this en.wikipedia.org/wiki/Single_responsibility_principle Class Database shouldn't have file operations. It should be another class.
• amolkul: Use a Static Code Review tool like CheckStyle or PMD, which will cover most of the best practices. That way you can learn faster.

Answer 1

Personally I would

• always log rather than use e.printStackTrace. You can log to console if you require. By logging you can then change it easily to file logging and include your own comments
• use File.seperator instead of \. This gives a system independent way of separating your file paths.
• Consider making dbName final. You do not need to initialise it first and then in the ctor. Just do it in the ctor. Things like dbName should not be mutable.

• hofmeister: Thanks for your reply. Normally I log, just to handle the exception I added e.printStackTrace. The File.separator is new to me, but very nice to know, thanks. The dbName I changed to final private String dbName;. Greetz.
• user846476: Hi hofmeister, SUN convention say that is should be private final String dbName with the final after the private. Glad I could help
• Donald.McLean: By logging the errors, rather than handling them separately, you can have better control of what gets logged and to where.
• cl-r: When testing dabasebase you can use new File(dir+"/test.db").delete(); ... After you have to opening it and manage unexpected results.

Answer 2

Consider in the following:

public Database(String dbName) {
 this.dbName = dbName;
}

1. I would check for a null dbName being passed in. Note that you pass this reference and then simply store it. Consequently, if it's null, you won't find out until later (perhaps, much later). You then have to work out at what point that was set to null.
2. Will you change dbName ? If not, make it final. It'll stop you changing it inadvertently later on. Immutability is often a good idea. It makes the class more robust and thread-safety easier to achieve. Perhaps not a requirement here, but who knows ? It's easier to relax a restriction rather than apply it after-the-fact.

• hofmeister: Yea, I missing to check if the vaule is null. No I will not change, so final is better. Thanks!
• user846476: if you are checking for null then I would recommend constructing and IllegalArgumentException and setting the cause of this as NullPointerException. This way you are explicit in what is wrong.

Answer 3

From Effective Java

At the first glance :

you can put your class final

public final class Database {

and also the strings like :

final String file = directory.toString();
final String extension = file.substring(file.lastIndexOf("."));

Remove initialisation of fields :

private String dbName;
private SqlJetDb db;

Then in public void GetDirectoryContent use Files and Paths from new nio.2 java 7 package

And take care :

finally { // many possible problems

Read deeply and read again Joshua Bloch for good code, ... and enjoy yourself.

• Andre Hofmeister: Why remove private String dbName; and private SqlJetDb db;? Other functions use this aswell. Or did I missed something? I use Java6, so I cannot use nio, right? Greetz.
• cl-r: You do not have to remove field, but initialization of them, Objects are initalized to null at compile time, and "" String is initialized for nothing. Yes : nio.2 is linked to Java 7, so you cannot use it : it does not matter if you will not use new tools, if not a study to upgrade JVM may be needed.
• Andre Hofmeister: Fixed dbName. But I cannot initialize SqlJetDb there is now default consturctor with no parameter.
• cl-r: look comments in svn.sqljet.com/repos/sqljet/branches/maven/sqljet/src/main/java/… . Constructor(File, boolean) exist, but no default Constructor (not needed). . . . about your smelly problems, all responses are in Effective Java, not easy to understand, but necessary.

Answer 4

First of all, I'd leave the SqlJetException unchecked and not catch it in close method. I hate exceptions in close methods because what am I supposed to do with it? If I could do anything, I've already caught it, but most times it has to be muted catch(xxxx) {}.

I never use printStackTrace.

Does the GetDirectoryContent method insert records? The name is not clear enough, it's supposed to get a directory content, not insert any content. And it returns nothing... I think a better name would be something like

saveContentFrom(String directory)

Is this class a public API? If so, I would check for errors like "this param is null".

CreateDatabase, OpenDatabaseConnection and CloseDatabaseConnection are public, and you use them in GetDirectoryContent... what would happen if someone creates a database and calls your GetDirectoryContent? I would make those 3 methods private.

• Andre Hofmeister: I swaped GetDirectoryContent to a own class. Now I open the database connection with the Database class. The class which include GetDirectoryContent just read the data and append them to a ArrayList. Later I insert with the Database class the content of the ArrayList. Better? Greetz.