Using static final Strings for SQL Query in Spring Boot

Question

This is regarding code quality and coding standards with respect to Spring Ecosystem. Here there are two methods:

• isRecordExists is using a class level static constant String query. (Definitely suggestable for old way where we use prepared statements)
• isRecordExists2 is using method level String query

Which method is better (isRecordExists or isRecordExists2) in context with Spring Boot?

@Repository
public class SpringRepository { 
 @Autowired
 private NamedParameterJdbcTemplate namedParamJdbcTemplate;
 private static final String QUERY = " select * from Dual where created_date=:date";
 public void isRecordExists(Date date){
 /**
 * using constant for sql Query String at class level
 */
 MapSqlParameterSource parameters = new MapSqlParameterSource();
 parameters.addValue("date", date);
 namedParameterJdbcTemplate.queryForObject(QUERY, parameters, Integer.class);
 }
 public void isRecordExists2(Date date){
 String sqlQuery=" select * from Dual where created_date=:date";
 /**
 * using sql Query at method level
 */
 MapSqlParameterSource parameters = new MapSqlParameterSource();
 parameters.addValue("date", date);
 namedParameterJdbcTemplate.queryForObject(sqlQuery, parameters, Integer.class);
 }

• Stephen Byrne: Unrelated to the existing answers and perhaps only because it's example code, but surely these methods should return something (e.g boolean) as they are Queries, not Commands. Also you surely don't need to select *, you can just select TOP 1 which will be a lot more efficient.

Answer 1

Sorry to present the antithesis to an existing answer once more: if the string is neither reusable nor public, a constant makes no sense in my book. (And yes, this question is opinion-based ;-))

I see why you use a string-variable inside the medthod (to keep it readable for longer strings) which I find totally OK.

In general: aim to write the code for the audience of developers who come after you. In my experience, this future developer will dig through your code with a debugger because something goes wrong. Thus, he will not read the class in its entirity because he's curious, he will jump into that specific method from a stacktrace. All information that is locally there will be beneficial, as there is less jumping around in the code.

• Mohammad Javed: adding to above, Spring actually discourage the use of static members..... rules.sonarsource.com/java/tag/spring/RSPEC-3749
• K.H.: Glad to see reasonable disagreement. Good arguments.

Answer 2

Not many valid reasons, why would ever isRecordExists2 be better method. It is almost always good to extract reusable strings into constants and split code into smaller pieces. I definitely vote for isRecordExists. If you decide to stick with isRecordExists2, why even create sqlQuery variable? You might as well just pass the string itself into queryForObject.

Only possible downsides of using isRecordExists I can think of (but very small and insignificant imho):

• Static field is always there and taking some piece of memory. Local string would not unless while that method is being called.
• That string is not as close to method and therefore code might be harder for read (but we have IDEs to help with that). I would choose better name than QUERY to help with that too.

If there are specific coding standards in Spring boot against it, please link them :-)

• PhilipRoman: I don't think your first point about memory usage is correct - the string will be located in a constant pool regardless of how it's declared.
• Voo: So what's the justification to violate YAGNI? I mean it's almost certain that nobody is ever going to reuse an exact query string in another repository method. After all what would be the point?
• K.H.: There isn't one. If that's only place to be used, there's no point. Agreed.
• K.H.: @PhilipRoman What I mean is, that there will be always be that string in constant pool after this class has been loaded. In second case, it can get removed when not used anywhere.

Answer 3

I agree with @mtj's answer : YAGNI, no point in making the string reusable if it's not being reused at the moment.

I would want to add that in certain scenarios it may be smart to factor out only part of the SQL query.

For example if you had a few queries with slightly more complicated WHERE statements, something like :

select * from image where (created_date is null or created_date > date)

and

select id from text where (created_date is null or created_date > date)

Then we can have

private static String WHERE_CREATED_IS_NULL_OR_AFTER_DATE = " where (created_date is null or created_date > date) "

and use it in the respective methods :

public void getImages() {
 String query = "select * from image" + WHERE_CREATED_IS_NULL_OR_AFTER_DATE;
 ...
}

public void getTextIds() {
 String query = "select id from text" + WHERE_CREATED_IS_NULL_OR_AFTER_DATE;
 ...
}