I'm updating more than one table at once and, as you will see, it uses a lot of similar looking code. Is there a more efficient way of doing this, perhaps some way of combining the queries?

For context: A user submits a small form containing information about their age, gender and favorite genre of games.

String sex, age;
String[] checkboxes = request.getParameterValues("genre");

sex = request.getParameter("sex");
age = request.getParameter("age");

try(Connection con = ds.getConnection()){
    PreparedStatement updateSex = null;
    PreparedStatement updateAge = null;
    PreparedStatement updateGenre = null;

    String updateSexString = "UPDATE sex SET count=count+1 WHERE name=?";
    String updateAgeString = "UPDATE age SET count=count+1 WHERE id=?";
    String updateGenreString = "";

    for(int i = 0; i < checkboxes.length; i++){

        updateGenreString = "UPDATE genre SET count=count+1 WHERE id=?";
        updateGenre = con.prepareStatement(updateGenreString);
        updateGenre.setString(i+1, checkboxes[i].toString());
        updateGenre.executeUpdate();
        updateGenre.close();
    }

    updateSex = con.prepareStatement(updateSexString);
    updateSex.setString(1, sex);
    updateSex.executeUpdate();
    updateSex.close();

    updateAge = con.prepareStatement(updateAgeString);
    updateAge.setString(1, age);
    updateAge.executeUpdate();
    updateAge.close();

} catch(SQLException e){
    e.printStackTrace();
}
  • 2
    This code will break, whenever there is more than a single checkbox in the genre checkboxes! – Vogel612 Oct 19 '14 at 18:05
  • Ah I see now, it's the assignment of the checkbox, doh! – crm Oct 20 '14 at 14:15
up vote 4 down vote accepted

Declaring Variables and other things:

try (Connection con = ds.getConnection()) {
    PreparedStatement updateSex = null;
    PreparedStatement updateAge = null;
    PreparedStatement updateGenre = null;

    String updateSexString = "UPDATE sex SET count=count+1 WHERE name=?";
    String updateAgeString = "UPDATE age SET count=count+1 WHERE id=?";
    String updateGenreString = "";

In your current approach, multiple prepared statements are not helping anyone.. You can just put the new prepared statements you are performing into the same PreparedStatement currentStatement.

This cuts down your declarations by quite a bit. Next are the update*String variables you have here. These things are Constants. Don't declare them on method-level, they can be reasonably expected to not change. Instead these should be classwide constants:

public class ... {
    private static final String UPDATE_SEX_STRING = "UPDATE sex SET count=count+1 WHERE name=?";
    private static final String UPDATE_AGE_STRING = "UPDATE age SET count=count+1 WHERE id=?";
    // you get the gist ;)

The small and the big wtf:

String updateGenreString = "";

for(int i = 0; i < checkboxes.length; i++){

    updateGenreString = "UPDATE genre SET count=count+1 WHERE id=?";
    updateGenre = con.prepareStatement(updateGenreString);
    updateGenre.setString(i+1, checkboxes[i].toString());

Why didn't you move the assignment to the updateGenreString, that doesn't even change to the declaration?

Additionally: Why is the parameter index depending on your loop counter? That makes absolutely no sense, because in case you have more than a single checkbox, things will break with the current statement!
If there's no second parameter, what happens when you try to set it? You get a SQLException, at least per the JavaDoc.

And another one: Why is the id a String? In general using Strings as ID/UID/UUID is strongly advised against. You're losing out on fast searchability and a few other things.

The id in the age-table also seems to be a String, at least according to the method you call.

Bottom line: Your database schema seems skewed.

String updateSexString = "UPDATE sex SET count=count+1 WHERE name=?";

Just don't do it. Why do you need to count how many times, sex was has been selected? I mean, sex is important, but counting it this way is a bad style.

Moreover, it's prone to forgetting, when you do it manually every time. If you want it efficient, then use a stored procedure doing it all, so there's a single round trip to the database.


Most of the time, storing the data in their original form (possibly after some light preprocessing) makes more sense. The request seems to contain a couple of checkboxes, so store what the user checked. Obtaining some stats (like how many times what was checked) from the DB is usually pretty fast.


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

Not again! Why does everyone believe, that catching an exception and printStackTrace is of any use? Just drop the whole catch clause.

Since the queries are on different tables and columns, I don't think there's a better way to combine them.

Your code can be better by wrapping the PreparedStatements in a try like you did for Connection:

try (Connection con = dataSource.getConnection()) {
    String updateSexString = "UPDATE sex SET count=count+1 WHERE name=?";
    try (PreparedStatement st = con.prepareStatement(updateSexString)) {
        st.setString(1, sex);
        st.executeUpdate();
    }

    String updateAgeString = "UPDATE age SET count=count+1 WHERE id=?";
    try (PreparedStatement st = con.prepareStatement(updateAgeString)) {
        st.setString(1, age);
        st.executeUpdate();
    }
}

As for updating the genre, there are two improvements you could do:

  • Use a bulk update statement if your database backend supports it.
  • Instead of running multiple queries with id=?, you might want to considering running a single query with an in-clause, for example id IN (4, 5, 6). The downside of this is that a prepared statement won't be much help. Actually a bulk update will be better than this.

Partial executions ok?

Consider this: will it be ok if only some of the queries succeed? If not, then you will need to wrap all of this in a transaction, to make sure that either all the updates are applied or none of them.

Other alternatives

I recommend using Spring's JDBC helpers instead of coding these low-level DB operations yourself. That will help with the bulk update stuff as well.

Other minor issues

Don't e.printStackTrace(). Write to a log file instead, perhaps using a logger framework, such as log4j.

Also, in this case, probably you need more than just logging, but implement some kind of graceful handling, for example showing an error message to the user. (If this makes sense in your full program.)

  • Thanks @janos. It's actually good to know it can't be re factored that much - as ugly as it looks. – crm Oct 19 '14 at 14:01
  • 1
    @crm Well, at least I don't think so. But give it some time, maybe somebody else will have a better idea. In any case, this doesn't look all that bad to me. Btw I recommend using Spring's JDBC helpers instead of doing these low-level DB operations yourself. That will help with the bulk update stuff as well. Also, don't e.printStackTrace(), log it instead – janos Oct 19 '14 at 14:04
  • @crm It's ugly, but maybe because it's not exactly what you should/want to do. A single request usually leads to a single DB update, see my answer. – maaartinus Oct 19 '14 at 15:17
  • OT: janos, you've deleted your answer, which I found good (because of the clarity and the tests) and wrote a follow-up. Maybe undeleting it would be a good idea? – maaartinus Oct 19 '14 at 15:21

Different Schema?

I just want to suggest an alternative. It's not necessarily better, and it might even be worse.

You could completely modify your data structure and instead have one table where each entry is one "vote" (or maybe some better name) which would have fields of age, sex, genres (yes/no) and maybe also a timestamp.

You would then add some indices on those quantities. I actually don't know what the performance of this approach would be for fetching the number of 42 years-old as opposed to your approach. Maybe someone could comment on this.

Minor Detail about your Code

I don't like that you define your variables before you need them. I think it makes the code less readable.

String sex = request.getParameter("sex");
String age = request.getParameter("age");
String[] checkboxes = request.getParameterValues("genre");

instead of

String sex, age;
String[] checkboxes = request.getParameterValues("genre");

sex = request.getParameter("sex");
age = request.getParameter("age");

And you do something similar with the PreparedStatements when you declare them as null at the start of the method, but they can never really be null and they are only used later.

Your Answer

 

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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