How to avoid creating multiple string objects in java?

Question

I have a scenario in which I am using a Query object and a StringBuffer, in which I will build an SQL query to be executed.

Here it is:

countQueryBuf.append("Select count(e) From " + clz.getSimpleName()
            + " e");

Here my senior told me that it is bad practice to use string literals like "Select count(e) from ". We can make that:

string public final static selectCount="Select count(e) From "` 

in a interface, and then use that in the string buffer. But after seeing this Interfaces with static fields in java for sharing 'constants' question, which says it is bad practice, I am confused. Can anyone let me know what is the best way to do it and prove my scenario?

Answer 1

First of all, it should be emphasized that the prerequisite of you question is flawed. String literals in Java do not produce multiple String instances, so defining constants pointing to these String instances does not change the number of instances. But, most likely, your senior didn’t intend to discuss the number of instances with that advice.

String literals can be considered constant values like 123 or 44.1f. When such values appear somewhere in your code, they are often called “magic literals”, because they appear spuriously, without a recognizable source. In these cases, using a named constant, whose name explains it’s origin, should be preferred. E.g.

static final float COMPACT_DISC_FREQ_KHZ = 44.1f;

tells you something.

In contrast, constants like

static final int ONE = 1;

don’t tell you anything and are no improvement, only an attempt to fake a better coding style. I consider a constant like

static final String selectCount="Select count(e) From ";

to be of the same nonsensical category, as its name doesn’t tell me anything I can’t already see from the constant value.

---

Whether you place a named constant into an interface or an ordinary class doesn’t make much difference. But in the past, constants were placed into interfaces with the intention of implementing the interface to basically import these constants, to be able to refer them by simple name. It’s not the placement of the constants into the interface, that makes it a bad coding style, but the actually meaningless type inheritance relationship that exists only to save typing in the source code. Since Java 5, you can use import static to place constants into whatever type you wish and refer to them by simple name without creating a questionable inheritance relationship. So in most cases, you don’t want to use an interface for that.

---

As already pointed out by others, there are other issues with your code. StringBuffer has been superseded by StringBuilder for most use cases, further, it doesn’t make much sense to mix String concatenation with either StringBuffer or StringBuilder usage.

Use

countQueryBuf.append("Select count(e) From ").append(clz.getSimpleName()).append(" e");

consistently, if the existing countQueryBuf is needed, i.e. if there are other fragments to append. If the query consists of these three fragments only, code like

String query = "Select count(e) From "+clz.getSimpleName()+" e";

is preferable. Before Java 5, it used a StringBuffer under the hood, starting with Java 5, it will get compiled to use a StringBuilder and starting with Java 9, it will get compiled to use the builtin String concat factory instead. In other words, this simple expression will automatically get the benefit of future improvements when being (re)compiled, whereas dealing with StringBuffer or StringBuilder manually requires maintaining and sometimes rewriting the code to catch up with such development.

If the fragments of a query represent values, you should always use a PreparedStatement instead of assembling a new query string each time…

Answer 2

It's nothing wrong with using String concatenation as in

String query = "Select count(e) From " + clz.getSimpleName() + " e";

As long as you don't do it in loops. In loops or more complex String-assemblies, it's better to use a StringBuilder which is the un-synchronized version of a StringBuffer (and usually you don't need synchronization, unless you have multiple threads creating the String).

StringBuilder b = new StringBuilder().append("Select count(e) From ")
                                    .append(clz.getSimpleName())
                                    .append(" e");
String query = b.toString();

As an alternative, you can use the String format option with a template String.

String query = String.format("Select count(e) From %s e", clz.getSimpleName());

In terms of performance, StringBuilder or concatenation are faster than String format. But there are cases, where format would be better (i.e. define the templates in a property file)

If you're using the same string literal multiple times (i.e. more than twice), it's a good practice to define it as a constant and use the constant. It's not a good practice to put it into an interface, because interfaces are usually not intended for this (unless you're strings are part of a protocol)

I usually put them as private static final String in the same class. If they are used by multiple classes, you may put them in a final Constants class or use enums.

Further, it's a good practice to set an initial capacity of the StringBuilder, which should be any mulitple of 4 that is arround (>) the expected size (see also here) or by using an initial String (the initial size is the size of that String + 16), to improve Memory Allocation.

private final static String SELECT = "select ";
private final static String FROM = " from ";
...

String query = new StringBuilder(32).append(SELECT)
                                    .append("count(e)")
                                    .append(FROM)
                                    .append(clz.getSimpleName())
                                    .append(" e")
                                    .toString();

One final note, building SQL queries as a String only makes sense, when you're querying a database with SQL-like syntax but without other connectivity options (i.e. some timeseries databases, like InfluxDB). Building SQL queries by string concatenation is prone to SQL injection unless you sanitize the input properly. Its better to use a prepared statement or a persistence framework such as jOOQ or Hibernate/JPA

Answer 3

It is not about the string construction. it is on how you create an SQL statement. You should not use string concatination for this task as it is vunarable to SQL injection.

The correct way to do this is to use the PreparedStatement object.

  PreparedStatement updateemp = con.prepareStatement(
     "insert into emp values(?,?,?)");

  updateemp.setInt(1,23);
  updateemp.setString(2,"Frank");
  updateemp.setString(3, "Me");
  updateemp.executeUpdate();

Answer 4

The advice against constants in interfaces doesn't apply to classes. The reason it is (sometimes) bad in interfaces is because it has very low cohesion, that is to say, such interfaces often end up as dumping grounds for constants from all over the program that have very little to do with one another. If you have a class that has some constants which are used in that class, then there is high cohesion; the constants are relevant to the place where they are defined.

But there is a much bigger problem suggested by the code in this question. As it stands, you are only concatenating a table name based on the class name, but if you were to concatenate values taken from user input (say, as parameters to insert or values to use in a WHERE-clause) your code would be vulnerable to a SQL Injection attack.

Please make sure you use a PreparedStatement if you are going to be concatenating any data that can possibly be influenced by the user in your SQL query.