Forum:

How would you improve this code?

Ranch Hand

Posts: 47

posted 17 years ago

Number of slices to send:

Optional 'thank-you' note:

Send

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?

<%@page contentType="text/html"%>
<%@page pageEncoding="UTF-8"%>
<%@page import="java.sql.*" %>
<%--
The taglib directive below imports the JSTL library. If you uncomment it,
you must also add the JSTL library to the project. The Add Library... action
on Libraries node in Projects view can be used to add the JSTL 1.1 library.
--%>
<%--
<%@taglib uri="http://java.sun.com/jsp/jstl/core" prefix="c"%> 
--%>
 
<%
    Connection connection = null;
    Statement typeStatement = null;
    Statement statusStatement = null;
    Statement productStatement = null;
    Statement accountStatement = null;
 
    String set_header_color = "bgcolor= \"#CCCCFF\"";
    String set_row_color = "bgcolor = \"#c8d8f8\"";
    String name;
    String description;
    String id;
    
    int priority = 0;
    
    
    ResultSet typeSet = null;
    ResultSet statusSet = null;
    ResultSet productSet = null;
    ResultSet accountSet = null;
 
    
    Class.forName("org.gjt.mm.mysql.Driver").newInstance(); 
    
    connection = DriverManager.getConnection("jdbc:mysql://localhost:3306/database01", "user", "password"); 
    
    typeStatement = connection.createStatement(); 
    statusStatement = connection.createStatement();
    productStatement = connection.createStatement();
    accountStatement = connection.createStatement();
 
    
    typeSet = typeStatement.executeQuery("select * from task_type");
    statusSet = statusStatement.executeQuery("select * from task_status");
    productSet = productStatement.executeQuery("select * from product");
    accountSet = accountStatement.executeQuery("select * from account");
    
    %>
 
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"
   "http://www.w3.org/TR/html4/loose.dtd">
 
<html>
    <head>
        <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
        <title>JSP Page</title>
    </head>
    <body>
 
 
   
  <form name="submitTask" action="${pageContext.request.contextPath}/Task/Create.jsp" method=post>
<center>
<table cellpadding=6 cellspacing=4 border=0>
 
<th <%=set_header_color %> colspan=2>
<font size=5>Create Task</font>
<br>
</th>
 
<tr <%=set_row_color %>>
<td valign=top align=right>
<b>Title</b></td><td>
<input type="text" name="Title"  size=50 maxlength=30></td>
</tr>
 
<tr <%=set_row_color %>>
<td  valign=top align=right>
<b>Description</b></td><td>
<textarea cols="50" rows="10" name="description">
</textarea>
</tr>
 
<tr <%=set_row_color %>>
<td>
</td>
<td>
 
<table>
<tr><td align=center>Type</td><td align=center>Status</td>
<td align=center>Product</td><td align=center>Assigned</td>
<td align=center>Requested by</td><td align=center>Priority</td>
</tr>
<tr>
<td>
<select name="taskType">
<%
   while (typeSet.next() == true)
   {
        id = typeSet.getString("task_type_id");
        name = typeSet.getString("name");
        %>
        <option value="<%=id%>"><%=name%></option>
        <%
   }
 %>
</select>
</td>
<td>
<select name="taskStatus">
<%
   while (statusSet.next() == true)
   {
        id = statusSet.getString("task_status_id");
        name = statusSet.getString("name");
        %>
        <option value="<%=id%>"><%=name%></option>
        <%
   }
 %>
</select>
</td>
<td>
<select name="product">
<%
   while (productSet.next() == true)
   {
        id = productSet.getString("product_id");
        name = productSet.getString("name");
        %>
        <option value="<%=id%>"><%=name%></option>
        <%
   }
 %>
</select>
</td>
<td>
<select name="assignedTo">
<%
   while (accountSet.next() == true)
   {
        name = accountSet.getString("first_name");
        name = name + " ";
        name = name + accountSet.getString("last_name");
        id = accountSet.getString("account_id");
        %>
        <option value="<%=id%>"><%=name%></option>
        <%
   }
 %>
</select>
</td>
<td>
<select name="requestedBy">
<%
   accountSet.first();
   do 
   {
        name = accountSet.getString("first_name");
        name = name + " ";
        name = name + accountSet.getString("last_name");
        id = accountSet.getString("account_id");
        %>
        <option value="<%=id%>"><%=name%></option>
        <%
   } while (accountSet.next() == true);
 %>
</select>    
</td>
<td>
<select name="priority">
<%
    for (priority = 1; priority <= 10; priority++)
    {
       %>
        <option value="<%=priority%>"><%=priority%></option>
        <%
    }
 %>
</select>
</td>
<td>
</tr>
</table>
</td>
</tr>
</table>
  </form>
    </body>
</html>

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

SCJP

Bear Bibeault

Sheriff

Posts: 67747

173

I like...

posted 17 years ago

Number of slices to send:

Optional 'thank-you' note:

Send

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] [About Bear] [Books by Bear]

HTML 5 (verified skill)
Skill verified by Jeanne Boyarsky

CSS 3 (verified skill)
Skill verified by Jeanne Boyarsky

JavaScript (verified skill)
Skill verified by Jeanne Boyarsky

Tom Joiner

Ranch Hand

Posts: 47

posted 17 years ago

Number of slices to send:

Optional 'thank-you' note:

Send

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.

SCJP

Bear Bibeault

Sheriff

Posts: 67747

173

I like...

posted 17 years ago

Number of slices to send:

Optional 'thank-you' note:

Send

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.

[Asking smart questions] [About Bear] [Books by Bear]

Eddy Lee Sin Ti

Ranch Hand

Posts: 135

posted 17 years ago

Number of slices to send:

Optional 'thank-you' note:

Send

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

Posts: 1845

posted 17 years ago

Number of slices to send:

Optional 'thank-you' note:

Send

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

posted 17 years ago

Number of slices to send:

Optional 'thank-you' note:

Send

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.

Get me the mayor's office! I need to tell him about this tiny ad:

a bit of art, as a gift, that will fit in a stocking

https://gardener-gift.com