0
\$\begingroup\$

I wanted to write a program that performed a query to a database, then formatted the data before writing it to a file. I thus created 4 classes, which are:

  • a main class
  • a class dealing with the queries
  • a class dealing with the database connection
  • a class dealing with the formatting and writing of the data to a file

Does the meta structure make sense ?

Main class

public class Mat {
    public static final Prop QUERIES = new Prop("mat_queries");
    public static void main(String[] args) {
        String jdbc = "jdbc";
        if (args.length >= 2) {
            jdbc += args[1];
        }
        ConnectionUtils cu = new ConnectionUtils(jdbc);
        Connection conn = null;

        String runId = args[0]
        try {
            DataExtract deals = new DataExtract(jdbc);
            conn = cu.newConnection();
            File fileDeals = new File("DealData.csv");

            deals.getNodes(runId, fileDeals, conn);     
        } catch (Exception e) {
            e.printStackTrace();
        } finally {
            cu.close(conn);
        }
    }
}

Query class

public class DataExtract extends ConnectionUtils {
    private DataWriter dw;

    public DataExtract(String jdbcParams) {
        super(jdbcParams);
        this.jdbcParams = new JdbcParams(jdbcParams);
        this.dw = new DataWriter();
    }

    private static final int COL = 5;

    public void getNodes(String runId, File file, Connection conn) {
        PreparedStatement ps = null;
        ResultSet rs = null;

        try {
            ps = conn.prepareStatement(Mat.QUERIES.get("DealData.DATA_NTT"));
            ps.setString(1, runId);
            ps.setString(2, runId);
            ps.setFetchSize(10000);
            rs = ps.executeQuery();

            dw.writeCore(file, rs, COL);
            dw.writeFooter(file, "value;dealData");
        } catch (Exception e) {
            logger.error(e, e);
        } finally {
            close(rs, ps);
        }
    }
}

Connection class

public class ConnectionUtils {
    protected JdbcParams jdbcParams;

    public ConnectionUtils(String env) {
        this.jdbcParams = new JdbcParams(env);
    }

    public Connection newConnection() throws SQLException, ClassNotFoundException {
        Class.forName(jdbcParams.getString("jdbc.driver"));
        Connection connect;
        connect = DriverManager.getConnection(jdbcParams.getString("jdbc.url"),
                jdbcParams.getString("jdbc.username"),
                jdbcParams.getString("jdbc.password"));
        return connect;
    }

    public void close(Connection conn) {
        if (conn != null) {
            try {
                conn.close();
            } catch (SQLException e) {
                logger.error("The data source connection cannot be closed.", e);
            }
        }
    }

    public void close(ResultSet rs, PreparedStatement ps) {
        if (rs != null) {
            try {
                rs.close();
            } catch (SQLException e) {
                logger.error("The result set cannot be closed.", e);
            }
        }
        if (ps != null) {
            try {
                ps.close();
            } catch (SQLException e) {
                logger.error("The statement cannot be closed.", e);
            }
        }
    }
}

Writing class

public abstract class DataWriter {

    public static final String LINE_1 = "description;header;pricingContextId;pricingContextIdScheme;inventoryDate;CVACalculationTimestamp;CVAExtractionTimeStamp";

    public void writeHeader(File file, ResultSet rs) throws FileNotFoundException, IOException, SQLException {
        BufferedWriter output = new BufferedWriter(new OutputStreamWriter(new FileOutputStream(file, false), "UTF-8"));
        try {
            output.write(LINE_1);
            output.newLine();
            Date today = new Date();
            String dates = new String("value;header;;;");
            while (rs.next()) {
                dates += rs.getString(1) + ";" + rs.getString(2);
            }
            dates += ";" + new SimpleDateFormat("yyyy-MM-dd").format(today) + "T"
                    + new SimpleDateFormat("HH:mm:ss").format(today);

            output.write(dates);
            output.newLine();
        } finally {
            output.close();
        }
    }

