Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I'm trying to continue the learning process in Java, and I've made a simple yet useful class which will allow me to retrieve the data inside a table from an Oracle DB, using JDBC.


What I'm especially looking after in a review is:

  • styleguide - as in naming variables / classes, spacing, and so on)
  • usage of correct data types
  • best practices

Code:

package oraconnection;

import java.sql.*;

class OraConnection {

    public static void oracleDbConnection(
            String db_driver,
            String db_type,
            String db_driver_type,
            String db_host_name,
            String db_port,
            String db_service_name,
            String db_username,
            String db_password) {

        String connection_string = String.format("%s:%s:%s:@%s:%s/%s",
                db_driver,
                db_type,
                db_driver_type,
                db_host_name,
                db_port,
                db_service_name);

        try {
            Class.forName("oracle.jdbc.driver.OracleDriver");

            Connection connection = DriverManager.getConnection(
                    connection_string,
                    db_username,
                    db_password);

            Statement statement = connection.createStatement();
            String query = "select * from some_table";

            ResultSet result = statement.executeQuery(query);

            while (result.next()) {
                System.out.println(
                        result.getInt(1) + "  " + result.getString(2) + "  "
                        + result.getString(3));
            }
            connection.close();

        } catch (Exception e) {
            System.out.println(e);
        }

    }

    public static void main(String[] args) {
        oracleDbConnection(
                "jdbc",
                "oracle",
                "thin",
                "host",
                "port",
                "service-name",
                "user",
                "pass");
    }
}

Please bear with the fact that I'm a beginner when it comes to Java and I don't have a deep knowledge about its insides, so please make your answers as detailed as possible.

share|improve this question
up vote 5 down vote accepted

Minor detail: I prefer putting a closing parenthesis on its own line when the content between parentheses spans more than one line. (Basically, it looks like one-true-brace style with parentheses instead of braces.) I find it makes scanning vertically much simpler. If your team/company has chosen a different convention, that's fine.

The biggest problem I see with your code is that there's no division of responsibilities. For a very simple program, this doesn't matter, but in more complex application with a bunch of different queries and behaviors, it's a huge deal. The overhead of doing it isn't that big, either, so for almost any real world program, I recommend going ahead and dealing with this. (There's nothing more permanent than a temporary, quick, and dirty solution, after all.)

Creating the connection

Start by creating the connection in one place. This lets you reuse that code in multiple places in your application, and we'll see in a minute that it makes it much simpler to execute multiple queries in a transaction.

class OraConnection {
    public static Connection openConnection(
        String db_host_name,
        String db_port,
        String db_service_name,
        String db_username,
        String db_password
    ) {
        String connection_string = String.format(
            "%s:%s:%s:@%s:%s/%s",
            "jdbc",
            "oracle",
            "thin",
            db_host_name,
            db_port,
            db_service_name
        );

        Class.forName("oracle.jdbc.driver.OracleDriver");

        return DriverManager.getConnection(
            connection_string,
            db_username,
            db_password
        );
    }
}

I also recommend hard coding the "jdbc", "oracle", "thin" string here. The main reason is that the way you're opening the connection doesn't support any other values for them. For example, you can't open a PostgreSQL connection with this code because it looks for the oracle.jdbc.driver.OracleDriver class. Since the caller can't validly pass in anything else, there's no point in providing them as arguments. If your connection opening code were more general, I wouldn't advise that. However, this exposes one of the advantages of breaking things down into smaller pieces: you could change this code to be more general and allow more connection types without changing any of your other code.

Fetching results

Then create an object to represent your query results. Callers that want to execute your query and process the results shouldn't have to worry about details like a result set, what position each column is at, and what data type each column is. All that should be sorted out by the method that executes the query, and that method should return a simple object containing the results.

class SomeTable {
    private int column1;
    private int column2;
    private String column3;
    // ...and the rest of your columns

    public SomeTable(
        int column1,
        int column2,
        String column3,
        // the rest of your cols
    ) {
        this.column1 = column1;
        // etc.
    }

    public int getColumn1() { return this.column1; }
    // the rest of your columns
}

Then put the query itself in another class.

class SomeTableQueries {
    public static List<SomeTable> fetchAllSomeTableRows(Connection connection) {
        String query = "select column1, column2, column3, /* rest of your columns */ from some_table";

        try (
            Statement statement = connection.createStatement();
            ResultSet result = statement.executeQuery(query)
        ) {
            ArrayList<SomeTable> rows = new ArrayList<SomeTable>();
            while (result.next()) {
                rows.add(SomeTable(
                    result.getInt(1),
                    result.getString(2),
                    result.getString(3)),
                    // rest of your columns
                ));
            }
            return rows;
        }
    }
}

