6
\$\begingroup\$

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.

\$\endgroup\$
8
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ @Dex'ter You're welcome. I just made an edit to the connection opening code, btw. \$\endgroup\$ – jpmc26 Jan 11 '17 at 22:02
  • \$\begingroup\$ can you please put all the pieces of code into a single one ? I think I messed up something and I kinda' get some weird errors ^^ \$\endgroup\$ – яүυк Jan 13 '17 at 7:35
  • \$\begingroup\$ @Dex'ter This is more pseudocode to demonstrate the principles. I didn't compile it or test it. Notably, I didn't include any of the needed imports and the column lists are incomplete. Also, in a real world program, you would probably make these classes public and put them in separate files. There may be other syntax errors as well. Sorry. \$\endgroup\$ – jpmc26 Jan 13 '17 at 8:04
6
\$\begingroup\$

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");
    }
}
\$\endgroup\$
  • \$\begingroup\$ Hmm, I think you forgot the catch statement ^^ \$\endgroup\$ – яүυк Jan 11 '17 at 14:47
  • \$\begingroup\$ 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. \$\endgroup\$ – mtj Jan 11 '17 at 14:48
  • \$\begingroup\$ 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. \$\endgroup\$ – яүυк Jan 11 '17 at 14:52
  • \$\begingroup\$ 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 ;-) \$\endgroup\$ – mtj Jan 11 '17 at 15:05
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ But, will connection be available in the finally block ? '-_- \$\endgroup\$ – яүυк Jan 11 '17 at 13:46
  • \$\begingroup\$ 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 \$\endgroup\$ – Vogel612 Jan 11 '17 at 13:47
  • \$\begingroup\$ @Dex'ter edited my answer \$\endgroup\$ – Heslacher Jan 11 '17 at 13:49
  • \$\begingroup\$ @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 . \$\endgroup\$ – Heslacher Jan 11 '17 at 13:50
3
\$\begingroup\$

primitive obsession

Your method has a long parameter list and all parameters are of same primitive type.
(strictly speaking a String is not a primitive, but I count it as such to make my point...)

The problem with that is that you have to remember the order of the parameters when you use the method and you have no help from the compiler to detect missmatches.

So, if you look at the parameters you can group them into 3 clusters:

  • driver configuration (db_driver, db_type, db_driver_type)
  • connection configuration (db_host_name, db_port, db_service_name)
  • user authentication (db_username, db_password)

This means there could be 3 data transfer objects (DTO)s:

driver configuration

class OraDriverData{
   final String db_driver;
   final String db_type;
   final String db_driver_type;
  OraDriverData(String db_driver, String db_type, String db_driver_type){
     this.db_driver  =  db_driver;
     this.db_type  =  db_type;
     this.db_driver_type  =  db_driver_type;
  }

  // getters
   String getDbDriver(){
     return db_driver;
   }

   String getDbType(){
     return db_type;
   }

   String getDbDriverType(){
     return db_driver_type;
   }
}

connection configuration

class OraConnectionData{
   final String db_host_name;
   final String db_port;
   final String db_service_name;
   OraConnectionData(String db_host_name, String db_port, String db_service_name){
    // ... 
}

user authentication

class OraUserData{
   final String db_username;
   final String db_password;
   OraUserData(String db_username, String db_password){
   // ... 
}

your method signature would change to

public static void oracleDbConnection(
       OraDriverData oraDriverData,
       OraConnectionData oraConnectionData,
       OraUserData oraUserData
 ) {

With that signature the users of the method will be less confused. And you have the chance to store the DTOs somewhere and reuse them.

identifier names

This has three parts:

Naming conventions, prefixes and abbreviations.

Naming conventions

Java has detailed naming conventions: http://www.oracle.com/technetwork/java/codeconventions-135099.html

You should get used to this conventions. This will it make easier for you to read other peoples code and if you plan to work as a Java programmer in a company you have to anyway.

prefixes

Do not use prefixes for (all) identifiers.

Prefixes convey no information if all the identifiers in sight have the same (as in your example code).

When naming classes sometimes prefixes are a appropriate, but most times they are not.

Especially names of variables and methods should not have prefixes.

abbreviations

Do not use (uncommon) abbreviations.

Java identifiers can be of virtually infinite length and modern IDEs have auto completion. There is not need to use Abbreviations like Ora.

\$\endgroup\$
  • \$\begingroup\$ String is an object type, not a primitive. \$\endgroup\$ – jpmc26 Jan 16 '17 at 21:12
  • \$\begingroup\$ @jpmc26 I know and I mentioned that in the answer. \$\endgroup\$ – Timothy Truckle Jan 16 '17 at 21:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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