This week's book giveaway is in the OCPJP forum. We're giving away four copies of OCA/OCP Java SE 7 Programmer I & II Study Guide and have Kathy Sierra & Bert Bates on-line! See this thread for details.
Hi, everyone. There is this code I have written and it's very long(3 pages). It works fine, I have not detected any bugs. But the problem is I feel there is a way to write more sophisticated code. I am posting the code and anyone who has some free time to contribute, please examine the code and tell me where I can improve.
Thanks verymuch for your time. It will be a great help to me.
Background: This code is to be embedded in a JSP.
The code operates on 2 tables in the database. bookcopies and waitinglist.
The code is a module for online library management system(my semester project). We have book reservation facility in our system and the reservations are stored in waitinglist table. We have a bookcopies table which has the attributes: bookid,copynumber,borrower,duedate. I case the copy is not presently borrowed, the borrower and duedate will be null. Waitinglist has attributes like bookid,userid(user who reserved the book),listposition,activepassive,daycount.
activepassive is 1 when the waiter is active. activepassive is 0 when the waiter is passive.
If some thing is not understandable in my code, please ask and I shall reply back.
Here is the code
No code in a JSP can be "sophisticated" -- it is no longer acceptable, indeed it is irresponsible, to put code in a JSP at this point. The first step in sophisticating your code is to refactor the code out of the JSP and into a Java class or classes.
- Remove scriptlets from JSP, there should be no java code in JSP, use JSP absolutely for presentation purpose. it just displays data provided by controller
- Move data access code to data access objects - DAO
- Move business logic related code to services
- No data access/business logic code in your servlets/controller
- You controller (or servlet if you are not using any framework) uses services and DAOs and provided data to view (JSP) for display
- Have your domain model and entities instead of writing database table oriented code.
I'll echo the other comments about scriptlet code.
Java code belongs in a java class - and this is 99% java code.
Feedback on the code itself (ignoring the fact that it is in a JSP)
- This code is potentially open to SQL Injection attack. Instead of building up SQL using string concatenation, you should use a Prepared Statement.
- Instead of opening a database connection directly in your code, you should use a JNDI Datasource to get a connection from a connection pool
- I would not recommend using the JDBC-ODBC bridge driver. Use a proper database (eg Oracle, MSSql, MySQL) with a Type 4 JDBC connector.
- This code appears to always execute every single query in line. I would suggest breaking this up into methods, one action per method. You have already broken them up by comments, so extracting them into methods should be trivial.
- Often you are performing 2-3 seperate queries on seperate tables, where a better approach would be to use a single query and join the tables in the query.
SELECT DISTINCT BOOKID FROM USER.WAITINGLIST
SELECT TITLE,EDITION,AVAILABLECOPIES FROM USER.BOOKS WHERE BOOKID=
Could be written as one query:
- You don't seem to be releasing your database connection, though you seem pretty good about closing resultset/statement. My recommendation would be that you always release these resources in a finally block after a try/catch, so that they do get closed in the event of an exception.