Note that it does the following:

  • It hides the boiler plate of building a statement and converting the data into a simpler form from callers.
  • It avoids SELECT *. SELECT * is okay if you have code that can automatically figure out what the result columns are and stuff them into an appropriate object, but we haven't written code like that here. Since our column set and the order is hard coded as part of reading the data, it's better to list them explicitly.
  • It cleans up the Statement and ResultSet objects it creates.
  • Names the method to describe the query it runs

Logic that uses all this

Now that we've got separate pieces of code to do smaller pieces, we can write our main method. Note that this method must be responsible for ensuring that the connection gets closed properly.

public class PrintSomeTable {
    public static void main(String[] args) {
        try (Connection conn = OraConnection.openConnection(
            "host",
            "port",
            "service-name",
            "user",
            "pass"
        )) {
            for (SomeTable r: SomeTableQueries.fetchAllSomeTableRows(conn)) {
                System.out.println(
                    r.getColumn1() + 
                    " " + 
                    r.getColumn2() + 
                    " " +
                    r.getColumn3()
                );
            }
        }
    }
}

Note that it would be very simple to use the same connection for more queries and operations.

Closing notes

The overall theme here is that you should use methods and classes to hide complexities that callers don't really care about, which makes understanding the code much simpler. This way, you can consider the details of each operation in an isolated scope, instead of trying to understand what each "larger" logical operation is by sorting through every little detail.

Lastly, if you have a lot of queries, seriously consider using an ORM, a micro ORM, or other similar tooling to handle a good amount of this boiler plate for you.

share|improve this answer
    
Awesome explanation + answer. Thanks a lot – Dex' ter 13 hours ago
    
@Dex'ter You're welcome. I just made an edit to the connection opening code, btw. – jpmc26 12 hours ago

In addition, you should always close your ResultSet and your Statement explicitly. (Or: also use these in a try-with-resources block, and yes, both are AutoCloseable.)

The background is, that these objects might be tied to system resources, and there is no guarantee that the database driver automatically closes these dependent objects when you close the connection. (I think the oracle driver behaves well, but this is a specific implementation detail which you should never rely on.)

For the application you have (simple statement, read once) a single try-with-resources block seems preferable:

package oraconnection;

import java.sql.*;

class OraConnection {

    public static void oracleDbConnection(
            String db_driver,
            String db_type,
            String db_driver_type,
            String db_host_name,
            String db_port,
            String db_service_name,
            String db_username,
            String db_password) {

        String connection_string = String.format("%s:%s:%s:@%s:%s/%s",
                db_driver,
                db_type,
                db_driver_type,
                db_host_name,
                db_port,
                db_service_name);

        // matters of taste: you could also insert the second try-block into
        // this first try block after Class.forName, but I prefer to keep
        // the nesting depth low...
        try {
            Class.forName("oracle.jdbc.driver.OracleDriver");
        } catch (Exception e) {
            System.out.println(e);
            return;
        }

        // Changing the order: prepare everything (in this case: the query) before
        // we get to the resource-critical point
        String query = "select * from some_table";

        // Try-with-resources to encapsulate everything that needs to be closed
        try (
            Connection connection = DriverManager.getConnection(
                connection_string,
                db_username,
                db_password);
            Statement statement = connection.createStatement();
            ResultSet result = statement.executeQuery(query);
        ) {
            while (result.next()) {
                 System.out.println(
                        result.getInt(1) + "  " + result.getString(2) + "  "
                        + result.getString(3));
            }
            // As we did try-with-resources, all three objects in the try() will be closed
            // automagically. No need to call close() or for finally-blocks.
        } catch (Exception e) {
            System.out.println(e);
        }
    }

