Win a copy of Mesos in Action this week in the Cloud/Virtualizaton forum!
  • Post Reply
  • Bookmark Topic Watch Topic
  • New Topic

How would you improve this code?

 
Tom Joiner
Ranch Hand
Posts: 47
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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 ]
 
Bear Bibeault
Author and ninkuma
Marshal
Pie
Posts: 64844
86
IntelliJ IDE Java jQuery Mac Mac OS X
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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 ]
 
Tom Joiner
Ranch Hand
Posts: 47
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
Pie
Posts: 64844
86
IntelliJ IDE Java jQuery Mac Mac OS X
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
Posts: 135
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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. ;)
 
Stefan Evans
Bartender
Posts: 1721
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
Posts: 387
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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.
 
  • Post Reply
  • Bookmark Topic Watch Topic
  • New Topic