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.
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.
>> 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.
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 .
Save India From Corruption - Anna Hazare.
posted 9 years ago
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.
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.
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.