wood burning stoves 2.0*
The moose likes JDBC and the fly likes Is it possible to enhance this code further? Big Moose Saloon
  Search | Java FAQ | Recent Topics | Flagged Topics | Hot Topics | Zero Replies
Register / Login


Win a copy of Murach's Java Servlets and JSP this week in the Servlets forum!
JavaRanch » Java Forums » Databases » JDBC
Bookmark "Is it possible to enhance this code further?" Watch "Is it possible to enhance this code further?" New topic
Author

Is it possible to enhance this code further?

senthil kumar m
Greenhorn

Joined: Nov 28, 2010
Posts: 16
The code below is a part of a java file for authentication. I want to know how can i write optimized code for this(especially when i am using database with java what I need to consider). Comment on my code how can I improve it.

Is it up to Standard?

Is it possible to optimize my coder further?



Thanks In advance
Tom Reilly
Rancher

Joined: Jun 01, 2010
Posts: 618
Some suggestions:

1. Rather than bringing back all rows of tablename, you can add a where clause that brings back the row(s) that match(es) the username and password. Look at PreparedStatement to add variables to your SQL statement. You can also bring back fewer columns. That is, don't "select *". Instead specify, for example "select user_id", where user_id is a column name. If no rows are returned then you know the login attempt failed.

2. I notice that you only print "Fail" when an exception occurs. What happens when no rows are returned or no rows match your user name and password?

3. You should use variables that are better named. For example, instead of the variable str1, use userName.

4, Don't hardcode column indexes (rs.getString(8)) You have multiple choices here. Look at using constants (private static final) or enums. You can also use column names instead of column numbers.
Mohamed Sanaulla
Saloon Keeper

Joined: Sep 08, 2007
Posts: 3064
    
  33

>> You also need to close the Connection( if you arent using it further) or close the Statement. Put if in a finally block so that it is executed even if there is an exception.
>> Its better to print/log the stacktrace instead of some string say- "Fail", because a Stacktrace can provide more information and will be easier to debug or you can log some useful message. Also its better not to provide a Catch all block-


>> You can create a User defined exception for Authentication Failed and can throw that If the Authentication is not successful.

>> Its better to declare the connection related Strings at one place. Or better you can have a Factory class to give you a connection object.


Mohamed Sanaulla | My Blog
Alpesh Gediya
Greenhorn

Joined: Nov 23, 2009
Posts: 24
You can further enhance code following way.



Close the system resource in reverse order of its creation. its is good practice to close .. resultset then Statement and Then connection.
put resource release code always inside finally block.


Alpesh Gediya
Do not re-invent the wheel.
Ravi Kiran Va
Ranch Hand

Joined: Apr 18, 2009
Posts: 2234

what i suggest you to use a FindBugs tool and work on the suggestions made by the tool , try to work on the performance Category issues .

Try to run for every piece of code because it might be a lot when ran for the entire Application .

Thanks .


Save India From Corruption - Anna Hazare.
Lester Burnham
Rancher

Joined: Oct 14, 2008
Posts: 1337

if(rs != null)
{
rs.close();
}
if(stmt != null)
{
stmt.close();
}

Note that closing the Statement closes the ResultSet automatically, so there's no need to do it explicitly.

And I agree with mohamed - DB driver names, URLs, username and password should not be hardcoded; they should be stored elsewhere (maybe in a property file) and read at runtime.
Ravi Kiran Va
Ranch Hand

Joined: Apr 18, 2009
Posts: 2234



This way of writing code looks bad , because you need to write the same things again and again en every DAO layer class of your Application so i suggest you write a helper class with containing static methods , and pass these Connection , Statement , ResultSet to it and close them in that Method , then you can simply call this static methods of that class from myour finally block .
Alpesh Gediya
Greenhorn

Joined: Nov 23, 2009
Posts: 24
Instead writing Helper class on your own you can use industry proven solution.

Do not re-invent the wheel.

Apache DBUtil API to handle database related things.

Link to DBUtil http://commons.apache.org/dbutils/
DBUtil class org.apache.commons.dbutils.DbUtils

Ravi Kiran Va
Ranch Hand

Joined: Apr 18, 2009
Posts: 2234

tahnks Alpesh , this link looks good

http://www.devdaily.com/java/jwarehouse/commons-dbutils-1.0/src/java/org/apache/commons/dbutils/DbUtils.java.shtml
senthil kumar m
Greenhorn

Joined: Nov 28, 2010
Posts: 16
after reading all the informations in this thread,I modified my code and below is my new code,

Please comment on this whether i can enhance this further,

Jeanne Boyarsky
internet detective
Marshal

Joined: May 26, 2003
Posts: 30076
    
149

This construct is somewhat inelegant.


Consider this which does the same thing

I like the second one better because it doesn't clutter things up.

In general, I would also move the close logic to a helper class. With an API like close(Connection conn, PreparedStatement pstmt, ResultSet rs). It's just boilerplate code so doesn't add to the meaning of what you are doing. Similarly for getting a connection. For learning purposes, this doesn't matter.


[Blog] [JavaRanch FAQ] [How To Ask Questions The Smart Way] [Book Promos]
Blogging on Certs: SCEA Part 1, Part 2 & 3, Core Spring 3, OCAJP, OCPJP beta, TOGAF part 1 and part 2
senthil kumar m
Greenhorn

Joined: Nov 28, 2010
Posts: 16
I dont understand what is Helper Class? How can i apply that to my program?
senthil kumar m
Greenhorn

Joined: Nov 28, 2010
Posts: 16
Can somebdoy give me a hint about what is helper class , so that I can learn about that
Jeanne Boyarsky
internet detective
Marshal

Joined: May 26, 2003
Posts: 30076
    
149

A helper class is another class that your current class calls. It has a narrower focus. For example, maybe it just deals with getting/closing a connection.
senthil kumar m
Greenhorn

Joined: Nov 28, 2010
Posts: 16
I used the findbugs,tool as you suggested , even it is showing some errors though I corrected those things ,the screen shot is below,

Why these errors appeared? Am I using the tools wrongly ?



The Code is Below



Thanks for your time.
Kumar Raja
Ranch Hand

Joined: Mar 18, 2010
Posts: 518
    
    2

Thinking of extensibility and reuse, why don't you move our connection establishment to some other utility class and use it. Also, for this particular application, assuming if you want to use this in some concurrent request mode, you are creating the connection for every request. May be you can use a pool of pre-created connections and upon every single request, try to get a connection from pool.


Regards
KumarRaja

 
I agree. Here's the link: http://aspose.com/file-tools
 
subject: Is it possible to enhance this code further?
 
Similar Threads
Referencing to another object
String Problem
Substring Challenge/Console input error
== and equals arent clear yet ... look at this
comparing Strings