6
\$\begingroup\$

This question is related to my question on Stack Overflow earlier today. After getting suggestions on using the PreparedStatement from Java I changed my code a little.

EDIT: In my original question on Stack Overflow I attempted to process the query in a very messy way, see this short excerpt:

String sqlQuerry = "INSERT INTO TASKS " + "VALUES ("
        + table.getValueAt(i, 0) + ", "
        + table.getValueAt(i, 1) + ", "
        + table.getValueAt(i, 2) + ", "
        + table.getValueAt(i, 3) + ", "

Even though this might have worked, it would have been harder to understand and I ran into issues when the table.getValueAt(X,Y) returned "null".

As I am completely new to SQL in Java I want to know if the code below is "good" or at least "okayish" and can be maintained later on. If you got any tips on how to make this cleaner, let me know.

String sqlInsert = "INSERT INTO TASKS " + "VALUES" + "(?,?,?,?,?,?,?,?,?,?,?,?,?)";

for (int i = 0; i < table.getModel().getRowCount(); i++) {

    PreparedStatement sqlState = conn.prepareStatement(sqlInsert);

    int j = 1;

    sqlState.setInt(j++, Integer.parseInt(table.getValueAt(i, 0).toString()));
    sqlState.setInt(j++, Integer.parseInt(table.getValueAt(i, 1).toString()));
    sqlState.setInt(j++, Integer.parseInt(table.getValueAt(i, 2).toString()));
    sqlState.setInt(j++, Integer.parseInt(table.getValueAt(i, 3).toString()));

    sqlState.setString(j++, table.getValueAt(i, 4).toString());
    sqlState.setString(j++, table.getValueAt(i, 5).toString());
    sqlState.setString(j++, table.getValueAt(i, 6).toString());
    sqlState.setString(j++, table.getValueAt(i, 7).toString());

    sqlState.setInt(j++, Integer.parseInt(table.getValueAt(i, 8).toString()));

    sqlState.setString(j++, table.getValueAt(i, 9).toString());
    sqlState.setString(j++, table.getValueAt(i, 10).toString());
    sqlState.setString(j++, table.getValueAt(i, 11).toString());
    sqlState.setBoolean(j++, (boolean) table.getValueAt(i, 12));

    sqlState.executeUpdate();
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Hi, you should include the summary/overview from your SO question here too. \$\endgroup\$ – Kaz Sep 10 '15 at 12:26
  • \$\begingroup\$ Hope this helps. \$\endgroup\$ – Flatron Sep 10 '15 at 12:31
  • \$\begingroup\$ I meant the part about What your code is trying to do, in the wider context :) context is very important for us to give good feedback. \$\endgroup\$ – Kaz Sep 10 '15 at 12:46
8
\$\begingroup\$

There are some things to improve in this code. Especially when it's underlined that "if it can be maintained later on".

SQL query

The columns where the data will be inserted should always be defined, for example: INSERT INTO TASK(id_task, title, creation_time... etc) VALUES(... . It's the same problem as for SELECT *, that should never be used in serious queries. That's because the table definition evolves over time; tomorrow there may be new columns added, other removed or reordered. There is absolutely no warranty that the meaning of what is behind the question marks in the example will be valid after the next feature implemented into the software. Another reason to define them is just to help yourself: it's very difficult to remember which is the natural order of columns in the table for the thirteen question marks.

PreparedStatement

There is no need to recreate the PreparedStatement instance on each loop. Indeed, one of the reasons they are made for is the possibility to be reused. And their creation is sometimes quite costly. The third line should come just before the for loop.

Parameter Indices

From the point of view of maintenability, calculating/incrementing the parameter index (j variable) is not safe. It's just almost like the columns ordering issue above. If j is incremented, the order of the lines with parameters setting is crucial and prone to errors. To solve, some readable (local) constants may be introduced, for example:

final int COLUMN_ID = 1;
final int COLUMN_TITLE = 2;

...
sqlState.setInt(COLUMN_ID, id);

Parameters Mapping

There is absolutely no way to know the meaning of the values that are retrieved from the table. Since it looks like a JTable object, here is a way how an explicit mapping can be created.

Define a TableModel by extending, for example, the javax.swing.table.DefaultTableModel and map the values you need with their respective getters:

class TaskTableModel extends DefaultTableModel {
    ...
    public String getTitle(int rowIndex) {
        return getValueAt(rowIndex, 2);
    }
}

When the JTable object is created, set a TaskTableModel instance on it:

JTable table = new JTable(...);
table.setModel(new TaskTableModel());

When setting the parameter values of the PreparedStatement, use the TaskTableModel instance:

TaskTableModel tableModel = (TaskTableModel) table.getModel();
...
sqlState.setInt(COLUMN_TITLE, tableModel.getTitle(i));
\$\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.