4
\$\begingroup\$

I'm trying to write my first "complex" program in Java. So far, I created a local H2 database but I'm not sure about the quality of my code (I'm newbie in Java) because I don't know how I can deal with exceptions and I don't feel comfortable with the JDBC code.

 public boolean create(Project project)  {
 String id = UUID.randomUUID().toString().toUpperCase().replace("-", "");
 project.setId(id);

    try {
        Class.forName("org.h2.Driver");
        Connection connection = DriverManager.getConnection("jdbc:h2:~/dao_db", "sa", "");
        PreparedStatement preparedStatement = connection.prepareStatement("INSERT INTO PROJECT VALUES(?, ?,?)");

        preparedStatement.setString(1, id);
        preparedStatement.setString(2, project.getName());
        preparedStatement.setString(3, project.getTrigram());
        int resultSet = preparedStatement.executeUpdate();

        if(resultSet!=0)
        {
            System.out.println("Project created");
            connection.close();
            return true;
        }

        else {
            System.out.println("No project was  created ");
            connection.close();
            return false;
        }

    } catch (ClassNotFoundException | SQLException e) {
        e.printStackTrace();
    }
    return false;

}

@Override
public boolean update(Project project){

    try {
        Class.forName("org.h2.Driver");

        Connection connection = DriverManager.getConnection("jdbc:h2:~/dao_db", "sa", "");
        PreparedStatement preparedStatement = connection.prepareStatement("UPDATE PROJECT SET NAME=?, TRIGRAM = ? WHERE ID=?");

        preparedStatement.setString(1, project.getName());
        preparedStatement.setString(2, project.getTrigram());
        preparedStatement.setString(3, project.getId());
        int resultSet = preparedStatement.executeUpdate();

        if(resultSet==0) {
            System.out.println("The project was not updated ");
            connection.close();
            return false;
        }
        else {
            System.out.println("Project updated" + '\n'+  project.toString());
            connection.close();
            return true;
        }

    } catch (ClassNotFoundException | SQLException e) {
        e.printStackTrace();
    }
    return false;
}

@Override
public boolean delete(String id) {

    try {
        Class.forName("org.h2.Driver");
        Connection connection = DriverManager.getConnection("jdbc:h2:~/dao_db", "sa", "");
        PreparedStatement preparedStatement = connection.prepareStatement("DELETE FROM PROJECT WHERE ID=?");
        preparedStatement.setString(1,id);
        int result = preparedStatement.executeUpdate();

        if(result!=0) {

            PreparedStatement preparedStatement2 = connection.prepareStatement("DELETE FROM MILESTONE WHERE FK_PROJECT_ID =?");
            preparedStatement2.setString(1,id);
            preparedStatement2.executeUpdate();

            PreparedStatement preparedStatement3 = connection.prepareStatement("DELETE FROM REQUIREMENT WHERE FK_PROJECT_ID =?");
            preparedStatement3.setString(1,id);
            preparedStatement3.executeUpdate();

            System.out.println("Project deleted with id = " + id);
            connection.close();
            return true;

        }

        else {
            System.out.println("No project was deleted with id = " + id);
            connection.close();
            return false;
        }

    } catch (ClassNotFoundException | SQLException e) {
        e.printStackTrace();
    }
    return false;
}
  1. As you can see, the management of the exception here is very very poor. Can you help me to optimize this? Should I use finally or something like that?

  2. I feel that there a big redundancy in my code since that I'm obliged to write many lines to create a JDBC connection and execute a SQL query. I have 6 tables, so the quality is really poor. I don't know how can I optimize this because the query changes in each method.

\$\endgroup\$

locked by Jamal Apr 10 '16 at 20:58

This post has been locked while disputes about its content are being resolved. You may discuss this on meta if you have concerns.

Read more about locked posts here.

  • 1
    \$\begingroup\$ Please leave the code intact as answers have already been posted. \$\endgroup\$ – Jamal Apr 1 '16 at 21:48
3
\$\begingroup\$

Nitpick

 public boolean create(Project project)  {
 String id = UUID.randomUUID().toString().toUpperCase().replace("-", "");
 project.setId(id);

The last two lines should be indented relative to the method declaration. Perhaps this was just a copy/paste error that doesn't exist in the original code.

try

    try {
        Class.forName("org.h2.Driver");
        Connection connection = DriverManager.getConnection("jdbc:h2:~/dao_db", "sa", "");
        PreparedStatement preparedStatement = connection.prepareStatement("INSERT INTO PROJECT VALUES(?, ?,?)");

        preparedStatement.setString(1, id);
        preparedStatement.setString(2, project.getName());
        preparedStatement.setString(3, project.getTrigram());
        int resultSet = preparedStatement.executeUpdate();

        if(resultSet!=0)
        {
            System.out.println("Project created");
            connection.close();
            return true;
        }

        else {
            System.out.println("No project was  created ");
            connection.close();
            return false;
        }

    } catch (ClassNotFoundException | SQLException e) {
        e.printStackTrace();
    }

This is a really long try block, and most of the code doesn't seem to need it. Also, it doesn't make use of the try with resources, even though it seems to exactly cover the use case. Instead consider breaking it into two smaller try blocks.

    try {
        Class.forName("org.h2.Driver");
    } catch (ClassNotFoundException e) {
        e.printStackTrace();
        return false;
    }

All this code does is verify that the class exists. I would actually do this outside a try block, as it seems like a fatal error. But this version at least isolates the class problem from the SQLException issue, as they have different responses.

    int resultSet = 0;

Technically, the 0 is unnecessary here, as it is already the default. I find it clearer to initialize it explicitly. Others disagree.

Removing from the try block will allow it to be used outside the block later.

    try (Connection connection = DriverManager.getConnection("jdbc:h2:~/dao_db", "sa", "")) {
        PreparedStatement preparedStatement = connection.prepareStatement("INSERT INTO PROJECT VALUES(?, ?,?)");

        preparedStatement.setString(1, id);
        preparedStatement.setString(2, project.getName());
        preparedStatement.setString(3, project.getTrigram());

        resultSet = preparedStatement.executeUpdate();
    } catch (SQLException e) {
        e.printStackTrace();
    }

Now you don't have to worry about explicitly closing the connection. The try with resources will handle that automatically no matter how or when you leave the try block.

Note that in the original code, an exception after the connection was made but before the connection was explicitly closed would bypass closing the connection. You could have used a finally block to work around this, but the try with resources is simpler and more reliable.

    if (resultSet != 0) {
        System.out.println("Project created");
        return true;
    }

    System.out.println("No project was created");
    return false;

This block of code cannot generate either of the caught exceptions. As such, it does not need to be in the try block. So go ahead and take it out.

Consistent formatting

There are three common ways to handle a {} block. You use all three. Please pick one and stick to it consistently. Consistent formatting makes it easier to both read and modify the code.

Start and end on the same line:

    } else {

Start and end on separate lines:

    }
    else
    {

Closing } alone:

    }
    else {

Also, note that the second and third versions allow you to put extra blank lines between the braces and the else. Please don't do that. When you do that, it obscures that the previous block has an else. It's much clearer if you treat the braces and the else as one symbol and always write them the same way. Then the reader can immediately see which block ends and which will continue later.

Personally, I prefer the first form, but all three are in common use. The Java coding standard also prefers the first form, but some prefer one of the others. Consistent use of any of them within your code will allow most programmers to adapt.

\$\endgroup\$
1
\$\begingroup\$

This code is repeated several times:

Class.forName("org.h2.Driver");
Connection connection = DriverManager.getConnection("jdbc:h2:~/dao_db", "sa", "");

So you should do a refactor, and you should extract this code in another function. This function could be called from the other functions where you need the Database Connection.

Also you can do a general function in order to execute different queries, depending on the kind of query (insert, update, select...). This function would receive and sql query. For instance:

private PreparedStatement executeQuery (String sqlQuery, HashMap <String,String> parameters){

PreparedStatement preparedStatement = connection.prepareStatement(sqlQuery);

//Iterate and fill the necessary parameters

return preparedStatement;

}

On the other hand in production environment you should not use:

e.printStackTrace();

Because the log would be very dirty and you will have some problems in order to do search within the logs, furthermore you won't be able to find the errors, so you can use another method to show the exeception message, for example you can use the getMessage() method.

Finally, Depending on the kind of application you want to do , you should process the exceptions in different ways.

Regards Ángel

\$\endgroup\$
  • \$\begingroup\$ Why shouldn't stack traces be printed? I think they can be useful in debugging problems (though a debugger fills the same role). \$\endgroup\$ – jacwah Apr 1 '16 at 9:13
  • \$\begingroup\$ @jacwah Depends on your logger system, I mean, If you only have a one log file, perhaps you have problems to find the error, because you have previously a lot of exceptions and you have research what the exception is the proper. However if your have a good logger system, you can split the logs in different files, so you can have a file only for exceptions and errors, furthermore you can have another file for info , another for webservices requests, etcetera In our projects we have different logger files for different parts, including exceptions, also we try to make easier our log search \$\endgroup\$ – angel javier salido martinez Apr 1 '16 at 10:09

Not the answer you're looking for? Browse other questions tagged or ask your own question.