    public void writeCore(File file, ResultSet rs, int colNode) throws FileNotFoundException, IOException {

        int count = 0;
        try {
            BufferedWriter output = new BufferedWriter(
                    new OutputStreamWriter(new FileOutputStream(file, true), "UTF-8"));

            ResultSetMetaData rsmd = rs.getMetaData();
            int colCount = rsmd.getColumnCount();

            while (rs.next()) {
                for (int col = 1; col <= colCount; col++) {
                    try {
                        String val = rs.getString(col);
                        if (rs.wasNull()) {
                            val = "";
                        }
                        output.append(val);
                    } catch (ArrayIndexOutOfBoundsException e) {
                        String dec = rs.getBigDecimal(col).toPlainString();
                        System.err.println(String.format("%s %d %s %s %d %s %d", rs.getString(1), col,
                                rsmd.getColumnTypeName(col), file, count, dec, dec.length()));
                        output.append(dec);
                    }
                    if (col < colCount) {
                        output.append(";");
                    }
                }
                output.newLine();
                count++;
            }
            output.close();
        } catch (Exception e) {
            logger.error(e, e);
        }
    }

    public void writeFooter(File file, String searched) throws FileNotFoundException, IOException {
        InputStream is = new BufferedInputStream(new FileInputStream(file));
        BufferedWriter output = new BufferedWriter(new OutputStreamWriter(new FileOutputStream(file, true), "UTF-8"));
        try {
            byte[] c = new byte[1024];
            int count = 1;
            int readChars = 0;
            while ((readChars = is.read(c)) != -1) {
                for (int i = 0; i < readChars; ++i) {
                    if (c[i] == '\n') {
                        ++count;
                    }
                }
            }
            output.append("value;footer;" + count + ";" + getOffset(file, searched));
            output.newLine();
        } finally {
            is.close();
            output.close();
        }
    }

    public int getOffset(File file, String searched) throws FileNotFoundException {
        Scanner scanner = new Scanner(file).useDelimiter(System.getProperty("line.separator"));
        int occurences = 0;
        while (scanner.hasNextLine()) {
            String s = scanner.nextLine();
            if (s.indexOf(searched) >= 0) {
                occurences++;
            }
        }
        return occurences;
    }
}

So basically my next question is: What can be improved? I already know that sonar is complaining about my exception handling, but I don't really know where (and what) to catch and where to throw in my case.

\$\endgroup\$
  • 1
    \$\begingroup\$ As it is, the code you have can not work. There are missing semicolons, use of a non existing logger field and calling the constructor of an abstract class... \$\endgroup\$ – bowmore Jan 9 '15 at 20:53
2
\$\begingroup\$

Redundant code

In the constructor of DataExtract (which extends ConnectionUtils) you have

super(jdbcParams);
this.jdbcParams = new JdbcParams(jdbcParams);  

and in the constructor of ConnectionUtils you have

this.jdbcParams = new JdbcParams(env);  

Also, I would expect the name of the parameter be the same so it should be either jdbcParams of env for both.

DataExtract

  • getNodes() I don't see the point of passing a Connection to the method. The DataExtract can create its own connection. If you need any parameters like username and password etc. you should pass this as parameters to the constructor or by using a "property".

  • the DataWriter dw is only used in the getNodes() method and doesn't depend on any class objects. So move it to the getNodes() method.

  • if one read the method name getNodes() one would expect to get Nodes.

DataWriter

  • Instead of doing formatting the output and writing the output in one class you should consider to separate the responsibilites into two classes.

  • Instead of adding String's you should use a StringBuilder

General

  • You should have a Formatter class which takes a ResultSet and returns a String.

  • You should have a Writer class which takes the a formatted String and a filename and returns a boolean indicating success.

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

You never use DataWriter.writeHeader(). Who ever is going to use this file will have a hard time processing it without the header.


Expanding on Heslacher's answer about how the responsibilities are not properly partitioned.

You say that DataExtract is the "query" class (knows how to get data from the database). But for some reason it has to know about File, which deals with the file system. This means the only way to have an application do some processing on the data in the database is to query the database and have the results written to the disk. Then, code is needed to read the format of this custom text file. Then, you can use the data produced by this new file reader class.

Additionally, since you are explicitly passing in the File to write to, it is not possible to write the output anywhere else. What happens if you want to write to stdout or over a network connection? This isn't possible since DataWriter is explicitly creating the streams it operates on.

On top of that, DataExtract explicitly instantiates the class that writes to the file. This means you can only every write the data out in this one format. If you need a different text format of your data, you need to use the above file reader to get the value and write them back in a different format.


ConnectionUtils really wants to be a static utility class, but you make it base class by adding one method that depends on a member variable. There is nothing preventing you from having newConnection() taking in an argument. It seems like you are using it as a base class just to expose the methods to the other classes. This is a bad way to use inheritance and will only lead to problems down the road. Composition is much easier to work with and is better suited for sharing functionality between classes that don't represent the same thing.

\$\endgroup\$

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.