    public static void main(String[] args) {
        oracleDbConnection(
                "jdbc",
                "oracle",
                "thin",
                "host",
                "port",
                "service-name",
                "user",
                "pass");
    }
}
share|improve this answer
    
Hmm, I think you forgot the catch statement ^^ – Dex' ter 19 hours ago
    
That depends :-) To fit in you code, yes, you are right, we'd need the catch. As I often prefer to handle the exception on application (i.e. caller) level instead of utility level, I'd probably leave the block without a catch and declare a throws clause in real life. – mtj 19 hours ago
    
Could you please edit your answer to add the full code ? I have some doubts regarding how the final solution will look like in the end. – Dex' ter 19 hours ago
    
Sure, see above. Please note, that I only shuffeled text around in an editor, i.e. I have not tried to compile it - syntax problems might be included ;-) – mtj 19 hours ago

There is one important thing you should always do if working with a database:

You should always close the connection. Therefore I would suggest adding a finally clause to the try..catch and add the following

Connection connection = null;
try {
    Class.forName("oracle.jdbc.driver.OracleDriver");

    connection = DriverManager.getConnection(
            connection_string,
            db_username,
            db_password);

    Statement statement = connection.createStatement();
    String query = "select * from some_table";

    ResultSet result = statement.executeQuery(query);

    while (result.next()) {
        System.out.println(
                result.getInt(1) + "  " + result.getString(2) + "  "
                + result.getString(3));
    }
    connection.close();

} catch (Exception e) {
    System.out.println(e);
} finally {
    if (connection != null && !connection.isClosed()) {
        connection.close();
    }  
}

and remove the connection.close() at the end of the try block. Just note you need to declare Connection connection; outside of the try block.

Any code in the finally block will be executed regardless if you return early out of the try block or if an exception had been trown insider the try block.

Like @Vogel612 has stated in the comment it would be better to use try-with-resources which can be used with objects which implenents the [AutoClosable][2] interface.

This is (at least as I know) syntactical sugar for the try..finally usage.


I've made a simple yet useful class which will allow me to retrieve the data inside a table from an Oracle DB, using JDBC.

Looking at the class in question, where the only non main() is named oracleDbConnection it doesn't read like you are retrieving anything but it reads more like a constructor. Methods should be named using verbs whereas classes/objects should be named using nouns.


It would be better to have a class which really creates an connection or execute a adjustable query against a created connection. Right now the query is hardcoded to String query = "select * from some_table";. I understand that this is just a test query, but if you would have a method where you pass that querystring to then it would be better, wouldn't it ?

In addition, you shouldn't query for * if you don't need all columns/rows.

share|improve this answer
    
But, will connection be available in the finally block ? '-_- – Dex' ter 20 hours ago
    
instead of try-catch-finally it's more idiomatic (and cleaner) to use a try-with-resources in java (which is equivalent to c#'s using. Additional note: Passing query strings to a class is often an encapsulation violation. You'd want the calling code to not know that the persistence layer is a SQL-Db as opposed to a NoSqlDB, (e.g. a redis cache) or even a file – Vogel612 20 hours ago
    
@Dex'ter edited my answer – Heslacher 20 hours ago
    
@Vogel612 regarding try-with-resources that is a valid point. I thought about it but hadn't the time to remebre the name and to look if Connection is a AutoClosable . – Heslacher 20 hours ago

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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