• Post Reply
  • Bookmark Topic Watch Topic
  • New Topic

Is it possible to enhance this code further?

 
senthil kumar m
Greenhorn
Posts: 16
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
Posts: 618
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
Posts: 3159
33
Google App Engine Java Ruby
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
>> 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.

 
Alpesh Gediya
Greenhorn
Posts: 24
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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.
 
Ravi Kiran Va
Ranch Hand
Posts: 2234
Eclipse IDE Firefox Browser Redhat
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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 .
 
Lester Burnham
Rancher
Posts: 1337
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

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
Posts: 2234
Eclipse IDE Firefox Browser Redhat
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator


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
Posts: 24
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
Posts: 2234
Eclipse IDE Firefox Browser Redhat
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
Posts: 16
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
author & internet detective
Marshal
Posts: 34973
379
Eclipse IDE Java VI Editor
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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.
 
senthil kumar m
Greenhorn
Posts: 16
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I dont understand what is Helper Class? How can i apply that to my program?
 
senthil kumar m
Greenhorn
Posts: 16
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Can somebdoy give me a hint about what is helper class , so that I can learn about that
 
Jeanne Boyarsky
author & internet detective
Marshal
Posts: 34973
379
Eclipse IDE Java VI Editor
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
Posts: 16
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
Posts: 547
2
Hibernate Java Spring
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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.

 
  • Post Reply
  • Bookmark Topic Watch Topic
  • New Topic