Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Before, I had this code, sometimes it is longer:

String dbEntry = "idBldgInfo = '" + currentBldgID + "',scenario = '305',installCost='" +
installCost + "',annualSave='" + annualSave + "',simplePayback='" + simplePayback + 
"',kwhPre='" + preKWH + "',kwhPost='" + postKWH + "',co2Pre='" + preCO2e + "',co2Post='" +
postCO2e + "',mbtuPre='" + preMBtu + "',mbtuPost='" + postMBtu + "',shortDescription='" +
econ4ShortDescription +  "',category='" + category + "',longDescription='" +
econ4LongDescription + "'";

I refactored it into:

String newDbEntry = 
    getDBEntryFromDBPairs(
        new DBPair("idBldgInfo", currentBldgID), new DBPair("scenario", "305"),
        new DBPair("installCost", installCost), new DBPair("annualSave", annualSave),
        new DBPair("simplePayback", simplePayback), new DBPair("kwhPre", preKWH),
        new DBPair("kwhPost", postKWH), new DBPair("co2Pre", preCO2e),
        new DBPair("co2Post", postCO2e), new DBPair("mbtuPre", preMBtu),
        new DBPair("mbtuPost", postMBtu), new DBPair("shortDescription", econ4ShortDescription),
        new DBPair("category", category), new DBPair("longDescription", econ4LongDescription));

It looks only slightly better. Is it worth changing it?

The getDBEntryFromDBPairs method is much more efficient since it uses a StringBuilder. But IDK. Any thoughts/suggestions are appreciated.

share|improve this question
1  
What are you going to do with that dbEntry? Be sure you are not going to concatenate it with other strings to build a SQL query, it is extremely risky and will lead to SQL Injections. –  mariosangiorgio Jan 10 '13 at 15:39
    
@Mariosangiorgio All values are locally computed, the only one that is not is currentBldgID. currentBldgID is valided in the front-end. –  jsn Jan 10 '13 at 15:43
1  
What are you using to connect to the database? Usually you can create a prepared statement and then use the APIs to create the bindings between the columns in the database and the values you want to store. Moreover, why aren't you using an Object Relational Mapping framework that would take care of persistence for you? –  mariosangiorgio Jan 10 '13 at 15:48
    
@mariosangiorgio I am trying to make the best out of the code I inherited. I have not used Hibernate before, but might be a good learning experience. Thank you. –  jsn Jan 10 '13 at 15:54
    
Are you working on some legacy code? Be aware that introducing Hibernate in the project you will probably need to do a lot of refactoring. –  mariosangiorgio Jan 10 '13 at 16:27
add comment

2 Answers

I think your second approach is better. It is more readable and more easily maintained. I don't have much experience with databases, but lets say (sorry if its a stupid example) there is a database that causes problems for strings like "colId = colValue", and to fix it you need to remove the spaces. How many places there would be where you will go through String dbEntry = ...; statements to correct this problem? What if for a certain database you need to use : instead of =? Using data structures like this is definitely going to make your life a lot easier in such cases.

So the conclusion is, are there any such cases that you need to consider where you might need to go through your code and change the resultant string? Because changing DBPair's implementation once would be a lot easier.

share|improve this answer
add comment

Your first implemenattion also uses StringBuilder under the hood. See javadoc of java.lang.String:

The Java language provides special support for the string concatenation operator ( + ), and for conversion of other objects to strings. String concatenation is implemented through the StringBuilder(or StringBuffer) class and its append method

So you cannot expect an speed improvement by your second version.

share|improve this answer
    
Well, JLS only says it may do that. So why not. –  jsn Jan 10 '13 at 18:13
    
Experience shows it is done. –  MrSmith42 Jan 10 '13 at 18:51
    
I tried to make a test. But JIT optimizes the hell out of it, so the + concatenation turns out faster. –  jsn Jan 10 '13 at 19:00
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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