File APIs for Java Developers
Manipulate DOC, XLS, PPT, PDF and many others from your application.
http://aspose.com/file-tools
The moose likes JSP and the fly likes How would you improve this code? Big Moose Saloon
  Search | Java FAQ | Recent Topics | Flagged Topics | Hot Topics | Zero Replies
Register / Login


Win a copy of Spring in Action this week in the Spring forum!
JavaRanch » Java Forums » Java » JSP
Bookmark "How would you improve this code?" Watch "How would you improve this code?" New topic
Author

How would you improve this code?

Tom Joiner
Ranch Hand

Joined: Sep 19, 2006
Posts: 47
I put this code together to gather data for a "task" in a task list. It has a number of attributes which are all foreign keys to other database tables. Hence, it produces a number of drop down lists which correspond to these.

What would you do differently to improve this?

Close the database connection explicitly?
Close the resultSets explicitly?


[ October 13, 2006: Message edited by: Bear Bibeault ]

SCJP
Bear Bibeault
Author and ninkuma
Marshal

Joined: Jan 10, 2002
Posts: 61413
    
  67

Step 1: Move the code out of the JSP and into a Java class that is independent of the UI.
[ October 13, 2006: Message edited by: Bear Bibeault ]

[Asking smart questions] [Bear's FrontMan] [About Bear] [Books by Bear]
Tom Joiner
Ranch Hand

Joined: Sep 19, 2006
Posts: 47
So lets suppose I did this.. would the java class return a set of lists, a list for every one of the attributes that need to appear in the drop down?

So the java servlet would be invoked, create a set of lists, and then pass this onto a jsp page which would parse out the lists and construct the page?

I have been looking around on web sites for examples and this seems to be the paradigm, although I feel I am missing some key components still.
Bear Bibeault
Author and ninkuma
Marshal

Joined: Jan 10, 2002
Posts: 61413
    
  67

That's pretty close to the idea.

The "big picture" is to keep everything as stand-alone and encapsulated as possible. This makes things not only easier to code, but to re-use, to test, to extend, and to maintain.

With regards to web app structure, have you stumbled across this article that I wrote for the JR Journal? It might be helpful.
Eddy Lee Sin Ti
Ranch Hand

Joined: Oct 06, 2005
Posts: 135
A quick refactor can use the JSTL SQL taglib to remove all the ugly scriptlet. Well, like Bear said, eventually the non-presentation related codes should be moved to other more appropriate layers. ;)


SCJP, SCWCD, SCJWS, IBM 700,IBM 701, IBM 704, IBM 705, CA Clarity Technical<br /> <br /><a href="http://eddyleesinti.blogspot.com" target="_blank" rel="nofollow">http://eddyleesinti.blogspot.com</a>
Stefan Evans
Bartender

Joined: Jul 06, 2005
Posts: 1018
The database queries need to be run in a bean/servlet.
You should open the resultset, run through it and save the results to a List/Map (or whatever datastructure you prefer)

You should then set this as an attribute in appropriate scope.
Most often I use request scope, but that means it needs to be requeried on each request.

In this case, these dropdowns seem to be global and unchanging no matter who is logged into the application. Storing them in the application scope might make sense.
If it is user-specific data that doesn't change while the user is logged in, then session scope would be applicable.
Then rather than iterating over a result set in the jsp, you iterate over a list/map that is in scope.


The other suggestion is one more focused on the HTML rather than the JSP per se.
Use css styles for your formatting, rather than hardcoding in colours, font sizes, bold, alignments etc etc.
It will make the page both simpler to read, and easier to change.
As a quick example:



The other change I would recommend is to use connection pooling on your server rather than open a connection to the DB with each and every JSP/Servlet that runs.
Dan Bizman
Ranch Hand

Joined: Feb 25, 2003
Posts: 387
I don't know the structure of your DB so I can't say for sure but some kind of join statement is probably in order here. At the very least, a select statement from multiple tables would be a good idea too. Putting all those selects into one DB call will save a LOT, as it would be only one DB call instead of 4 and when you're reading the data, you could do it in one loop not 4, and you'd only have to create one resultset, not 4. Sure it might be "cleaner" to keep them all separate, but in a production environment that'd be a LOT slower. Think about it, 1000 requests all generating 4 DB calls and looping through 4 resultsets is a LOT more intensive than 1 per.
 
I agree. Here's the link: http://aspose.com/file-tools
 
subject: How would you improve this code?